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

Use RViz node clock in tools #519

Merged
merged 1 commit into from Mar 17, 2020
Merged

Use RViz node clock in tools #519

merged 1 commit into from Mar 17, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Mar 16, 2020

Precisely what the title says. Without this patch, a default clock (i.e. a clock hooked up to system time) is always used, not abiding to use_sim_time.

CI up to rviz_default_plugins:

  • Linux Build Status

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic requested a review from wjwwood Mar 16, 2020
Copy link
Member

@wjwwood wjwwood left a comment

lgtm, it would be good to mention this in the release notes and to try it out locally if you haven't done that already.

@hidmic
Copy link
Contributor Author

hidmic commented Mar 17, 2020

it would be good to mention this in the release notes

Which release notes? Foxy's at index.ros.org? I plan to backport this to Eloquent (at least) too, should we also update those release notes?

try it out locally if you haven't done that already.

Timestamps look like they should locally 👍

@wjwwood
Copy link
Member

wjwwood commented Mar 17, 2020

Yes the foxy release notes.

This changes ABI, so I wouldn’t push it to eloquent myself, why does it need that?

@hidmic
Copy link
Contributor Author

hidmic commented Mar 17, 2020

This changes ABI

Argh, yes, I changed a member field.

I'm using Eloquent elsewhere and having RViz tools working with Gazebo is practical at least. I can build from source though.

@clalancette
Copy link
Contributor

clalancette commented Mar 17, 2020

Argh, yes, I changed a member field.

As an ABI-compatible alternative, maybe you can not store clock_ and instead just always do raw_node_->get_clock() where you need it?

@hidmic
Copy link
Contributor Author

hidmic commented Mar 17, 2020

Good idea, I'll open another PR after merging this one that does that. Unless @wjwwood you'd rather have the non-ABI breaking change everywhere (despite extra indirections).

@wjwwood
Copy link
Member

wjwwood commented Mar 17, 2020

Good idea, I'll open another PR after merging this one that does that. Unless @wjwwood you'd rather have the non-ABI breaking change everywhere (despite extra indirections).

I don't have a strong preference. I guess having to fetch it each time might be a performance issue, but I don't know that for sure.

@hidmic
Copy link
Contributor Author

hidmic commented Mar 17, 2020

Ok, two patches then.

hidmic added a commit to ros2/ros2_documentation that referenced this pull request Mar 17, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Mar 17, 2020

Release notes PR: ros2/ros2_documentation#568

@hidmic
Copy link
Contributor Author

hidmic commented Mar 17, 2020

I'll go ahead and merge, as CI on other platforms won't reveal much (for this particular case).

@hidmic hidmic merged commit ff8fcf9 into ros2 Mar 17, 2020
@hidmic hidmic deleted the hidmic/use-node-clock-in-tools branch Mar 17, 2020
kscottz added a commit to ros2/ros2_documentation that referenced this pull request Mar 18, 2020
* Update Foxy Release Notes after ros2/rviz#519

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* RST wants double quotes, not single.

Co-authored-by: Katherine Scott <katherineAScott@gmail.com>
ferranm99 added a commit to ferranm99/ferran-ros that referenced this pull request May 20, 2022
* Update Foxy Release Notes after ros2/rviz#519

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* RST wants double quotes, not single.

Co-authored-by: Katherine Scott <katherineAScott@gmail.com>
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.

None yet

3 participants