Skip to content
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

Merged
merged 2 commits into from Sep 16, 2018

Conversation

matkatz
Copy link
Contributor

@matkatz matkatz commented Sep 9, 2018

This PR fixing an issue in the pointcloud's "map_to" API

Copy link
Collaborator

@ev-mp ev-mp left a 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

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;
Copy link
Collaborator

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

_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())
Copy link
Collaborator

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

return;

_other_stream = nullptr;
Copy link
Collaborator

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;
Copy link
Collaborator

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

@@ -217,16 +214,7 @@ namespace librealsense
rs2_stream stream = profile.stream_type();
Copy link
Collaborator

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)
Copy link
Collaborator

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

Copy link
Collaborator

@ev-mp ev-mp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well accomplished

@dorodnic dorodnic merged commit b8fc6d4 into IntelRealSense:development Sep 16, 2018
@matkatz matkatz deleted the fix_pointcloud_map_to branch December 23, 2018 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants