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

Add plotter 2D for arbitrary message types and fields #20

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

rcp1
Copy link

@rcp1 rcp1 commented Nov 23, 2024

Ported the jsk plotter 2D.

I've enhanced it with ros_babel_fish to subscribe to any message type and extract any field which is implicitly convertable to double.

@authaldo authaldo requested a review from ottojo December 24, 2024 16:06
Copy link
Member

@ottojo ottojo left a comment

Choose a reason for hiding this comment

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

hi! thank you so much for the contribution, and thank you for your patience until i finally review this.
I looked at the diff to the original JSK implementation, that looks good.

I like the ability to access numeric fields of any topic, but i don't want to merge that as is for the following reason: The "easy" use case of selecting a F32 topic in the "create visualization" dialog ("Add" button) does not create a working display, and selecting a F32 topic from the "Topic" drop down does not work (it does not show any topics).
The first issue is due to required "topic field" and "topic message type" properties which are empty by default (which is never a valid state), the second is related, but may just be a missing filter for F32 message type in the RosTopicProperty (i'm not very familiar, and it doesnt seem to have any docs?)

If those issues are harder to work around than i think (maybe due to ros/rviz expecting a fixed message type) i would prefer just sticking with F32 message for now, trading some flexibility for user experience.
I think the average rviz UX is already rather bad and can't really afford even the smallest papercuts...

Last, please add a brief sentence to the docs about the capability/necessity to select a message field and specifying the topic type.

-Jonas

Comment on lines +47 to +49
/** @brief Convenience typedef so subclasses don't have to change their implementation compared to
* templated topic display. */
typedef RosBabelFishTopicDisplay RTDClass;
Copy link
Member

Choose a reason for hiding this comment

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

i dont understand this, it seems unused?

#include <rviz_common/properties/float_property.hpp>
#include <rviz_common/properties/int_property.hpp>
#include <rviz_common/properties/string_property.hpp>
#include <rviz_common/properties/ros_topic_property.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

unused

@rcp1
Copy link
Author

rcp1 commented Jan 25, 2025

hi! thank you so much for the contribution, and thank you for your patience until i finally review this. I looked at the diff to the original JSK implementation, that looks good.

I like the ability to access numeric fields of any topic, but i don't want to merge that as is for the following reason: The "easy" use case of selecting a F32 topic in the "create visualization" dialog ("Add" button) does not create a working display, and selecting a F32 topic from the "Topic" drop down does not work (it does not show any topics). The first issue is due to required "topic field" and "topic message type" properties which are empty by default (which is never a valid state), the second is related, but may just be a missing filter for F32 message type in the RosTopicProperty (i'm not very familiar, and it doesnt seem to have any docs?)

If those issues are harder to work around than i think (maybe due to ros/rviz expecting a fixed message type) i would prefer just sticking with F32 message for now, trading some flexibility for user experience. I think the average rviz UX is already rather bad and can't really afford even the smallest papercuts...

Last, please add a brief sentence to the docs about the capability/necessity to select a message field and specifying the topic type.

-Jonas

Good catches, yeah, I did not have the "easy" use case in mind and rather replaced it with the more generic use case of allowing to plot any numeric field in an arbitrarily typed msg. Also I did not really care about the rviz2 UX experience, since as you said, it is already broken in many ways...

If I just set the defaults to the "easy" use case (so Float32 msg type and data as field) and try to discover dynamically such typed topics for showing them in the dropdown list, would you think this is sufficient?

Only the dynamic discovery is probably a bit harder to work around.

@ottojo
Copy link
Member

ottojo commented Jan 25, 2025

If I just set the defaults to the "easy" use case (so Float32 msg type and data as field) and try to discover dynamically such typed topics for showing them in the dropdown list, would you think this is sufficient?

yes, very much so, that would be perfect!

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.

2 participants