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

retain width and height after resize for master #193

Merged

Conversation

tonylitianyu
Copy link
Contributor

Reopen a pull request for the master branch. The width and height were modified after this resize call. Now the width and height will be retained if they were set before. A new method is also provided to input the set width and height.

Signed-off-by: tonylitianyu tonylitianyu@gmail.com

Signed-off-by: tonylitianyu <tonylitianyu@gmail.com>
@jacobperron jacobperron self-requested a review May 25, 2022 20:11
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!
Could you also add test to this file for the resize methods?

sensor_msgs/include/sensor_msgs/point_cloud2_iterator.hpp Outdated Show resolved Hide resolved
Signed-off-by: tonylitianyu <tonylitianyu@gmail.com>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Tests look good to me, thanks for adding them.
I have a few minor comments; after those are addressed I we should trigger CI before merging.

sensor_msgs/include/sensor_msgs/point_cloud2_iterator.hpp Outdated Show resolved Hide resolved
sensor_msgs/test/test_pointcloud_iterator.cpp Outdated Show resolved Hide resolved
Signed-off-by: tonylitianyu <tonylitianyu@gmail.com>
@tonylitianyu tonylitianyu force-pushed the tonylitianyu/fix_point_cloud_size_master branch from 04b7adc to ec845d1 Compare May 31, 2022 17:56
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

@mjeronimo
Copy link
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@tonylitianyu
Copy link
Contributor Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@jacobperron jacobperron merged commit 65e9cc4 into ros2:master Jun 3, 2022
@Blast545
Copy link
Contributor

Blast545 commented Jun 7, 2022

👨‍🌾 It seems this PR introduced some test regressions on the windows debug buildfarm jobs:
https://ci.ros2.org/view/nightly/job/nightly_win_deb/2381/msbuild/folder.-1615888395/

I think in the resize function, instead of using size_t, you should be using uint32_t. Can I ask you to take a look? @tonylitianyu

@tonylitianyu
Copy link
Contributor Author

new PR added for the type issue
#194

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

4 participants