Migrate Wrench Display #396
Conversation
Martin-Idel
force-pushed
the
feature/migrate_wrench_display
branch
from
71c1b35 to
255bc01
Compare
- make visual update length when user updates it in the display panel
Martin-Idel
force-pushed
the
feature/migrate_wrench_display
branch
from
255bc01 to
2736ebf
Compare
| { | ||
|
|
||
|
|
||
| class RVIZ_DEFAULT_PLUGINS_PUBLIC WrenchStampedDisplay : public |
There was a problem hiding this comment.
I don't think there's anything to do in this PR, but I noticed we're not including "Stamped" in the name of classes for other stamped message types (except PointStamped). Perhaps we should consider renaming this (and the PointStamped) plugin to remove "Stamped" from the name for consistency. Or, add "Stamped" to the other plugins for stamped messages.
There was a problem hiding this comment.
I agree. I could change it for the WrenchDisplay in this PR if you want because it's not yet on master. For the PointStamped, we should definitely have a new PR and we probably also need a workaround for older configs.
rviz_rendering/include/rviz_rendering/objects/wrench_visual.hpp
Outdated
Show resolved
Hide resolved
| EXPECT_THAT(force_arrow->getScale(), Vector3Eq(Ogre::Vector3(0.2f, 3, 0.2f))); | ||
| } | ||
|
|
||
| TEST_F(WrenchVisualTestFixture, setWrench_updates_force_array_correctly) { |
There was a problem hiding this comment.
What is this supposed to be testing? It looks identical to the previous test.
| auto wrench_visual = std::make_shared<rviz_rendering::WrenchVisual>(scene_manager, root_node); | ||
|
|
||
| wrench_visual->setForceScale(1); | ||
| wrench_visual->setWidth(0.2f); |
There was a problem hiding this comment.
It would also be nice to test the case when the width is greater than the arrow length.
There was a problem hiding this comment.
I rewrote the second test to do just that. I think I wanted to do this and therefore copied the first but never actually changed it...
|
Thanks for the review. I agree with the naming problems. Should I just change the WrenchDisplay name here? |
I think that would be good. Then we only have the PointStamped message to deal with later. |
- other displays don't include the "stamped" part in their name
Closes #91
As before:
CI: