Skip to content

[image_view] Add dynamic reconfigure to image_nodelet.cpp in melodic#504

Merged
SteveMacenski merged 3 commits intoros-perception:melodicfrom
708yamaguchi:add-dynamic-reconfigure
Mar 20, 2020
Merged

[image_view] Add dynamic reconfigure to image_nodelet.cpp in melodic#504
SteveMacenski merged 3 commits intoros-perception:melodicfrom
708yamaguchi:add-dynamic-reconfigure

Conversation

@708yamaguchi
Copy link
Copy Markdown

I added dynamic reconfigure to image_nodelet.cpp in melodic, which is removed in #479 .
We can select do_dynamic_scaling, colormap, min_image_value and max_image_value.

The converted image is published as ~/output topic.

], "colormap")

gen.add('do_dynamic_scaling', bool_t, 0, 'Do dynamic scaling about pixel values or not', False)
gen.add('do_dynamic_scaling', bool_t, 0, 'Do dynamic scaling about pixel values or not', True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please turn back to false, don't change default behavior please

{
// We want to scale floating point images so that they display nicely
bool do_dynamic_scaling = (msg->encoding.find("F") != std::string::npos);
bool do_dynamic_scaling = do_dynamic_scaling_ & (msg->encoding.find("F") != std::string::npos);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you intend to do a bitwise operation, or do you actually mean &&?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the message is asking for scaling because its a float image, shouldn't we always do it? Maybe the logic should be that it does scaling for floats, but then it checks the do_dynamic_scaling_ for non-float images to do scalling on those?

@708yamaguchi
Copy link
Copy Markdown
Author

708yamaguchi commented Mar 19, 2020

Thank you for your kind review.

I understand that do_dynamic_scaling in imageCb() should be:

  • (for float images) always true.
  • (for non-float images) true only when do_dynamic_scaling_ is true.

I pushed an additional commit based on your advice.

Copy link
Copy Markdown
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

@JWhitleyWork take a look.

Copy link
Copy Markdown
Collaborator

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

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

Thank you for adding this functionality back to the node. LGTM.

@SteveMacenski SteveMacenski merged commit 9dd6ce3 into ros-perception:melodic Mar 20, 2020
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.

3 participants