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 properties for the covariance of the initial pose tool #1255

Conversation

ipa-fez
Copy link
Contributor

@ipa-fez ipa-fez commented Jun 11, 2018

Currently the initial pose tool has a hardcoded covariance matrix. Sometimes it is useful to send an initial pose with a low covariance. This PR adds properties to the initial pose tool that allows specifying the main elements of the covariance matrix.

@rhaschke
Copy link
Contributor

rhaschke commented Mar 8, 2019

I recently took over maintenance of rviz and I'm trying to catch up with open PRs.
I like the idea behind this PR, but I suggest to modify the interface to enter std deviations instead of variance values. Requiring the user to enter sigma^2 seems odd. What do you think?
Could you also target this PR to Melodic, please?

@ipa-fez ipa-fez force-pushed the feature/initial_pose_covariance_properties branch from f42d212 to 4a3aa40 Compare March 8, 2019 12:26
@ipa-fez ipa-fez changed the base branch from kinetic-devel to melodic-devel March 8, 2019 12:26
@ipa-fez ipa-fez force-pushed the feature/initial_pose_covariance_properties branch from 4a3aa40 to dedcaee Compare March 8, 2019 12:30
@ipa-fez
Copy link
Contributor Author

ipa-fez commented Mar 8, 2019

Thanks for looking at this PR! I've changed it to std deviations and targeted it against melodic-devel.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks a lot. Looks good.

@rhaschke
Copy link
Contributor

rhaschke commented Mar 8, 2019

Unfortunately, adding new member variables breaks ABI compatibility:

Size of InitialPoseTool has been increased from 320 bytes to 344 bytes.
Effects:
1) An object of this class can be allocated by the applications and old size will be hardcoded at the compile time. Call of any exported constructor will break the memory of neighboring objects on the stack or heap.
2) The memory layout and size of subclasses will be changed.

Personally, I don't think that this is critical, because probably nobody instantiates the InitialPoseTool manually. The common usage should be instantiating this class via the plugin mechanism, which ensures that the proper, new version is allocated.
However, @wjwwood could you please have a short look at this ABI breakage?

@wjwwood
Copy link
Member

wjwwood commented Mar 8, 2019

My hard and fast rule is "no ABI breaks in existing releases", but as you pointed out @rhaschke, in this case it might be protected by the plugin interface. So I think it's fine to make a judgement call here to allow it if you want.

@rhaschke rhaschke merged commit f25e85b into ros-visualization:melodic-devel Mar 8, 2019
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.

3 participants