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

generalize WrenchVisual #982

Merged

Conversation

rhaschke
Copy link
Contributor

This PR attempts to increase code-reusability for rviz::WrenchStampedVisual, which is currently used by the wrench display plugin.
The visual itself doesn't handle or use the timestamp. Hence I suggest to:

  • change the API of setMessage() to accept a geometry_msgs::Wrench 1c341c0
  • rename setMessage() to setWrench() 1c341c0
  • add an additional method setWrench(Vector3 force, Vector3 torque) 24e4bab

Furthermore, I modified the code to allow for

  • hiding the markers e78ebee
  • auto-hiding the force or torque marker if their magnitude becomes too small (which created strange artifacts in existing code) 7a51333

@wjwwood
Copy link
Member

wjwwood commented Apr 3, 2016

@rhaschke I'll have to consider this for Kinetic-only. I think there's a good change others are already using the API as-is in Indigo/Jade.

@rhaschke
Copy link
Contributor Author

rhaschke commented Apr 4, 2016

@wjwwood Targeting kinetic is fine. To maintain API stability, I can add the original setMessage() function, which would be a simple wrapper then.
If you agree to this idea, I could prepare an API-compatible version targeted to indigo, depreacating the critical API functions for later removal in kinetic?

@wjwwood
Copy link
Member

wjwwood commented Apr 4, 2016

@rhaschke that would work fine. I would consider releasing for Indigo if this only added to the current API.

@rhaschke rhaschke force-pushed the generalize-wrench-visual branch from 7a51333 to aea6a01 Compare April 6, 2016 07:35
@rhaschke
Copy link
Contributor Author

rhaschke commented Apr 6, 2016

@wjwwood I fixed my changes to retain API and ABI compatibility.
If you have merged this into Indigo, I will create another PR for Kinetic, merging and cleaning up this, i.e. removing the deprecated function and renaming the class WrenchStampedVisual to WrenchVisual.

@wjwwood
Copy link
Member

wjwwood commented Apr 7, 2016

lgtm, I'm going to squash it somehow.

@wjwwood wjwwood merged commit 0cdf1ee into ros-visualization:indigo-devel Apr 7, 2016
wjwwood pushed a commit that referenced this pull request Apr 7, 2016
* whitespace normalization in wrench_display.* + wrench_visual.*

* generalized WrenchStampedVisual::setMessage()

- interface changed to receive a Wrench& only
- renamed setMessage() to setWrench()

* alternative API: WrenchStampedVisual::setWrench(OgreVector3 force, OgreVector3 torque)

* expose SceneNode::setVisible()

* separate scene nodes for force and torque marker

... to allow hiding of arrows if they become shorter than wide

* retain API compatibility

* retain ABI compatibility
@wjwwood wjwwood mentioned this pull request Apr 7, 2016
wjwwood added a commit that referenced this pull request Apr 7, 2016
* whitespace normalization in wrench_display.* + wrench_visual.*

* generalized WrenchStampedVisual::setMessage()

- interface changed to receive a Wrench& only
- renamed setMessage() to setWrench()

* alternative API: WrenchStampedVisual::setWrench(OgreVector3 force, OgreVector3 torque)

* expose SceneNode::setVisible()

* separate scene nodes for force and torque marker

... to allow hiding of arrows if they become shorter than wide

* retain API compatibility

* retain ABI compatibility
@rhaschke rhaschke deleted the generalize-wrench-visual branch April 8, 2016 07:54
@rhaschke rhaschke mentioned this pull request Apr 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants