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

[Eloquent backport]: Allow the MapDisplay "Update Topic" to be changed. (#517) #518

Merged
merged 1 commit into from
Mar 12, 2020

Conversation

clalancette
Copy link
Contributor

The major reason for this is so that the "Update Topic"
(and more importantly the QoS profile) is saved when clicking
"Save Config" in RViz2. The more minor reason is that a user
might want to use a different topic for this. We still
auto-populate this field with <topic_name>_updates by default,
but the user can now override it.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

The major reason for this is so that the "Update Topic"
(and more importantly the QoS profile) is saved when clicking
"Save Config" in RViz2.  The more minor reason is that a user
*might* want to use a different topic for this.  We still
auto-populate this field with <topic_name>_updates by default,
but the user can now override it.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I think this is fine.

One thing I was concerned with is if this change will cause any problems with existing .rviz files, since I think this change will affect the stored config structure. I didn't test for this case though.

@clalancette
Copy link
Contributor Author

One thing I was concerned with is if this change will cause any problems with existing .rviz files, since I think this change will affect the stored config structure. I didn't test for this case though.

I don't believe it will.

In the existing Eloquent RViz, because this is marked as read-only, clicking "Save" in RViz won't actually save it. So for most "normal" uses of RViz, this is a straightforward addition.

If someone went ahead and and hand-typed the data into a saved RViz file, then in current Eloquent RViz it will be properly loaded in (I have a customer using that workaround right now). By changing this to not be read-only, that behavior is preserved, but it additionally allows us to save by clicking "Save" (and has the side-effect of allowing the user to set the "Update Topic" to an arbitrary value).

@wjwwood wjwwood merged commit bab3134 into eloquent Mar 12, 2020
@wjwwood wjwwood deleted the eloquent-backport-517 branch March 12, 2020 21:06
@wjwwood
Copy link
Member

wjwwood commented Mar 12, 2020

@mjcarroll if you want me to do a release into Eloquent let me know.

@jacobperron jacobperron added this to Needs Release in Eloquent Patch Release 2 Apr 17, 2020
@jacobperron jacobperron moved this from Needs Release to Released in Eloquent Patch Release 2 Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants