Python wrapper - Explicit config enable_stream(...) to have unique arg count (API CHANGE)
#11861
Conversation
Nir-Az
changed the title
enable_stream(...) to have unique arg countenable_stream(...) to have unique arg count (API CHANGE)
|
I think one of our examples has one call with only one argument... have you checked? |
Missed that one, fixed |
| @@ -18,7 +18,7 @@ | |||
| pipe = rs.pipeline() | |||
| config = rs.config() | |||
| # Enable depth stream | |||
| config.enable_stream(rs.stream.depth) | |||
| config.enable_stream(rs.stream.depth, rs.format.z16, 30) | |||
There was a problem hiding this comment.
The old version was the one that was (stream_type,stream_index=-1), right? This is another version of enable_stream, right? Is it equivalent? Just want to make sure...
There was a problem hiding this comment.
Correct this use "stream_type"_a, "format"_a, "framerate"_a)
I will also run the example before merging it
There was a problem hiding this comment.
Verified
I wonder if we should add a python only 1 argument function.
stream type only.
Normally the user doesn't care about stream index.
Thoughts?
There was a problem hiding this comment.
It's an option, if we want better backwards compatibility. (and usage, I guess)
Should I wait before approving, then?
wrappers/python/pyrs_pipeline.cpp
Outdated
| @@ -48,6 +48,9 @@ void init_pipeline(py::module &m) { | |||
| "Upon calling resolve(), the config checks for conflicts between the application configuration requests and the attached computer vision " | |||
| "modules and processing blocks requirements, and fails if conflicts are found.\n" | |||
| "Before resolve() is called, no conflict check is done.", "stream_type"_a, "stream_index"_a, "width"_a, "height"_a, "format"_a, "framerate"_a) | |||
|
|
|||
|
|
|||
| .def("enable_stream", []( rs2::config* c, rs2_stream s ) -> void { return c->enable_stream( s, -1 ); }, "Stream type only. Other parameters are resolved internally.", "stream_type"_a ) | |||
There was a problem hiding this comment.
Why the explicit -1 instead of using the C++ enable_stream that takes only 1 arg? (i.e., it has a default second arg)
There was a problem hiding this comment.
Or it can be:
.def("enable_stream", (void (rs2::config::*)(rs2_stream)) &rs2::config::enable_stream, ...)
No? Not sure it'll work -- but if not, then just remove the explicit -1
There was a problem hiding this comment.
It can be yes, I wanted to show that I explicitly call this command arguments but without it it will also work the same.
Do you think it's preferred?
There was a problem hiding this comment.
The only thing I don't like is the explicit -1, because it presumes to know what the default is. Up to you how to do it.
Python cannot distinguish between enums, all enums are treated as ints.
So we cannot have 2 similar functions with the same arg count that only change the enum type.
This PR enforce no default arguments in
enable_stream()functions (luckily we have only unique counts on those)