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

Feature/refactor covariance displays #11

Merged
merged 38 commits into from
Mar 2, 2016

Conversation

Ellon
Copy link
Collaborator

@Ellon Ellon commented Feb 22, 2016

In this PR I re-factored the plugin's Pose and Odometry displays to enable back all the functionalities from the original rviz displays, in a way that the covariance looks like more or less like an addon to the original displays. This PR is based on the code from #9.

The computation of the covariances are the same as before, but just wrapped into one file (covariance_visual.cpp). The covariance visualization is also the same (ellipse for position and a cone (?) for orientation). I think this way it's easy to improve the drawing of the covariances later.

One thing I did was to add some functionality from Pose to Odometry, like possibility to visualize the odometry as arrow or axes, and a finer control of the shapes being draw (axis length and radius, and arrows shaft/head length/radius). The selection tool for the pose with covariance is also working.

Nils Berg and others added 28 commits October 14, 2015 15:26
Now it's possibile to fix the visualization on the pose
This class will exhibit the covariance properly given a covariance
6d covariance matrix (float64 covariance[36]) from a message
While here, add visibility parameter to constructor
…iances

Since we use the scale to set the covariance shapes, we had to add geometry
properties for the arrow like it is done with rviz::PoseDisplay (head and shaft
properties separated), instead of just a scale value like it's done in
rviz::OdometryDisplay.
Also, while here fix includes of rviz files.

Rationale behind this commit:

The arrow class like it's defined points to -Z, so we were rotating it to make
it point to +X. The problem is that if we make the arrow's node parent of
covariance's nodes we'll have the covariance rotated as well. The work-around is
to keep a deque of scene nodes, and make them parents of respective arrow and
covariance nodes. With this arrow's node can be rotated without affecting the
rotation of covariance's nodes.
@Ellon Ellon force-pushed the feature/refactor_covariance_displays branch from db9d630 to 42faf6a Compare February 23, 2016 00:04
This allow us to draw the position covariance wrt in the frame defined by
message's frame_id, while the orientation covariance is still drawn in the
"child frame".

The frame node should be positioned externally (by the displays
holding covariance visual objects).
@georgebrindeiro
Copy link
Contributor

Looks good to me

The scale should be twice the standard deviation. This way the scale 1.0 will
represent one standard deviation.

While here, fix the parameter name (shape -> scale).
@protobits
Copy link

Can this be merged and then any subsequent improvements be proposed in new PRs? Integrating this would allow this package to be used, which is not possible ATM due to the wrong results of the ellipse orientation.

@Ellon
Copy link
Collaborator Author

Ellon commented Feb 29, 2016

@v01d It doesn't look this thread is being watched by any maintainers... Maybe @thomas-moulard or @mherrb could take a look at it?

@mherrb
Copy link
Member

mherrb commented Feb 29, 2016

If Thomas is still interested, let him validate this. If he doesn't answer in a few days, I will take care of it (or just give you access to the repository)

@thomas-moulard
Copy link
Contributor

Hi all, thank for waiting for my input!

@mherrb could please go ahead and merge this? I don't have any write-access to the repo anymore and I don't use ROS for my work currently so I don't have the bandwidth to maintain this plugin.

IMHO the best thing to do is to send this as a PR to rviz and see if we can get this included by default. Last time we checked the maintainers seemed to be willing to accept it.

@thomas-moulard
Copy link
Contributor

BTW if rviz integration is not an option but someone is interested to maintain this code. Please check with @mherrb . On my side, I don't have any objection to have anyone take over this repo. Thanks!

@Ellon
Copy link
Collaborator Author

Ellon commented Mar 1, 2016

Hello Thomas, thanks for the input. I plan to send this as a PR to rviz later, after I have the angular covariances being displayed properly.

@wjwwood
Copy link

wjwwood commented Mar 2, 2016

Hi guys. I found my way over here from ros-visualization/rviz#974. My slight preference is to get this working here first and then propose merging this into rviz after people agree that it is working correctly. The idea being that people who are already using this as an external plugin can benefit from the improvements too. I'd probably only consider this plugin for merging into Jade or Kinetic. If merging went well for those versions then I'd consider Indigo, but it won't be a quick turn around.

Thanks!

@Ellon
Copy link
Collaborator Author

Ellon commented Mar 2, 2016

(Sorry I miss-clicked to close the PR)

OK @wjwwood I'll update ros-visualization/rviz#974 when this PR is merged. I'm not sure if there's anyone using this plugin apart from myself and maybe @v01d . I think the agreement will probably come from two of us. :)

@Ellon Ellon closed this Mar 2, 2016
@Ellon Ellon reopened this Mar 2, 2016
mherrb added a commit that referenced this pull request Mar 2, 2016
@mherrb mherrb merged commit 126ba69 into laas:indigo Mar 2, 2016
@mherrb
Copy link
Member

mherrb commented Mar 2, 2016

Hi Ellon, I've merge the request and added you as team member for this repository. You can now manage it directly.

@Ellon
Copy link
Collaborator Author

Ellon commented Mar 2, 2016

Ok @mherrb , Thanks!

@Ellon Ellon deleted the feature/refactor_covariance_displays branch March 2, 2016 11:36
@NikolausDemmel
Copy link

Looks like I'm late to the party... @Ellon: We are using this plugin. Thanks for the update, i will give it a try shortly.

@NikolausDemmel
Copy link

So it works quite nicely! Thank you very much @Ellon for cleaning this up, and fixing the reference frames as well as the eigen vector order.

I have some comments, but they are of a more general nature, so I will add them over at #10.

@NikolausDemmel
Copy link

P.S.: +1 for getting this into rviz proper, even indigo, and discutinuing development here.

@wjwwood
Copy link

wjwwood commented Mar 2, 2016

Now that the issues are sorted out here, I'll take a look at the rviz pr. I just wanted to separate the concern of fixing the thing and getting it put into rviz.

Also, since I take on some level of maintenance when things are merged into rviz, so I tend to review it carefully before rolling it in.

@Ellon
Copy link
Collaborator Author

Ellon commented Mar 2, 2016

Er... in fact there are still some issues I just opened, mostly related with 2D poses and rotational covariance. Please check #10 and the currently open issues.

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.

7 participants