[geometry] Move RenderEngineVtk into its own directory #17169
Conversation
jwnimmer-tri
added
priority: low
release notes: newly deprecated
|
+@zachfang for feature review, please. \CC @svenevs @rpoyner-tri @joemasterjohn FYI |
There was a problem hiding this comment.
Reviewed 30 of 30 files at r1, all commit messages.
Reviewable status: LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @zachfang)
There was a problem hiding this comment.
Just a few naming conventions to follow up.
- In
geometry/render_gltf_client, we putMakeRenderEngineGltfClient()andRenderEngineGltfClientParamsdirectly underdrake::geometrynamespace. - The factory method lives in
factory.{h, cc}rather thanrender_engine_gltf_client_factory.{h, cc}. - The
engineis omitted inrender_gltf_client_params.{.h, cc}.
I was following the Slack discussion but I can modify the files in geometry/render_gltf_client to make them a completely mirrored structure.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @zachfang)
There was a problem hiding this comment.
Most likely I did it wrong here, and gltf is correct. I'll go back and check.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri and @zachfang)
a discussion (no related file):
Working
I think the factory naming isn't quite right.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri, @rpoyner-tri, and @zachfang)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
I think the factory naming isn't quite right.
Done. The factory and namespace stuff was wrong, fixed now. For the params, we need the struct classname and filename to match, so I kept the filename unchanged.
a discussion (no related file):
Working
Double-checking the deprecation shims via Anzu testing.
jwnimmer-tri
force-pushed
the
render-vtk
branch
2 times, most recently
from
9b3f54f to
e1a9976
Compare
There was a problem hiding this comment.
@rpoyner-tri it seems you've already read the whole PR? Would you like to serve as platform review?
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @rpoyner-tri and @zachfang)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Double-checking the deprecation shims via Anzu testing.
Done -(status: do not merge).
There was a problem hiding this comment.
Reviewed 12 of 14 files at r2, 3 of 4 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform) (waiting on @zachfang)
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),zachfang (waiting on @zachfang)
…on#17169) Deprecate the older paths, and non-factory access to VtkRenderEngine.
…on#17169) Deprecate the older paths, and non-factory access to VtkRenderEngine.
Leave only the params class and factory function as non-internal.
This scheme aligns with what we agreed on for
geometry/render_gltf_client.Towards #16502.
This change is