Publish ground truth robot position as dynamic TF by peci1 · Pull Request #759 · osrf/subt

peci1 · 2021-01-19T15:17:06Z

Solves #659 and #607. I hope I got all the places where ground truth publishing might be controlled.

Isn't there really a way to just <include> some common file defining all these common definitions for all robots? This approach seems pretty awkward to me...

osrf-jenkins · 2021-01-19T15:29:49Z

Build finished. 15 tests run, 0 skipped, 0 failed.

adlarkin

Thanks for working on this! The changes here look good, but I'm not approving it yet because #745 combines a few of the circuit launch files into one - so, once #745 is merged, I think it would be good to re-base this on master, and then apply the ground truth update to just the newly combined competition.ign file instead of applying it separately to the launch files for cave, tunnel, and urban.

Isn't there really a way to just some common file defining all these common definitions for all robots? This approach seems pretty awkward to me...

This functionality should be available, but I'm not sure that we've taken advantage of it yet (I'd have to double-check). Perhaps we can make the include update in this PR, or in a separate PR after merging this.

subt_ign/launch/cloudsim_sim.ign

peci1 · 2021-01-19T18:56:41Z

Okay, I can rebase on top of #745. Or do you expect it to change a lot until merged? And it'd be great to utilize some include facility, I can also add it to this PR. Is there some documentation/examples for it?

adlarkin · 2021-01-20T01:24:52Z

Okay, I can rebase on top of #745. Or do you expect it to change a lot until merged?

#745 hasn't been reviewed yet, so I'd wait to rebase in case any changes happen there. I don't think much will change, but perhaps reviewers for that PR will catch something that I missed originally. It's probably best to rebase this PR on master once #745 has been merged.

And it'd be great to utilize some include facility, I can also add it to this PR. Is there some documentation/examples for it?

I don't think there is any documentation for this, but I have tested a possible solution with X1_SENSOR_CONFIG_6. Here's what you can do (if any of these steps seem confusing, keep in mind that the files we are dealing with are written in ruby):

Create a file that defines the pose publishing snippet. For example, you can put the following file in subt/subt_ign/launch: https://gist.github.com/adlarkin/eedb69675ead36373f2ddb6ab40fd6e8
In a submitted model's spawner.launch, make the following change (notice how the path is ../../subt_ign/... instead of ../../../subt_ign/... because this is the relative install path when using catkin_make install. spawner.rb is called from the install location when running ign launch, so we need to make sure that the paths are in the context of the workspace's install/ directory). Here's an example with x1_sensor_config_6: https://gist.github.com/adlarkin/dc8ca333b154e8c98473918bbb8f9bed

I'm no ruby expert, so if you have a better idea for the include functionality, feel free to propose your solution here. If we do something similar to what I described above, then we will have to make sure that the ruby file with the pose publishing snippet is also included in the circuit launch files so that the "default" X1...X4 configurations get this change as well (you'll also need to do something similar for ign_migration_scripts/launch/preview.ign).

peci1 · 2021-01-20T02:34:04Z

Thanks for the hint, so I'll wait for #745.

Isn't there a way to achieve the include via SDF? That would look more natural to me...

peci1 · 2021-01-20T03:01:44Z

And what would you think about extracting more pieces to the common include file - like gas sensor or joint state publisher?

adlarkin · 2021-01-20T16:29:21Z

Isn't there a way to achieve the include via SDF? That would look more natural to me...

I don't believe that there is. The <include> tag for SDF is meant for models: http://sdformat.org/tutorials?tut=composition&ver=1.5#including-a-model

So, we will probably need to do something with ruby... but maybe there's a cleaner solution than the one I proposed above.

And what would you think about extracting more pieces to the common include file - like gas sensor or joint state publisher?

I think it would be worth using some sort of "include" mechanism for all common parts of model files. If we want to do something like this, it would probably be best to leave it for a separate PR, since this PR is really meant for fixing the topic for the ground truth data.

So, after some more thought, I'm thinking that the best approach moving forward would be this:

Make this PR fix the issue of publishing ground truth data to /<name>/pose instead of /<name>/pose_static (i.e., merge what you currently have implemented)
Open another PR that creates "include" functionality for the ground truth stuff done here, along with any other code snippets that are common across models

There are a few things to keep in mind:

Combine all circuit launch files #745 combines some of the launch files - after some more thought, I think it's fine to go ahead and update all of the individual circuit launch files with the ground truth fix in this PR. I can always update Combine all circuit launch files #745 later if this PR gets merged first (and if Combine all circuit launch files #745 gets merged before this, then we can update this PR instead... we'll just figure it out when the time comes) Combine all circuit launch files #745 has been merged, so make sure to update this PR with master and then update the new competition.ign file to have the ground truth topic fix.
There are several other newly submitted models that are currently in review/being merged... we probably want to wait to merge this PR until all of the other submitted models are merged. Once the other submitted models are merged, we should then update this PR with the newly merged models, and add this fix to the new models.

What do you think about this approach? Any thoughts/objections?

# Conflicts: # subt_ign/launch/cave_circuit.ign # subt_ign/launch/tunnel_circuit_practice.ign # subt_ign/launch/urban_circuit.ign

peci1 · 2021-02-09T00:21:37Z

I updated this PR with the changes from #745 and also applied the changes to the newly accepted robots, so it's ready to be merged.

osrf-jenkins · 2021-02-09T00:34:13Z

Build finished. 21 tests run, 0 skipped, 1 failed.

peci1 · 2021-02-10T03:38:43Z

I opened up #786 with the larger refactoring. Could you please have a look, @adlarkin ?

adlarkin · 2021-02-18T20:25:08Z

I updated this PR with the changes from #745 and also applied the changes to the newly accepted robots, so it's ready to be merged.

There were a few more robot configurations added in the last week or two that might need these updates as well:

I opened up #786 with the larger refactoring. Could you please have a look, @adlarkin ?

Yeah, I'll take a look (sorry for the delayed response). If #786 is based off of this PR, do we still need this PR? Or can we close it?

nkoenig · 2021-02-19T17:18:59Z

Yeah, I'll take a look (sorry for the delayed response). If #786 is based off of this PR, do we still need this PR? Or can we close it?

I think it's safe to get this PR in, and then @peci1 can update #786.

peci1 · 2021-02-19T17:20:18Z

I think it's safe to get this PR in, and then @peci1 can update #786.

Yes, that's the preferred option. I'll update this PR later today.

# Conflicts: # subt_ign/launch/virtual_stix.ign # subt_ign/launch/virtual_stix_headless.ign

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

peci1 · 2021-02-23T02:05:49Z

Updated. I did a search throughout whole repo and all files except validate_connections.ign show an even number of occurences of ignition-gazebo-pose-publisher-system.

osrf-jenkins · 2021-02-23T02:15:14Z

Build finished. 21 tests run, 0 skipped, 0 failed.

adlarkin

Changes look okay to me. @nkoenig can you or someone else take a quick look at this to make sure we didn't miss any updates anywhere?

…h_tf

peci1 · 2021-03-03T18:59:16Z

I've updated this PR with the latest changes from master (no merge conflicts).

peci1 · 2021-11-01T16:26:32Z

I think this PR would still bring value in the simulator. However, it'd need a major update since it was last synced with master in March... Is the idea of this PR still welcome?

# Conflicts: # submitted_models/ctu_cras_norlab_absolem_sensor_config_1/launch/spawner.rb # submitted_models/ctu_cras_norlab_absolem_sensor_config_2/launch/spawner.rb

peci1 · 2021-11-16T17:17:28Z

I've updated this PR so that it handles all models currently in master.

Publish ground truth robot position as dynamic TF.

1527a8c

This was referenced Jan 19, 2021

Ground truth pose publishers should not publish static TFs #607

Open

Ground Truth TF's slow after update #659

Open

adlarkin mentioned this pull request Jan 19, 2021

Publish ground truth data to /<name>/pose instead of /<name>/pose_static #760

Closed

1 task

adlarkin self-requested a review January 19, 2021 17:15

This was linked to issues Jan 19, 2021

Ground truth pose publishers should not publish static TFs #607

Open

Ground Truth TF's slow after update #659

Open

adlarkin reviewed Jan 19, 2021

View reviewed changes

subt_ign/launch/cloudsim_sim.ign Show resolved Hide resolved

peci1 added 2 commits February 9, 2021 00:59

Merge branch 'master' into dynamic_ground_truth_tf

252d6dc

# Conflicts: # subt_ign/launch/cave_circuit.ign # subt_ign/launch/tunnel_circuit_practice.ign # subt_ign/launch/urban_circuit.ign

Fix a few more places with ground truth publishers.

ef61729

peci1 mentioned this pull request Feb 10, 2021

Refactor Ignition launch files and spawners to share code #786

Draft

adlarkin self-requested a review February 18, 2021 20:25

peci1 added 2 commits February 23, 2021 02:53

Merge branch 'master' into dynamic_ground_truth_tf

5c43ef1

# Conflicts: # subt_ign/launch/virtual_stix.ign # subt_ign/launch/virtual_stix_headless.ign

Fix ground truth publishers on new robots.

fb24a0c

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

adlarkin reviewed Feb 26, 2021

View reviewed changes

peci1 mentioned this pull request Mar 3, 2021

Implement ROS bridge for calling set_rate on cameras and lidars #791

Merged

Merge remote-tracking branch 'origin/master' into dynamic_ground_trut…

a65f934

…h_tf

peci1 added 3 commits November 16, 2021 01:16

Merge branch 'master' into dynamic_ground_truth_tf

6877088

# Conflicts: # submitted_models/ctu_cras_norlab_absolem_sensor_config_1/launch/spawner.rb # submitted_models/ctu_cras_norlab_absolem_sensor_config_2/launch/spawner.rb

Synced with master.

4d60386

Added more models.

adbf938

peci1 added 2 commits November 19, 2021 22:38

Merge branch 'master' into dynamic_ground_truth_tf

5f0791d

Mode models.

406fd47

peci1 mentioned this pull request Dec 26, 2021

Error when using ctu_cras_norlab_spot_sensor_config_1 #1044

Open

Conversation

peci1 commented Jan 19, 2021

Uh oh!

osrf-jenkins commented Jan 19, 2021

Uh oh!

adlarkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peci1 commented Jan 19, 2021

Uh oh!

adlarkin commented Jan 20, 2021

Uh oh!

peci1 commented Jan 20, 2021

Uh oh!

peci1 commented Jan 20, 2021

Uh oh!

adlarkin commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peci1 commented Feb 9, 2021

Uh oh!

osrf-jenkins commented Feb 9, 2021

Uh oh!

peci1 commented Feb 10, 2021

Uh oh!

adlarkin commented Feb 18, 2021

Uh oh!

nkoenig commented Feb 19, 2021

Uh oh!

peci1 commented Feb 19, 2021

Uh oh!

peci1 commented Feb 23, 2021

Uh oh!

osrf-jenkins commented Feb 23, 2021

Uh oh!

adlarkin left a comment

Choose a reason for hiding this comment

Uh oh!

peci1 commented Mar 3, 2021

Uh oh!

peci1 commented Nov 1, 2021

Uh oh!

peci1 commented Nov 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adlarkin commented Jan 20, 2021 •

edited

Loading