Add functionalities for optical tactile plugin

[mcres]

This PR adds the following to gazebosim#229:

• Service for enabling/disabling the plugin
• Topic for the normal forces computed
• Test
• Visualize the contacts from the contact sensor

[mcres]

@mabelzhang

[mabelzhang]

The initial test is good, but more things need to be tested. The plugin has many lines, and the current test coverage is probably only a small portion of the plugin.

The resolution parameters are not tested - with a known camera image size and a known resolution in the test SDF, you should be able to EXPECT the number of points published based on the depth sensor data.

The visualization is not tested. If it's set to true, there should be some markers, and if it's set to false, there should be no markers.

Etc. Maybe not everything can be tested, but the essential functionalities such as the above should be tested. There may be more.

[mcres]

Addressed the feedback in 2db11bc.

Re the tests, I'm thinking about writing several sdf files in /test/worlds/optical_tactile_plugin with different parameter configuration to test each one of those. Is this a good idea?

[mabelzhang]

Thanks for addressing the changes. Having multiple SDF files is fine.

[mabelzhang]

@mcres Friendly ping. Do you have time to add the new tests? Ideally we'd like to get this feature in before Ignition Dome code freeze Sep 23, for it to be included in the Dome release. Otherwise it will go in after the release.

[mcres]

Sorry, I didn't have much time these last few weeks. I'll try to add them before the 23rd.

[mcres]

I've taken a look at the testing, but there are a couple of issues:

• We've already talked about this, but it would be easier to test the markers published into the simulation through the marker/list topic. Besides, there are some plugin parameters like <visualize_sensor>, <visualize_forces> or <visualize_contacts> that can't be tested otherwise (at least I don't know how).
  I was wondering if accessing this topic in the tests will be available anytime soon? Also, tests would be more readable and easier to maintain. Given that the plugin will be in a later Dome minor release (sorry about that), maybe it would be worth waiting for that feature?
• Independently of that, I think there's something wrong with the normals published in /<namespace>/normal_forces: given that the user can specify the resolution in <visualization_resolution>, the dimensions of the depth camera message and the normal forces message are not the same. This is not the case currently, and I'm thinking about adding a couple of counters for width/height, filling the buffer and somehow "cutting it back" before publishing it. What do you think about that?

[mabelzhang]

maybe it would be worth waiting for that feature?

The marker feature is not on the core development roadmap, so I don't know if any of us has time for it any time soon. I would not wait for it.

something wrong with the normals published

Testing more is always good, but I'm concerned with how long this would take. It's worth it to add the test if you think you realistically have time to do this soon, say in the next 2-3 weeks. Otherwise I would prefer to just merge everything associated with this plugin, create an issue on ign-gazebo about making this test and labeling it help-wanted, so that we can get the plugin's functionality into ign-gazebo. My main concern is that the longer we wait, the less we will remember, and the more upstream will change and have more conflicts in code, and at some point we'll forget about it and it never goes in.

Regarding the tests, you were thinking about adding several more SDF files above. Is that something you're still considering / doable with the existing features?

[mcres]

Regarding the tests, you were thinking about adding several more SDF files above. Is that something you're still considering / doable with the existing features?

The reason of adding several SDF files was to be able to test a specific set of the plugin parameters. Regarding the tests, it changes nothing.

I'm afraid I won't be able to address this in the near future. Independently of that, I think that following your suggestion about merging and creating the corresponding issue is a good idea, especially given that everything will be in the ign-gazebo repository.

[mabelzhang]

Sounds good. Let's just try to merge existing code.

Here's a plan:

1. For the upstream PR on ign-gazebo, we were thinking of addressing the trivial warning on Mac on the upstream PR, and Addisu and I were going to talk about the one remaining comment on there. Those are the two remaining bits for that PR before merging into ign-gazebo4.

   Being aware of your time constraints, I was going to open a PR in your fork here to address those issues. If you could look out for an upcoming PR in your fork and approve it, that'll be great!

2. The tutorial upstream has already been merged into the feature branch optical_tactile_plugin. We'll open another PR to get it into ign-gazebo4.

3. As for this PR here, could you close this PR and open another targeting optical_tactile_plugin branch in the upstream ign-gazebo repo? That way we can just merge it into the feature branch and push directly to it to fix any remaining bits. We'll then open a PR from the feature branch into ign-gazebo4 containing the changes here and the tutorial.

To summarize, action items are bullet 1. approve an upcoming PR, and bullet 3. open a PR upstream and create the issue for what we discussed above.
How does that sound?

[mcres]

I was going to open a PR in your fork here to address those issues. If you could look out for an upcoming PR in your fork and approve it, that'll be great!

Hmm I don't follow this one. Doesn't it make sense to address those issues in the optical_tactile_plugin of the ign-gazebo repo?

[mabelzhang]

Well, we need CI to be clear before we merge into ign-gazebo4, since the upstream PR is going there. Right now we cannot merge that because Mac CI is yellow. So I was going to open a PR here to your fork to fix that last bit, as well as Addisu's remaining comment. Once everything in the upstream PR is addressed, we can merge it into ign-gazebo4, and do the remainder of the work in ign-gazebo:optical_tactile_plugin where we have write access.

[mcres]

OK! I'll look out for the PR then.