Allow usage of lambdas as transport subscription callbacks

[roncapat]

This patch adds one method to gazebo::transport::Node, enabling users to pass a lambda (even a capturing one) as callback to call when a message is received.

This does not modify any previously existing logic. Before this patch, only pure functions or class methods were supported.

This is a prerequisite for ros-controls/gazebo_ros2_control#185 (even if an out-of-tree redefinition of the Node class is possible without Gazebo recompilation, if this PR is not merged in time).

[roncapat]

Hello, may I ask to rerun the CI / give me a first review iteration so that I can address arising problems, if any?

[roncapat]

@traversaro sorry for tagging (I saw you were active recently on this repo), may I ask for a feedback here?

[traversaro]

@traversaro sorry for tagging (I saw you were active recently on this repo), may I ask for a feedback here?

Sure, give me one day, thanks!

[roncapat]

No rush ;)

[traversaro]

No rush ;)

In the meanwhile, can you rebase on the latest gazebo11 branch? In this way we can run the ABI backward compatibilty check, that was not working when you originally opened the PR ( see #3292 ). Thanks!

[roncapat]

Build error on windows, not due to the patch (at least IMHO)

jom 1.1.3 - empower your cores

Error: syntax error in C:\J\workspace\gazebo-ci-pr_any-windows7-amd64\ws\gazebo-classic\build\deps\opende\GIMPACT\CMakeFiles\gazebo_gimpact.dir\build.make line 76
Error: syntax error in C:\J\workspace\gazebo-ci-pr_any-windows7-amd64\ws\gazebo-classic\build\deps\libccd\CMakeFiles\gazebo_ccd.dir\build.make line 72
Error: syntax error in C:\J\workspace\gazebo-ci-pr_any-windows7-amd64\ws\gazebo-classic\build\deps\opende\OPCODE\CMakeFiles\gazebo_opcode.dir\build.make line 76
Error: Target deps\tiny-process-library\CMakeFiles\tiny-process-library.dir\depend doesn't exist.
jom: C:\J\workspace\gazebo-ci-pr_any-windows7-amd64\ws\gazebo-classic\build\CMakeFiles\Makefile2 [deps\opende\OPCODE\CMakeFiles\gazebo_opcode.dir\all] Error 2

[roncapat]

@traversaro I see some test failing (IMHO not related to the patch itself). Is there something I can do to help push forward this patch?

[scpeters]

I think the test errors are unrelated. Sorry for the delay in review on my part.

Can you please add a test that uses a lambda callback? You can copy one of the PubSub integration tests:

https://github.com/gazebosim/gazebo-classic/blob/gazebo11/test/integration/transport.cc#L124

[roncapat]

@scpeters thank you! Added the test with the first test appearing at the code line you linked, linux & mac build passing (windows still fails).

[traversaro]

(windows still fails).

Sorry for the noise, we are working in fixing that: #3330 .

[roncapat]

Now that #3331 got merged, may I ask to please trigger again Windows CI here?

[traversaro]

Now that #3331 got merged, may I ask to please trigger again Windows CI here?

If you rebase the PR on gazebo11 it will trigger on its own.

[roncapat]

Now that #3331 got merged, may I ask to please trigger again Windows CI here?

If you rebase the PR on gazebo11 it will trigger on its own.

Well, did it but I do not see the 3 CI jobs called "C++ CI Workflow with conda-forge dependencies" unfortunately :(

[traversaro]

I am not sure what happened, but apparently now I need to manually start the conda-forge CI for each new commit, I will look into it. Anyhow, now I started them.

[scpeters]

there appears to be a compilation error arising from the test:

• https://build.osrfoundation.org/job/gazebo-ci-pr_any-ubuntu_auto-amd64-gpu-nvidia/1169/console
• https://build.osrfoundation.org/job/gazebo-ci-pr_any-homebrew-amd64/2941/console

test/integration/transport.cc:164:45: error: no matching member function for call to 'Subscribe'
...
test/integration/transport.cc:179:26: error: no matching member function for call to 'Subscribe'
...

[roncapat]

there appears to be a compilation error arising from the test:

• https://build.osrfoundation.org/job/gazebo-ci-pr_any-ubuntu_auto-amd64-gpu-nvidia/1169/console
• https://build.osrfoundation.org/job/gazebo-ci-pr_any-homebrew-amd64/2941/console

test/integration/transport.cc:164:45: error: no matching member function for call to 'Subscribe'
...
test/integration/transport.cc:179:26: error: no matching member function for call to 'Subscribe'
...

Do conda-forge builds also run test or are just for build checks? Because those seems to go well. The failures you mention are from the integration test, I am of course digging into them already and will provide a fix ASAP.

[scpeters]

Do conda-forge builds also run test or are just for build checks? Because those seems to go well.

The test steps are currently commented out in our conda-forge workflow

The failures you mention are from the integration test, I am of course digging into them already and will provide a fix ASAP.

thanks!

[roncapat]

Lambdas can be supported, but with a small caveat.

Capturing lamdas work fine!
Non-capturing lambdas may trigger an "ambiguous call" error of the compiler.
As you can see in the test I added, disambiguation is needed (prepending a "+" to the empty lambda capture list).

See this one...
https://stackoverflow.com/questions/17771671

Note: existing code may have already leveraged some non-capturing lambdas (due to implicit conversion to bare function) and may thus require to introduce this kind of disambiguation.

Another solution could be to remove the Subscribe call accepting raw functions, but this requires at least some small changes here:

gazebo-classic/gazebo/transport/TransportIface.cc

Line 212 in 17e09f5

SubscriberPtr responseSub = node->Subscribe("~/response", &on_response, true);

(since it breaks when I hide the aforementioned "Subscribe" call).

[roncapat]

Compilation passing, some unrelated test still failing. Could you please review/trigger conda-forge builds?

[roncapat]

Conda-forge ok, new tests passed 🥳
Please note I added one test with non-capturing lambda, and one with a capturing lambda.
With non-capturing lambdas, disambiguation (with leading +) needs to be used.

[roncapat]

Thanks for approval! Is there something else I could do, or this can be merged?

[traversaro]

As @scpeters asked for the tests I was waiting for him to see if the added test was ok.

[roncapat]

@scpeters did you have perhaps the chance to give a look here?

[scpeters]

thanks for your patience and for adding a test!