Skip to content

Add a default for touch plugin namespace#3513

Open
luca-della-vedova wants to merge 4 commits intogazebosim:mainfrom
luca-della-vedova:luca/default_touch_namespace
Open

Add a default for touch plugin namespace#3513
luca-della-vedova wants to merge 4 commits intogazebosim:mainfrom
luca-della-vedova:luca/default_touch_namespace

Conversation

@luca-della-vedova
Copy link
Copy Markdown
Member

🎉 New feature

Adds a default value to the namespace for TouchPlugin.

Summary

Previously, namespace was a mandatory parameter. This makes it a bit harder to have multiple instances of the same model without having to edit the SDF.
Specifically, if a user had a model with a TouchPlugin in its SDF, two instances of the same model would inevitably publish to the same topic (unless they did some SDF experimental params overrides).
With this PR, if users skip the namespace parameter, the plugin's namespace will default to model_name/touch making it easier to have models publish to unique topics.

Test it

Run the touch_plugin demo then list the topics:

Current main:

$ gz sim -r touch_plugin.sdf
$ gz topic -l
[...]
/white_touches_only_green/touched

Then change the demo world to remove the namespace parameter

$ gz sim -r touch_plugin.sdf
$ gz topic -l
[...]
/white_box/touch/touched

Checklist

  • Signed all commits for DCO
  • Added a screen capture or video to the PR description that demonstrates the feature
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • Updated Bazel files (if adding new files). Created an issue otherwise.
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers
  • Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Generated-by: Remove this if GenAI was not used.

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by and Generated-by messages.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@github-project-automation github-project-automation Bot moved this from Inbox to In review in Core development May 6, 2026
@azeey
Copy link
Copy Markdown
Contributor

azeey commented May 6, 2026

Github won't let me comment on the line, so I'm posting here instead. On src/systems/touch_plugin/TouchPlugin.cc:268 consider adding a gzmsg stating that the default namesapce was used.

@iche033
Copy link
Copy Markdown
Contributor

iche033 commented May 6, 2026

I think there's an edge case that if the model has 2 touch plugins, e.g. here, the topic names will collide?

@luca-della-vedova
Copy link
Copy Markdown
Member Author

I think there's an edge case that if the model has 2 touch plugins, e.g. here, the topic names will collide?

Indeed! I don't have a very good solution, my suggestion would be to use nested models so the automatic namespacing computation still works. For example in that case we could have a sfp_port_0 and a sfp_port_1 nested models.
Alternatives on the top of my head could be:

  • Add a new optional parameter, something like namespace_suffix that gets postponed to the namespace, so in your case users could specify sfp_port_0 and sfp_port_1 for their namespace_suffix parameter, which with autonamespacing would resolve to {model_name}/{namespace_suffix}/touch/[touched/enable].
  • Add an auto-computation of index and increase indices as new plugins get instantiated, so the first plugin has namespace, the following namespace_1, etc. A way to do this could be to add a component to the model that contains a std::vector<TouchPluginInfo>, where TouchPluginInfo contains a string with the namespace and a boolean for whether it was auto-computed or not, we could have some logic that, for new autocomputed namespaces, looks up the highest index and increases it.
  • [Breaking] Updating the message published by this plugin, instead of just a boolean (that is always set to true anyway) something more descriptive (ideally a string for the target and one for the collision, but if we don't want a new message just one for the collision).

I don't like option 2 since it's a quite obscure and brittle behavior, option 3 would be nice but breaking since it's a message type change, that leaves us with a new parameter (option 1) or just accepting that users will have to do the nested model workaround (option 0)

@iche033
Copy link
Copy Markdown
Contributor

iche033 commented May 7, 2026

hmm ok I think I prefer option 0 compared to adding another new param. We could just add to the doc in the header to warn that if more than one touch plugins are used, the auto-generated namespaces will conflict, and propose to them to either use the nested model workaround you described or just fill the <namespace> param.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Copy Markdown
Member Author

Added both the logging and the note in the header in f44c42f

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Copy Markdown
Member Author

luca-della-vedova commented May 7, 2026

With the recommendation of using the nested model approach in 43d2d5f I changed the topic name to be the scoped name instead of the model name.
Using the model name was cleaner but if we suggest using nested models we will have the same topic conflict issue that if we instantiate a model multiple times, the nested models will have the same name.
Sadly scoped names are not very pretty but at least they are unique. For the example linked by @iche033, when I updated it locally to instantiate the plugin inside two nested models called sfp_port_0 and sfp_port_1 (patch):

Before:

/sfp_port_0/touch/touched
/sfp_port_1/touch/touched

After:

/model/task_board/model/nic_card_mount_0/model/sfp_port_0/touch/touched
/model/task_board/model/nic_card_mount_0/model/sfp_port_1/touch/touched

Note how the first one would fail if we were to instantiate multiple of the nic_card_mount model.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

4 participants