Update RenderEngineVtk to use RenderMaterial

[SeanCurtis-TRI]

RenderEngineVtk stops using vtkOBJReader and defers to RenderMesh and RenderMaterial to get OBJ mesh data and appearance.

Behavior implications

While the "if foo.png exists for foo.obj, apply it" behavior isn't gone,m it has been supplanted in the case where foo.obj actually has a material library (mtl). So, if models out there had let the mtl file rot when they catered to Drake's former idiosyncracies, this will cause the appearance to shift back towards the actual material specification.

Implementation details

• The render engine is simply responsible for mapping those types to the VTK-specific representation.
• internal_render_engine_vtk_base defines a new vtk polygon source based on the RenderMesh data. This is what replaces vtkOBJReader.
• All non-mesh Shape types now use the RenderMaterial APIs to map from geometry properties to RenderMaterial.
• The unit tests have simplified vis a vis meshes and textures. Now that it simply translates, we can rely on RenderMaterial tests to confirm the protocol logic and we just need to confirm RenderEngineVtk is exercising it and translating it correctly.
• The RenderMesh -> vtkPolyData is capable of providing a more compact representation of the mesh than the vtkOBJReader (it created unique vertex data for each triangle). The gltf server code uses VTK to create gltf files. The reference gltf files used in the tests reflected the old, bloated mesh descriptions. They have been updated to the newer compact sizes.

Some incidental clean up:

• Default diffuse color is properly stored as Rgba now.
• Optimized the normal case of not scaling the texture.
• Added note for clarity about the utility of the scaling factor; it seems to pass in test, but it isn't clear why. This needs further investigation.

Relates #18844

---

This change is 

[SeanCurtis-TRI]

+@zachfang for feature review -- this'll give you a chance to see how materials are getting introduced to the RenderEngine family. I'm not sure what we'll do vis a vis the remote servers. Some of the core functionality isn't bound in pythong.

+(release notes: breaking change)

Reviewable status: LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI and @zachfang)

[jwnimmer-tri]

Reviewed 5 of 12 files at r1.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI and @zachfang)

---

