scene.proto: add shadow_caster_material_name

[scpeters]

🎉 New feature

Motivated by gazebosim/gazebo-classic#3048.

Summary

We added a custom SDFormat parameter in gazebosim/gazebo-classic#3048 to allow specifying the name of a custom shadow caster material for a scene. Given that gazebo classic communicates scene information from the server to the client over a transport interface, it would have been convenient to add this field to the scene.proto message, but this would have broken ABI. As such, we added a separate ignition-transport service for this parameter.

Before ABI freezes for fortress, I thought I would submit this change, in case we decide to use the parameter in ignition in the future.

cc @iche033 @WilliamLewww

Test it

It's not used yet; this is just in anticipation of a future use of this parameter.

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

[codecov]

Codecov Report

Merging #179 (3dabcd7) into main (adf2644) will not change coverage.
The diff coverage is n/a.

❗ Current head 3dabcd7 differs from pull request most recent head e418f2f. Consider uploading reports for the commit e418f2f to get more accurate results

@@           Coverage Diff           @@
##             main     #179   +/-   ##
=======================================
  Coverage   84.56%   84.56%           
=======================================
  Files           7        7           
  Lines         855      855           
=======================================
  Hits          723      723           
  Misses        132      132           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adf2644...e418f2f. Read the comment docs.

[iche033]

one suggestion is to use a script for shadow caster:

  message Script
  {
    repeated string uri = 1;
    string name = 2;
  }

...

  Script shadow_caster_script = 13;

This allows users to specify a path to the material script in case it's not in the resource paths already.

[scpeters]

one suggestion is to use a script for shadow caster:

  message Script
  {
    repeated string uri = 1;
    string name = 2;
  }

...

  Script shadow_caster_script = 13;

This allows users to specify a path to the material script in case it's not in the resource paths already.

@WilliamLewww does that make sense to you?

[WilliamLewww]

one suggestion is to use a script for shadow caster:

  message Script
  {
    repeated string uri = 1;
    string name = 2;
  }

...

  Script shadow_caster_script = 13;

This allows users to specify a path to the material script in case it's not in the resource paths already.

@WilliamLewww does that make sense to you?

I think in this case, the string name would refer to the shadow caster material name and the string url would refer to the file that contains the material (just in case the material file is not in a default path) right?

[scpeters]

one suggestion is to use a script for shadow caster:

  message Script
  {
    repeated string uri = 1;
    string name = 2;
  }

...

  Script shadow_caster_script = 13;

This allows users to specify a path to the material script in case it's not in the resource paths already.

@WilliamLewww does that make sense to you?

I think in this case, the string name would refer to the shadow caster material name and the string url would refer to the file that contains the material (just in case the material file is not in a default path) right?

yeah, perhaps we should rename name to material_name?

[scpeters]

one suggestion is to use a script for shadow caster:

  message Script
  {
    repeated string uri = 1;
    string name = 2;
  }

...

  Script shadow_caster_script = 13;

This allows users to specify a path to the material script in case it's not in the resource paths already.

@WilliamLewww does that make sense to you?

I think in this case, the string name would refer to the shadow caster material name and the string url would refer to the file that contains the material (just in case the material file is not in a default path) right?

yeah, perhaps we should rename name to material_name?

added the Script sub-message in 6addaa4

[chapulina]

I don't know if it would be a good idea to use it here, but I'll just point out that we already have a Material::Script message here:

https://github.com/ignitionrobotics/ign-msgs/blob/2f2d86edbcbeec3e66a553608787f5792858c76e/proto/ignition/msgs/material.proto#L40-L44

[chapulina]

Suggested change

	/// \brief Name of shadow caster material.
	/// \brief Shadow caster material script.

[scpeters]

applied in scpeters@e418f2f

[scpeters]

I don't know if it would be a good idea to use it here, but I'll just point out that we already have a Material::Script message here:

https://github.com/ignitionrobotics/ign-msgs/blob/2f2d86edbcbeec3e66a553608787f5792858c76e/proto/ignition/msgs/material.proto#L40-L44

I've switched to using the existing Material.Script message in scpeters@e418f2f

[chapulina]

The suggested changes have been made, I think that everyone is onboard with the current implementation. I'll merge this but speak up if you don't agree, we still have until the end of code freeze to revert it if needed.