-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix pointcloud map_to #2371
fix pointcloud map_to #2371
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor suggestions inline.
I would really appreciate to have an extra-session before merge - to go through the core changes
src/proc/pointcloud.cpp
Outdated
if (_other_stream != nullptr && other.get_profile().as<rs2::video_stream_profile>() == *_other_stream.get()) | ||
if (_stream_filter != _prev_stream_filter) | ||
_other_stream = nullptr; | ||
_prev_stream_filter = _stream_filter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be encapsulated with the previous line under the "if.." statement
src/proc/pointcloud.cpp
Outdated
_other_stream = nullptr; | ||
_prev_stream_filter = _stream_filter; | ||
|
||
if (_extrinsics.has_value() && _other_stream != nullptr && other.get_profile().as<rs2::video_stream_profile>() == *_other_stream.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using "(!_extrinsics.has_value() || .." would be more readable, i.e. maintainable
src/proc/pointcloud.cpp
Outdated
return; | ||
|
||
_other_stream = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "if" in the next line is redundant now, and should be removed to make it straight-forward: i.e:
_other_stream = <override_value>
@@ -61,8 +61,8 @@ namespace librealsense | |||
_holes_filling_mode(holes_fill_def), | |||
_holes_filling_radius(0) | |||
{ | |||
_stream_filter = RS2_STREAM_DEPTH; | |||
_stream_format_filter = RS2_FORMAT_Z16; | |||
_stream_filter.stream = RS2_STREAM_DEPTH; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to be move to initialization list, so if one day one other class member become dependent on this struct, the initialization order will be preserved, or at least compiler warning will be generated
src/proc/synthetic-stream.cpp
Outdated
@@ -217,16 +214,7 @@ namespace librealsense | |||
rs2_stream stream = profile.stream_type(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those three lines seem to be redundant after refactoring
return true; | ||
} | ||
|
||
bool operator!=(const stream_filter& other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the == and ~= operator be swapped, i.e to implement != by terms of == ?
The (!(!(something)) is really counter-intuitive and hard to maintain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well accomplished
This PR fixing an issue in the pointcloud's "map_to" API