geometry/render/BUILD.bazel line 104 at r1 (raw file):

    testonly = 1,
    srcs = glob([
        "test/meshes/**/*.*",

nit Requiring a period (i.e., *.*) while globbing is somewhat unusual.

Suggestion:

"test/meshes/*",

---

geometry/render/test/meshes/box_no_mtl.obj line 1 at r1 (raw file):

# N.B. Do not include a mtllib in this file!

nit Is it important that this file remain identical to box.obj modulo the mtllib and usemtl lines?

If yes, then we should either enforce that with a linter or else have a BUILD rule generate this file on the fly using sed instead of omitting it.

[zachfang]

First pass done with some minor nits.

Reviewed 12 of 12 files at r1.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

---

geometry/render/BUILD.bazel line 104 at r1 (raw file):

    testonly = 1,
    srcs = glob([
        "test/meshes/**/*.*",

BTW. There are no subfolders under test/meshes, I assume the recursive ** is mainly due to a common glob pattern.

---

geometry/render_vtk/internal_render_engine_vtk.h line 227 at r1 (raw file):

  // Performs the common setup for all shape types.
  void ImplementGeometry(vtkPolyDataAlgorithm* source,
                         const geometry::internal::RenderMaterial& material,

BTW. Based on IWYU, we also need #include "drake/geometry/render/render_material.h. And, if I understand correctly, render_mesh.h is only needed in the .cc file.

---

geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 380 at r1 (raw file):

    material.AddProperty("label", "id", expected_label_);
    if (use_texture) {
      // The simple material's texture is always should always reproduce the

nit. (Somehow I couldn't quote the text.) The simple material's texture is always should always reproduce the

[SeanCurtis-TRI]

Comments addressed. @zachfang let me know when you feel good about this going to platform.

Reviewable status: LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI and @zachfang)

---

geometry/render/BUILD.bazel line 104 at r1 (raw file):

Previously, zachfang wrote…

BTW. There are no subfolders under test/meshes, I assume the recursive ** is mainly due to a common glob pattern.

Yep. Mostly just copypasta (and if a sub-folder were added, it would still work.

---

geometry/render/test/meshes/box_no_mtl.obj line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Is it important that this file remain identical to box.obj modulo the mtllib and usemtl lines?

If yes, then we should either enforce that with a linter or else have a BUILD rule generate this file on the fly using sed instead of omitting it.

Excellent point.

BTW I wasn't 100% clear on the proper bazel spelling. I've added the generated file as data to the file group. I also tried adding it as one of the srcs. Both work but I'm not sure which would be preferred.

---

geometry/render_vtk/internal_render_engine_vtk.h line 227 at r1 (raw file):

Previously, zachfang wrote…

BTW. Based on IWYU, we also need #include "drake/geometry/render/render_material.h. And, if I understand correctly, render_mesh.h is only needed in the .cc file.

Done

(While it's not enforced for non C++ libraries, it's always good. Thanks for catching it for me.)

[zachfang]

except for the box_no_mtl part, which I will leave to Jeremy.

(I plan to spend more time on the relevant merged PRs, but I will do that in the background.)

Reviewed 5 of 6 files at r2.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

[jwnimmer-tri]

Reviewed 3 of 6 files at r2.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

---

geometry/render/BUILD.bazel line 104 at r2 (raw file):

    srcs = ["test/meshes/box.obj"],
    outs = ["test/meshes/box_no_mtl.obj"],
    cmd = "sed '/mtllib\\|usemtl/d' $< > $@",

nit This sed-ism works on Ubuntu, but not macOS. Here's one that works on both:

cmd = "sed '/mtllib/d; /usemtl/d' $< > $@",

---

geometry/render/test/meshes/box_no_mtl.obj line 1 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Excellent point.

BTW I wasn't 100% clear on the proper bazel spelling. I've added the generated file as data to the file group. I also tried adding it as one of the srcs. Both work but I'm not sure which would be preferred.

I think srcs is nominally correct. It's a direct member of the group of files, just like the other objs and pngs and mtls and etc.

[SeanCurtis-TRI]

+@sherm1 for platform review, please.

Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

---

geometry/render/test/meshes/box_no_mtl.obj line 1 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think srcs is nominally correct. It's a direct member of the group of files, just like the other objs and pngs and mtls and etc.

Thanks

[jwnimmer-tri]

Reviewed 1 of 1 files at r3.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

[sherm1]

Platform with some minor comments.

From Zach's comment above "LGTM except for the box_no_mtl part, which I will leave to Jeremy" I think we need an LGTM from Jeremy also +@jwnimmer-tri

After merging, please be sure to update Epic #18844 where needed.

Reviewed 6 of 12 files at r1, 5 of 6 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri)

---

geometry/render_vtk/internal_render_engine_vtk.cc line 558 at r3 (raw file):

    // TODO(SeanCurtis-TRI): It doesn't seem like the scale is used to actually
    // *scale* the image.
    const Vector2d& uv_scale = data.properties.GetPropertyOrDefault(

BTW seems mildly misleading to make this a reference since it will have to refer to a temp copy. Is there a reason to prefer the reference here?

Suggestion:

const Vector2d uv_scale

---

geometry/render_vtk/internal_render_engine_vtk_base.cc line 313 at r3 (raw file):

    output->SetPolys(newPolys);

    return 1;

BTW consider a comment saying what "1" means here

---

geometry/render_vtk/internal_render_engine_vtk_base.cc line 318 at r3 (raw file):

 private:
  // Does vtkPolyDataAlgorithm have copy semantics such that I need to do this
  // explicitly? Or is this just a relic from older C++?

BTW not sure exactly what you're saying here. Did this come from copying an example and you're not sure if it is still needed? Is it a TODO?

---

geometry/render_vtk/test/internal_render_engine_vtk_test.cc line 389 at r3 (raw file):

    } else {
      const Rgba color_n(default_color_.r / 255., default_color_.g / 255.,
                         default_color_.b / 255., default_color_.a / 255.);

nit: per styleguide fp literals should have digits on both sides of the decimal point, 255.0. Plain integers are OK too.

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri)

---

geometry/render_vtk/internal_render_engine_vtk_base.cc line 318 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW not sure exactly what you're saying here. Did this come from copying an example and you're not sure if it is still needed? Is it a TODO?

Simply removed it.

[jwnimmer-tri]

Feature on the changes to the BUILD.bazel files.

Reviewable status: 1 unresolved discussion

[jwnimmer-tri]

@drake-jenkins-bot mac-arm-monterey-clang-bazel-experimental-release please

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions

---

geometry/render_vtk/internal_render_engine_vtk.cc line 545 at r4 (raw file):

          "Texture map '{}' has an unsupported bit depth, casting it to uchar "
          "channels.",
          material.diffuse_map);

nit Build fix for macOS:

Suggestion:

material.diffuse_map.string()

[SeanCurtis-TRI]

D'oh....I'm glad you thought of doing that mac test.

Reviewable status: 1 unresolved discussion

[SeanCurtis-TRI]

@drake-jenkins-bot](https://github.com/drake-jenkins-bot) mac-arm-monterey-clang-bazel-experimental-release please

[jwnimmer-tri]

@drake-jenkins-bot mac-arm-monterey-clang-bazel-experimental-release please

[SeanCurtis-TRI]

Wasn't paying attention. Siiiiiigh.