[render_vtk] New option to choose EGL instead of GLX

[jwnimmer-tri]

Towards #21700.

---

This change is 

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri)

---

-- commits line 3 at r1:
Working

Do not merge until after the upgrade has landed and we can rebase.

[jwnimmer-tri]

+@SeanCurtis-TRI for feature review, please.

Reviewed 8 of 23 files at r1, 15 of 15 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[jwnimmer-tri]

@drake-jenkins-bot linux-jammy-clang-bazel-experimental-everything-address-sanitizer please
@drake-jenkins-bot linux-jammy-clang-bazel-experimental-everything-leak-sanitizer please
@drake-jenkins-bot linux-jammy-clang-bazel-experimental-everything-thread-sanitizer please
@drake-jenkins-bot linux-jammy-clang-bazel-experimental-everything-undefined-behavior-sanitizer please
@drake-jenkins-bot linux-jammy-clang-bazel-experimental-everything-valgrind-memcheck please
@drake-jenkins-bot mac-arm-sonoma-clang-bazel-experimental-everything-release please

[SeanCurtis-TRI]

With some cosmetics things.

I'm looking into the MultiMaterialObject.

Reviewed 6 of 23 files at r1, 14 of 15 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers

---

geometry/render_vtk/internal_make_render_window.cc line 36 at r3 (raw file):

#else
  if (use_egl) {
    const bool load_egl_success = gladLoaderLoadEGL(EGL_NO_DISPLAY);

BTW The fact that gladLoaderLoadEGL() can apparently be run multiple times whereas gladLoaderLoadGLX()should be run no more than once per process feels like it is deserving of a comment rather than looking like an oversight.

---

geometry/render_vtk/internal_render_engine_vtk.h line 133 at r3 (raw file):

    ~RenderingPipeline();

    const bool used_egl;

BTW Perhaps using_egl would be more apt?

1. The difference between use and used is small and easy to miss (making things look like typos).
2. The gerund seems to more accurately reflect the state of affairs -- the render window is an egl window?

---

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

DEFINE_bool(show_window, false, "Display render windows locally for debugging");
DEFINE_double(sleep, 0, "Seconds to sleep between renders");
DEFINE_string(backend, "", "RenderEngineVtkParams.backend");

BTW Perhaps a note indicating that the default value for backend needs to remain the empty string?

[jwnimmer-tri]

+@ggould-tri for platform review per schedule, please.

Reviewed 3 of 3 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform)

[SeanCurtis-TRI]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform)

[ggould-tri]

The actual logic here looks fine; a representation quibble below.

Reviewed 5 of 23 files at r1, 12 of 15 files at r2, 2 of 3 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee ggould-tri(platform)

a discussion (no related file):
The meta of my comments below is that for a selection between three things, platform + bool + conditional silent defaulting is a weird representation. It seems to me like it would be cleaner and make for more readable callsites for the three string values to map to three values of an enum rather than a bool.

---

---

geometry/render_vtk/internal_make_render_window.h line 13 at r4 (raw file):

/* Returns a newly-constructed vtkRenderWindow, or else throws when unable.
On Linux, use_egl chooses between EGL (when true) and GLX (when false).
On macOS, use_egl is ignored. */

minor: Is ignoring it the right behaviour? It seems like a programmer error has occurred if you end up in this path, and the caller would like to know that their requested EGL is not available.

Code quote:

is ignored

---

geometry/render_vtk/render_engine_vtk_params.cc line 35 at r4 (raw file):

      static const logging::Warn log_once(
          "RenderEngineVtkParams.backend = 'EGL' is not available");
      return default_result;

minor: "EGL is not available" + setting use_egl to default seems odd; why not return false since you know that true is not valid?

Code quote:

      static const logging::Warn log_once(
          "RenderEngineVtkParams.backend = 'EGL' is not available");
      return default_result;

[jwnimmer-tri]

Reviewed 9 of 9 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform)

a discussion (no related file):

Previously, ggould-tri wrote…

The meta of my comments below is that for a selection between three things, platform + bool + conditional silent defaulting is a weird representation. It seems to me like it would be cleaner and make for more readable callsites for the three string values to map to three values of an enum rather than a bool.

I changed the parsing to use to enum for improved clarity.

Config files should not be platform-dependent, so the "warn instead of error when not compiled in this build" is absolutely what we want.

---

---

geometry/render_vtk/internal_make_render_window.h line 13 at r4 (raw file):

Previously, ggould-tri wrote…

minor: Is ignoring it the right behaviour? It seems like a programmer error has occurred if you end up in this path, and the caller would like to know that their requested EGL is not available.

Not a programmer error. The semantics of the bool was "use_egl_if_available".

In any case, with the enum this function doesn't "ignore" anymore.

---

geometry/render_vtk/render_engine_vtk_params.cc line 35 at r4 (raw file):

Previously, ggould-tri wrote…

minor: "EGL is not available" + setting use_egl to default seems odd; why not return false since you know that true is not valid?

Mooted by the change to enum.

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee ggould-tri(platform)

---

geometry/render_vtk/internal_make_render_window.h line 13 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Not a programmer error. The semantics of the bool was "use_egl_if_available".

In any case, with the enum this function doesn't "ignore" anymore.

(Actually maybe it was a programmer error? In any case, it's moot now.)

[ggould-tri]

Reviewed 9 of 9 files at r5, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),SeanCurtis-TRI(platform)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I changed the parsing to use to enum for improved clarity.

Config files should not be platform-dependent, so the "warn instead of error when not compiled in this build" is absolutely what we want.

Thank you; this is much clearer to me.

---

[SeanCurtis-TRI]

Reviewed 9 of 9 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion

---

geometry/render_vtk/internal_make_render_window.cc line 30 at r5 (raw file):

// with the logic in render_engine_vtk_params.cc.
vtkSmartPointer<vtkRenderWindow> MakeRenderWindow(
    RenderEngineVtkBackend backend) {

This code is subtle and there are some implications that I don't think are obvious:

1. If I'm on linux and I request the Cocoa back end:
   • It automatically falls through to attempt EGL.
   • If initializing EGL doesn't work, we throw.
2. If I'm on mac and request either GLX or EGL, I get the logic error exception (without any indication of what I've requested or why it's a problem.

Have I read that correctly? Is that intentional? If so, I feel the intention should be made explicit. Alternatively, there's an undocumented prerequisite that the backend value be compatible with the platform.

If the correctness of this function is dependent on the mapping from string to enum in ParseRenderEngineVtkBackend() then that dependency should be clear.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion

---

geometry/render_vtk/internal_make_render_window.cc line 30 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This code is subtle and there are some implications that I don't think are obvious:

1. If I'm on linux and I request the Cocoa back end:
   • It automatically falls through to attempt EGL.
   • If initializing EGL doesn't work, we throw.
2. If I'm on mac and request either GLX or EGL, I get the logic error exception (without any indication of what I've requested or why it's a problem.

Have I read that correctly? Is that intentional? If so, I feel the intention should be made explicit. Alternatively, there's an undocumented prerequisite that the backend value be compatible with the platform.

If the correctness of this function is dependent on the mapping from string to enum in ParseRenderEngineVtkBackend() then that dependency should be clear.

Oops. I forgot all of the break; statements. Done.

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),SeanCurtis-TRI(platform)