[workspace] Use VTK built from source #19945
Conversation
jwnimmer-tri
added
status: do not merge
status: do not review
release notes: newly deprecated
jwnimmer-tri
force-pushed
the
vtk-from-source
branch
from
85e1ea5
to
d436a40
Compare
jwnimmer-tri
force-pushed
the
vtk-from-source
branch
from
d436a40
to
006c244
Compare
jwnimmer-tri
force-pushed
the
vtk-from-source
branch
from
1ea1003
to
49e0120
Compare
jwnimmer-tri
force-pushed
the
vtk-from-source
branch
from
6971d01
to
25d9340
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jwnimmer-tri
force-pushed
the
vtk-from-source
branch
2 times, most recently
from
7988f2f
to
df28398
Compare
jwnimmer-tri
force-pushed
the
vtk-from-source
branch
3 times, most recently
from
da7394f
to
97ab847
Compare
jwnimmer-tri
force-pushed
the
vtk-from-source
branch
2 times, most recently
from
c102629
to
a4af80b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee svenevs
a discussion (no related file):
Previously, svenevs (Stephen McDowell) wrote…
Clarification, sorry! Just arbitrarily setting
show_window = truemakes them show up. So if anything I'll recompile a wheel locally with just that change if needed (faster than CI).
Huh. I thought show_window = true was (indirectly) already what the model_visualizer was doing. I wonder where in the plumbing that's getting dropped.
There was a problem hiding this comment.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee svenevs
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Huh. I thought
show_window = truewas (indirectly) already what the model_visualizer was doing. I wonder where in the plumbing that's getting dropped.
Just to double-check, you are passing --show_rgbd_sensor on the command line to model_visualizer?
There was a problem hiding this comment.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee svenevs
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Just to double-check, you are passing
--show_rgbd_sensoron the command line tomodel_visualizer?
The more direct unit test would be to bazel test //geometry/render_vtk:internal_render_engine_vtk_test --test_filter=*HorizonTest* --nocache_test_results after editing that file to change kShowWindow = false to true. For me that (briefly) pops up one native window, each time I run the test.
You could spot-check on master, and then maybe step back in time to VTK 8 or 7 or 6 to see if it ever worked.
There was a problem hiding this comment.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee svenevs
a discussion (no related file):
Just to double-check, you are passing
--show_rgbd_sensoron the command line tomodel_visualizer?
Yeah. The only thing that's dubious is the warning (but it also shows up on linux)
<frozen runpy>:128: RuntimeWarning: 'pydrake.visualization.model_visualizer' found in sys.modules after import of package 'pydrake.visualization', but prior to execution of 'pydrake.visualization.model_visualizer'; this may result in unpredictable behaviour
The more direct unit test would be to
bazel test //geometry/render_vtk:internal_render_engine_vtk_test --test_filter=*HorizonTest* --nocache_test_resultsafter editing that file to changekShowWindow = falsetotrue. For me that (briefly) pops up one native window, each time I run the test.
Sounds good to me. I think it's around 80% compiled now, will continue investigating.
There was a problem hiding this comment.
Reviewed 2 of 9 files at r17.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee svenevs
tools/install/libdrake/BUILD.bazel line 174 at r16 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Here's the list of vtk-related installed files:
bazel run //:install -- --list /tmp/ignored ... share/doc/vtk_internal/Copyright.txt share/doc/vtk_internal/ThirdParty/doubleconversion/vtkdoubleconversion/COPYING share/doc/vtk_internal/ThirdParty/glew/vtkglew/LICENSE.txt share/doc/vtk_internal/ThirdParty/pugixml/vtkpugixml/LICENSE.md share/doc/vtk_internal/ThirdParty/utf8/vtkutf8/LICENSE
done, packaging artifacts and wheels all have the correct licensing information as well.
tools/wheel/image/build-wheel.sh line 66 at r16 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, need to verify licenses are getting installed as expected
done, wheel licenses verified
tools/workspace/default.bzl line 198 at r16 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, confirm these and
setup/changes after inspection of macOS changes.
done, setup changes and soon to be deprecated defaults here look good to me
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee svenevs
a discussion (no related file):
Previously, svenevs (Stephen McDowell) wrote…
Just to double-check, you are passing
--show_rgbd_sensoron the command line tomodel_visualizer?Yeah. The only thing that's dubious is the warning (but it also shows up on linux)
<frozen runpy>:128: RuntimeWarning: 'pydrake.visualization.model_visualizer' found in sys.modules after import of package 'pydrake.visualization', but prior to execution of 'pydrake.visualization.model_visualizer'; this may result in unpredictable behaviourThe more direct unit test would be to
bazel test //geometry/render_vtk:internal_render_engine_vtk_test --test_filter=*HorizonTest* --nocache_test_resultsafter editing that file to changekShowWindow = falsetotrue. For me that (briefly) pops up one native window, each time I run the test.Sounds good to me. I think it's around 80% compiled now, will continue investigating.
I'm not really sure what to make of this situation. I can get the window to pop up quickly on macOS arm64 fine on this branch, but can't see what it is. I tried to figure out where it comes from and put a std::this_thread::sleep_for but couldn't find it.
Instead, I did bazel run //geometry/render_gltf_client:client_demo -- --render_engine vtk --save_dir "$PWD/img" and just did show_window = true inside the UpdateWindow for RenderEngineVtk. On linux it definitely shows the three windows I was expecting. On macOS, nothing ever shows up, even if you add --simulation_time 1000 (very long to account for these windows being slow to display sometimes).
However, when inspecting the img directory, RenderEngineVtk is clearly working correctly on macOS. Additionally, reinstalling glew and robotlocomotion/direct/[email protected] on current master results in the exact same behavior. It's not clear to me this was ever even working.
I could be convinced to just say 
if we're ready to possibly revert after the release if users get broken. I'm also open to doing a deeper dive and try and go back further in time to see if there was ever a point where this was working on drake with older VTKs.
There was a problem hiding this comment.
Reviewed 1 of 111 files at r1, 1 of 38 files at r4, 3 of 5 files at r18, 1 of 1 files at r19, 1 of 2 files at r20, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee svenevs
tools/workspace/vtk_internal/rules.bzl line 529 at r16 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, I'd like to double check this
done, AFAICT this is doing what we want. I personally have a somewhat strong disposition against R"foo(...)foo" (the choice of foo specifically), but in the end this is entirely unimportant.
tools/workspace/vtk_internal/rules.bzl line 560 at r16 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, this should be right for macOS and linux but worth double checking for metal
done, shaders look good, we also scoop up the .mm files for macOS correctly.
tools/workspace/vtk_internal/settings.bzl line 444 at r16 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
VTK has this line of code at
vtk/Rendering/OpenGL2/vtkRenderingOpenGLConfigure.h.in:31:#cmakedefine VTK_USE_XThe
cmake_definesandcmake_undefinesare not setting preprocessor flags, they are setting what happens with#cmakedefineconfigure files.As such, because we run our bazel
configure_file()in strict mode, every#cmakedefinechoice must be given either an affirmative or negative value; leaving it unmentioned entirely is an error (because probably the drake developer forgot about it, and so we should prompt them to make a choice).
done, ok, thank you for explaining. I walked through the code a big, it seems to be working the way we desire 👍
tools/workspace/vtk_internal/settings.bzl line 481 at r16 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, this is concerning, I will likely need to follow up internally
What we're doing here is definitely very far out of supported territory. The next VTK upgrade will be troubling. What I would like to complete this PR is something brief in the README:
- A simple CMake configure command: clone vtk,
mkdir build && cd build,cmake .. -Werror=dev -G Ninja -DVTK_MODULE_ENABLE_VTK_RenderingOpenGL2=YES. Now you cancat Rendering/OpenGL2/vtkRenderingOpenGL2ObjectFactory.cxx. - We're disabling
vtkHyperTreeGridMapper, remove its instances from the generated file. - Make sure to re-add the
#ifdefs in our local tracked file forVTK_USE_XvsVTK_USE_COCOA. - You can
diff/vimdiffthe generated file and the one we are tracking to compare, make sure it's not skipping anything.
There was a problem hiding this comment.
I'm happy to resolve the outstanding macOS vtkRenderWindow thread if we want to drop it. I only have a README.md update request, otherwise I think we're looking good!
Reviewed 1 of 2 files at r20.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee svenevs
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs
tools/workspace/vtk_internal/settings.bzl line 331 at r20 (raw file):
"visibility": ["//visibility:public"], # This module has a lot of code we don't need. We'll opt-out of the # default srcs glob, and instead just specify what Drake needs.
working, this was the only reason the freetype dependency was being kept (because the VTK::IOExport module requires VTK::RenderingFreetype). I just double checked that in docker on focal I can get drake_visualizer to run (well, launch and then fail needing OpenGL 3.x). This confirms that, for example, packages-focal.txt must keep libglew2.1 around.
- As far as I can tell we can get rid of
libfreetype6on jammy (only). I do not have access to a jammy machine to test this in person. We can add this to VTK Dependency Audit #17963 (needs to be rewritten now...). - Something a little odd: on a clean
setup/ubuntu/install_prereqs.shandbazel run //tools:drake_visualizer, all ofvtk_internalgets compiled. This seems incorrect,drake_visualizerhas its own entirely separate VTK (8) build. I looked at tools/BUILD.bazel but couldn't glean why it's always compiling.
jwnimmer-tri
force-pushed
the
vtk-from-source
branch
from
d805569
to
afba112
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r21, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs
tools/workspace/vtk_internal/rules.bzl line 529 at r16 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
done, AFAICT this is doing what we want. I personally have a somewhat strong disposition against
R"foo(...)foo"(the choice offoospecifically), but in the end this is entirely unimportant.
OK This is a fair point, I've switched from foo to something more arcane (less likely to see a false end-of-string-literal token).
tools/workspace/vtk_internal/settings.bzl line 481 at r16 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
What we're doing here is definitely very far out of supported territory. The next VTK upgrade will be troubling. What I would like to complete this PR is something brief in the README:
- A simple CMake configure command: clone vtk,
mkdir build && cd build,cmake .. -Werror=dev -G Ninja -DVTK_MODULE_ENABLE_VTK_RenderingOpenGL2=YES. Now you cancat Rendering/OpenGL2/vtkRenderingOpenGL2ObjectFactory.cxx.- We're disabling
vtkHyperTreeGridMapper, remove its instances from the generated file.- Make sure to re-add the
#ifdefs in our local tracked file forVTK_USE_XvsVTK_USE_COCOA.- You can
diff/vimdiffthe generated file and the one we are tracking to compare, make sure it's not skipping anything.
Done.
- Added more comments to the cxx.
- Added more help to the README.
tools/workspace/vtk_internal/settings.bzl line 331 at r20 (raw file):
... As far as I can tell we can get rid of
libfreetype6on jammy (only). I do not have access to a jammy machine to test this in person ...
We can rely on CI to catch this. I'll remove it now, and we can let the Jammy Unprovisioned catch any problems at load-time.
... all of
vtk_internalgets compiled ...
It's because drake_visualizer depends on all model files by default (so that it can load their meshes), and //examples/pr2:models uses //manipulation/util:stl2obj which uses @vtk_internal. The visualizer does not use @vtk_internal at runtime.
There was a problem hiding this comment.
Reviewed 3 of 5 files at r18, 1 of 1 files at r19, 1 of 2 files at r20, 5 of 5 files at r21, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs
There was a problem hiding this comment.
Reviewed 9 of 9 files at r17, 3 of 5 files at r18, 1 of 1 files at r19, 1 of 2 files at r20, 5 of 5 files at r21, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee svenevs
tools/workspace/vtk_internal/gen/README.md line 27 at r21 (raw file):
vtk/build/Rendering/OpenGL2/vtkRenderingOpenGL2ObjectFactory.cxx ... with the files in Drake's source tree. The Drake customizations that we should retain are summarized about this README, and also highlighted with
nit: clarity
Suggestion:
summarized above in this README
Drake now uses a custom version of VTK that is compiled from source.
Users on macOS (who are not running Drake's unit tests) no longer need
the "robotlocomotion/director" tap and may remove it.
The following Bazel workspace externals are now deprecated:
- double_conversion
- glew
- liblz4
- liblzma
- vtk
The repository rules will continue to work during the deprecation
window, but Drake no longer installs their Ubuntu packages; if you
need the packages, ensure your project setup scripts install them.
For CMake users on Ubuntu 20.04 ("Focal"), Drake's source build and
pre-compiled binaries no longer provide a copy of VTK 9.1. If you still
need Drake's build of VTK 9.1 in your project, you can download it here:
- https://drake-packages.csail.mit.edu/vtk/vtk-9.1.0-3-focal-x86_64.tar.gz
To avoid ABI/ODR headaches, Drake's VTK build uses a custom namespace
and hidden symbols. This is particularly an advantage for wheel builds.
During manual testing on macOS, we found that RenderEngineVtk sometimes
does not obey show_window=true. It is not clear whether this is a new
bug due to the upgrade. Refer to issue 20144 for further discussion.
jwnimmer-tri
force-pushed
the
vtk-from-source
branch
from
afba112
to
2011e1c
Compare
There was a problem hiding this comment.
I think we might be ready!
Once the normal builds pass, I'll re-fire all of the extra builds again.
Reviewed 3 of 3 files at r22, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs
a discussion (no related file):
Previously, svenevs (Stephen McDowell) wrote…
I'm not really sure what to make of this situation. I can get the window to pop up quickly on macOS arm64 fine on this branch, but can't see what it is. I tried to figure out where it comes from and put a
std::this_thread::sleep_forbut couldn't find it.Instead, I did
bazel run //geometry/render_gltf_client:client_demo -- --render_engine vtk --save_dir "$PWD/img"and just didshow_window = trueinside theUpdateWindowforRenderEngineVtk. On linux it definitely shows the three windows I was expecting. On macOS, nothing ever shows up, even if you add--simulation_time 1000(very long to account for these windows being slow to display sometimes).However, when inspecting the
imgdirectory,RenderEngineVtkis clearly working correctly on macOS. Additionally, reinstallingglewandrobotlocomotion/direct/[email protected]on currentmasterresults in the exact same behavior. It's not clear to me this was ever even working.I could be convinced to just say
Done
(1) Filed #20144 for tracking.
(2) Amended comment message to mention it.
(3) Amended Doxygen to mention it.
|
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-bazel-experimental-release please |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r22, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee svenevs
geometry/render_vtk/factory.h line 18 at r22 (raw file):
renderer. @warning On macOS, we've observed that RenderEngineVtk sometimes does not obey
BTW I'd considered that the user would be more likely to encounter this flag in CameraConfig's documentation than here. Thoughts?
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs
geometry/render_vtk/factory.h line 18 at r22 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW I'd considered that the user would be more likely to encounter this flag in
CameraConfig's documentation than here. Thoughts?
That class has nothing to do with VTK, and I don't want to pollute it. If the user wants to find out about our bugs, the most appropriate answer is to search the bug tracker.
There was a problem hiding this comment.
🙂
Reviewed 4 of 5 files at r21, 3 of 3 files at r22, all commit messages.
Reviewable status: 1 unresolved discussion
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Done
(1) Filed #20144 for tracking.
(2) Amended comment message to mention it.
(3) Amended Doxygen to mention it.
done, alrighty then!
tools/workspace/vtk_internal/settings.bzl line 481 at r16 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Done.
- Added more comments to the cxx.
- Added more help to the README.
done, thanks!
tools/workspace/vtk_internal/settings.bzl line 331 at r20 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
... As far as I can tell we can get rid of
libfreetype6on jammy (only). I do not have access to a jammy machine to test this in person ...We can rely on CI to catch this. I'll remove it now, and we can let the Jammy Unprovisioned catch any problems at load-time.
... all of
vtk_internalgets compiled ...It's because drake_visualizer depends on all model files by default (so that it can load their meshes), and
//examples/pr2:modelsuses//manipulation/util:stl2objwhich uses@vtk_internal. The visualizer does not use@vtk_internalat runtime.
done, ok works for me. going to resolve the thread and let CI determine
tools/workspace/vtk_internal/gen/vtkRenderingOpenGL2ObjectFactory.cxx line 34 at r22 (raw file):
#include "vtkOpenGLGlyph3DMapper.h" // Removed the next line for Drake. // #include "vtkOpenGLHyperTreeGridMapper.h"
btw good call on flagging these for future us 🙂
|
@drake-jenkins-bot linux-focal-clang-bazel-experimental-address-sanitizer please @drake-jenkins-bot mac-arm-monterey-clang-bazel-experimental-everything-address-sanitizer please |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r22, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
geometry/render_vtk/factory.h line 18 at r22 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
That class has nothing to do with VTK, and I don't want to pollute it. If the user wants to find out about our bugs, the most appropriate answer is to search the bug tracker.
"nothing to do with" is strong, but I can accept the argument that this is such a small part of the overall that it's not worth the churn.
Closes #16502.
Closes #17962 given this alternative implementation.
Latest test wheels:
Latest packaging artifacts:
Requires:
show_windowcapabilityKnown issues:
This change is