From e9102a8b0494673bac9d2b2f7678a69bbe8ff400 Mon Sep 17 00:00:00 2001 From: Betsy McPhail Date: Thu, 22 Aug 2024 13:21:34 -0400 Subject: [PATCH] [workspace] Upgrade vtk_internal to latest commit (#21799) - Remove patches that have been upstreamed to VTK. - Add more printouts when verbose mode has been enabled. - Prune out some new dependencies that cause trouble / waste. - Adjust image loading to avoid a VTK segfault. Co-authored-by: Jeremy Nimmer --- systems/sensors/image_io_load.cc | 3 + .../vtk_internal/patches/camera_copy.patch | 72 -------- .../vtk_internal/patches/gltf_parser.patch | 155 ------------------ .../patches/gltf_quiet_image_errors.patch | 76 --------- tools/workspace/vtk_internal/repository.bzl | 7 +- tools/workspace/vtk_internal/rules.bzl | 5 + tools/workspace/vtk_internal/settings.bzl | 9 + 7 files changed, 19 insertions(+), 308 deletions(-) delete mode 100644 tools/workspace/vtk_internal/patches/camera_copy.patch delete mode 100644 tools/workspace/vtk_internal/patches/gltf_parser.patch delete mode 100644 tools/workspace/vtk_internal/patches/gltf_quiet_image_errors.patch diff --git a/systems/sensors/image_io_load.cc b/systems/sensors/image_io_load.cc index 16d9bd1e1520..b7901ec8ed44 100644 --- a/systems/sensors/image_io_load.cc +++ b/systems/sensors/image_io_load.cc @@ -126,6 +126,9 @@ ImageIo::LoaderTools ImageIo::MakeLoaderTools( } tools.reader->AddObserver(vtkCommand::ErrorEvent, tools.reader_observer); tools.reader->Update(); + if (tools.errors->size() > 0) { + return tools; + } // Note that we do NOT call ImageLowerLeftOff() here. That means that the // lower left pixel is (0, 0), counting upwards moving up the image. tools.loader->SetInputConnection(tools.reader->GetOutputPort(0)); diff --git a/tools/workspace/vtk_internal/patches/camera_copy.patch b/tools/workspace/vtk_internal/patches/camera_copy.patch deleted file mode 100644 index f076d51e17a7..000000000000 --- a/tools/workspace/vtk_internal/patches/camera_copy.patch +++ /dev/null @@ -1,72 +0,0 @@ -Drake sets the VTK camera's projection matrix directly from Drake's intrinsics. -This matrix was not copied from camera to camera, leading to erroneous results. - ---- Rendering/Core/vtkCamera.cxx -+++ Rendering/Core/vtkCamera.cxx -@@ -1537,6 +1537,16 @@ void vtkCamera::ShallowCopy(vtkCamera* source) - { - this->ModelViewTransform->Register(this); - } -+ -+ if (this->ExplicitProjectionTransformMatrix != nullptr) -+ { -+ this->ExplicitProjectionTransformMatrix->Delete(); -+ } -+ this->ExplicitProjectionTransformMatrix = source->ExplicitProjectionTransformMatrix; -+ if (this->ExplicitProjectionTransformMatrix != nullptr) -+ { -+ this->ExplicitProjectionTransformMatrix->Register(this); -+ } - } - - //------------------------------------------------------------------------------ -@@ -1589,6 +1599,24 @@ void vtkCamera::DeepCopy(vtkCamera* source) - this->UserViewTransform->DeepCopy(source->UserViewTransform); - } - -+ if (source->ExplicitProjectionTransformMatrix == nullptr) -+ { -+ if (this->ExplicitProjectionTransformMatrix != nullptr) -+ { -+ this->ExplicitProjectionTransformMatrix->UnRegister(this); -+ this->ExplicitProjectionTransformMatrix = nullptr; -+ } -+ } -+ else -+ { -+ if (this->ExplicitProjectionTransformMatrix == nullptr) -+ { -+ this->ExplicitProjectionTransformMatrix = -+ static_cast(source->ExplicitProjectionTransformMatrix->NewInstance()); -+ } -+ this->ExplicitProjectionTransformMatrix->DeepCopy(source->ExplicitProjectionTransformMatrix); -+ } -+ - if (source->ViewTransform == nullptr) - { - if (this->ViewTransform != nullptr) -@@ -1757,6 +1785,8 @@ void vtkCamera::PartialCopy(vtkCamera* source) - while (i < 3) - { - this->FocalPoint[i] = source->FocalPoint[i]; -+ this->FocalPointShift[i] = source->FocalPointShift[i]; -+ this->NearPlaneShift[i] = source->NearPlaneShift[i]; - this->Position[i] = source->Position[i]; - this->ViewUp[i] = source->ViewUp[i]; - this->DirectionOfProjection[i] = source->DirectionOfProjection[i]; -@@ -1785,8 +1815,15 @@ void vtkCamera::PartialCopy(vtkCamera* source) - this->FocalDisk = source->FocalDisk; - this->FocalDistance = source->FocalDistance; - this->EyeSeparation = source->EyeSeparation; -+ this->UseExplicitProjectionTransformMatrix = -+ source->UseExplicitProjectionTransformMatrix; - - this->ViewingRaysMTime = source->ViewingRaysMTime; -+ -+ this->ExplicitAspectRatio = source->ExplicitAspectRatio; -+ this->UseExplicitAspectRatio = source->UseExplicitAspectRatio; -+ this->NearPlaneScale = source->NearPlaneScale; -+ this->ShiftScaleThreshold = source->ShiftScaleThreshold; - } - - //------------------------------------------------------------------------------ diff --git a/tools/workspace/vtk_internal/patches/gltf_parser.patch b/tools/workspace/vtk_internal/patches/gltf_parser.patch deleted file mode 100644 index 0184560de19b..000000000000 --- a/tools/workspace/vtk_internal/patches/gltf_parser.patch +++ /dev/null @@ -1,155 +0,0 @@ -This corrects a few defects in VTK's glTF parsing. - -The first defects share a common origin: VTK assumes that if *any* texture is -present, it must be the base color texture. If this texture is missing, then -other textures confuse VTK, leading to spurious warnings (multiple sets of UVs -defined) and otherwise cause the textures to get ignored. - -We address this with two independent changes: - - - Change the logic that the glTF parser uses to determine if a mesh primitive - has been defined with multiple sets of uv coordinates. - - Make the inclusion of metallic, normal, emmissive, and ambient occlusion - maps *no longer* dependent on the presence of a base color map. - -Furthermore, those warnings use the vtkWarningWithObjectMacro in a way which -forces the messages to be written directly to the console. We change the -calls so that it gets registered with an objects event listener so that Drake -can choose to capture the warnings and present as it sees fit. - -These changes *should* be upstreamed to VTK. - ---- IO/Geometry/vtkGLTFDocumentLoader.h -+++ IO/Geometry/vtkGLTFDocumentLoader.h -@@ -265,7 +265,7 @@ public: - struct TextureInfo - { - int Index = -1; -- int TexCoord; -+ int TexCoord = -1; - }; - - /** - ---- IO/Import/vtkGLTFImporter.cxx -+++ IO/Import/vtkGLTFImporter.cxx -@@ -43,6 +43,7 @@ - - #include - #include -+#include - #include - - VTK_ABI_NAMESPACE_BEGIN -@@ -80,7 +81,8 @@ vtkSmartPointer GLTFCameraToVTKCamera(const vtkGLTFDocumentLoader::Ca - //------------------------------------------------------------------------------ - vtkSmartPointer CreateVTKTextureFromGLTFTexture( - std::shared_ptr model, int textureIndex, -- std::map>& existingTextures) -+ std::map>& existingTextures, -+ vtkGLTFImporter* parent) - { - - if (existingTextures.count(textureIndex)) -@@ -160,14 +162,23 @@ vtkSmartPointer CreateVTKTextureFromGLTFTexture( - } - - //------------------------------------------------------------------------------ --bool MaterialHasMultipleUVs(const vtkGLTFDocumentLoader::Material& material) -+bool MaterialHasMultipleUVs(const vtkGLTFDocumentLoader::Material& material, -+ vtkGLTFImporter* parent) - { -- int firstUV = material.PbrMetallicRoughness.BaseColorTexture.TexCoord; -- return (material.EmissiveTexture.Index >= 0 && material.EmissiveTexture.TexCoord != firstUV) || -- (material.NormalTexture.Index >= 0 && material.NormalTexture.TexCoord != firstUV) || -- (material.OcclusionTexture.Index >= 0 && material.OcclusionTexture.TexCoord != firstUV) || -- (material.PbrMetallicRoughness.MetallicRoughnessTexture.Index >= 0 && -- material.PbrMetallicRoughness.MetallicRoughnessTexture.TexCoord != firstUV); -+ std::set uv_sets; -+ for (const vtkGLTFDocumentLoader::TextureInfo& info : -+ { material.PbrMetallicRoughness.BaseColorTexture, material.EmissiveTexture, -+ material.NormalTexture, material.OcclusionTexture, -+ material.PbrMetallicRoughness.MetallicRoughnessTexture }) -+ { -+ if (info.Index >= 0) uv_sets.insert(info.TexCoord); -+ } -+ if (uv_sets.contains(-1)) { -+ vtkErrorWithObjectMacro(parent, -+ "A material defined a texture index without " -+ "defining a texture coordinate set."); -+ } -+ return uv_sets.size() > 1; - } - - //------------------------------------------------------------------------------ -@@ -188,15 +199,16 @@ bool PrimitiveNeedsTangents(const std::shared_ptr - //------------------------------------------------------------------------------ - void ApplyGLTFMaterialToVTKActor(std::shared_ptr model, - vtkGLTFDocumentLoader::Primitive& primitive, vtkSmartPointer actor, -- std::map>& existingTextures) -+ std::map>& existingTextures, -+ vtkGLTFImporter* parent) - { - vtkGLTFDocumentLoader::Material& material = model->Materials[primitive.Material]; - -- bool hasMultipleUVs = MaterialHasMultipleUVs(material); -+ bool hasMultipleUVs = MaterialHasMultipleUVs(material, parent); - if (hasMultipleUVs) - { - vtkWarningWithObjectMacro( -- nullptr, "Using multiple texture coordinates for the same model is not supported."); -+ parent, "Using multiple texture coordinates for the same model is not supported."); - } - auto property = actor->GetProperty(); - property->SetInterpolationToPBR(); -@@ -233,9 +245,10 @@ void ApplyGLTFMaterialToVTKActor(std::shared_ptr m - { - // set albedo texture - vtkSmartPointer baseColorTex; -- baseColorTex = CreateVTKTextureFromGLTFTexture(model, texIndex, existingTextures); -+ baseColorTex = CreateVTKTextureFromGLTFTexture(model, texIndex, existingTextures, parent); - baseColorTex->UseSRGBColorSpaceOn(); - property->SetBaseColorTexture(baseColorTex); -+ } - - // merge ambient occlusion and metallic/roughness, then set material texture - int pbrTexIndex = material.PbrMetallicRoughness.MetallicRoughnessTexture.Index; -@@ -291,7 +304,7 @@ void ApplyGLTFMaterialToVTKActor(std::shared_ptr m - { - pbrImage.ImageData->GetPointData()->GetScalars()->FillComponent(0, 255); - } -- auto materialTex = CreateVTKTextureFromGLTFTexture(model, pbrTexIndex, existingTextures); -+ auto materialTex = CreateVTKTextureFromGLTFTexture(model, pbrTexIndex, existingTextures, parent); - property->SetORMTexture(materialTex); - } - } -@@ -300,7 +313,7 @@ void ApplyGLTFMaterialToVTKActor(std::shared_ptr m - int emissiveTexIndex = material.EmissiveTexture.Index; - if (emissiveTexIndex >= 0 && emissiveTexIndex < static_cast(model->Textures.size())) - { -- auto emissiveTex = CreateVTKTextureFromGLTFTexture(model, emissiveTexIndex, existingTextures); -+ auto emissiveTex = CreateVTKTextureFromGLTFTexture(model, emissiveTexIndex, existingTextures, parent); - emissiveTex->UseSRGBColorSpaceOn(); - property->SetEmissiveTexture(emissiveTex); - } -@@ -309,10 +322,9 @@ void ApplyGLTFMaterialToVTKActor(std::shared_ptr m - if (normalMapIndex >= 0 && normalMapIndex < static_cast(model->Textures.size())) - { - actor->GetProperty()->SetNormalScale(material.NormalTextureScale); -- auto normalTex = CreateVTKTextureFromGLTFTexture(model, normalMapIndex, existingTextures); -+ auto normalTex = CreateVTKTextureFromGLTFTexture(model, normalMapIndex, existingTextures, parent); - property->SetNormalTexture(normalTex); - } -- } - - // extension KHR_materials_unlit - actor->GetProperty()->SetLighting(!material.Unlit); -@@ -496,7 +508,7 @@ void vtkGLTFImporter::ImportActors(vtkRenderer* renderer) - if (primitive.Material >= 0 && - primitive.Material < static_cast(model->Materials.size())) - { -- ApplyGLTFMaterialToVTKActor(model, primitive, actor, this->Textures); -+ ApplyGLTFMaterialToVTKActor(model, primitive, actor, this->Textures, this); - } - renderer->AddActor(actor); - diff --git a/tools/workspace/vtk_internal/patches/gltf_quiet_image_errors.patch b/tools/workspace/vtk_internal/patches/gltf_quiet_image_errors.patch deleted file mode 100644 index ec8a0b1fed7a..000000000000 --- a/tools/workspace/vtk_internal/patches/gltf_quiet_image_errors.patch +++ /dev/null @@ -1,76 +0,0 @@ -[vtk] GLTF loader doesn't error on extentionsUsed images - -When images are only referenced by extensionsUsed that we're skipping -over anyway, we should not report false positive error messages. The -warning about the skipped extensionsUsed is sufficient on its own. - -Also be sure to forward warnings and errors from the loader that's -nested inside the importer back to the importer proper. - -This patch should be upstreamed. - ---- IO/Geometry/vtkGLTFDocumentLoader.cxx -+++ IO/Geometry/vtkGLTFDocumentLoader.cxx -@@ -819,6 +819,15 @@ bool vtkGLTFDocumentLoader::LoadImageData() - { - reader = vtkSmartPointer::New(); - } -+ else -+ { -+ // We must not report any error here; extensions allow other image types. -+ // It is perfectly valid to declare other, extension-supported image types, -+ // so long as they are never required by the scene. Therefore, the possible -+ // error is deferred til later. -+ image.ImageData = nullptr; -+ continue; -+ } - - // If image is defined via bufferview index - if (image.BufferView >= 0 && - ---- IO/Geometry/vtkGLTFDocumentLoaderInternals.cxx -+++ IO/Geometry/vtkGLTFDocumentLoaderInternals.cxx -@@ -616,13 +616,7 @@ bool vtkGLTFDocumentLoaderInternals::LoadImage( - { - image.MimeType.clear(); - } -- else if (image.MimeType != "image/jpeg" && image.MimeType != "image/png") -- { -- vtkErrorWithObjectMacro(this->Self, -- "Invalid image.mimeType value. Must be either image/jpeg or image/png for image " -- << image.Name); -- return false; -- } -+ - // Read the bufferView index value, if it exists. - image.BufferView = -1; - if (vtkGLTFUtils::GetIntValue(root, "bufferView", image.BufferView)) - ---- IO/Import/vtkGLTFImporter.cxx -+++ IO/Import/vtkGLTFImporter.cxx -@@ -81,11 +81,15 @@ vtkSmartPointer CreateVTKTextureFromGLTFTexture( - const vtkGLTFDocumentLoader::Texture& glTFTex = model->Textures[textureIndex]; - if (glTFTex.Source < 0 || glTFTex.Source >= static_cast(model->Images.size())) - { -- vtkErrorWithObjectMacro(nullptr, "Image not found"); -+ vtkErrorWithObjectMacro(parent, "Image not found"); - return nullptr; - } - - const vtkGLTFDocumentLoader::Image& image = model->Images[glTFTex.Source]; -+ if (image.ImageData == nullptr) { -+ vtkErrorWithObjectMacro(parent, "Image mimeType not supported"); -+ return nullptr; -+ } - - vtkNew texture; - texture->SetColorModeToDirectScalars(); -@@ -381,6 +381,8 @@ int vtkGLTFImporter::ImportBegin() - vtkNew forwarder; - forwarder->SetTarget(this); - this->Loader->AddObserver(vtkCommand::ProgressEvent, forwarder); -+ this->Loader->AddObserver(vtkCommand::WarningEvent, forwarder); -+ this->Loader->AddObserver(vtkCommand::ErrorEvent, forwarder); - - // Check extension - std::vector glbBuffer; diff --git a/tools/workspace/vtk_internal/repository.bzl b/tools/workspace/vtk_internal/repository.bzl index 63c7bb7fe3d9..df98f11639fd 100644 --- a/tools/workspace/vtk_internal/repository.bzl +++ b/tools/workspace/vtk_internal/repository.bzl @@ -172,16 +172,13 @@ def vtk_internal_repository( # TODO(jwnimmer-tri) Once there's a tagged release with support for # VTK_ABI_NAMESPACE, we should switch to an official version number # here. That probably means waiting for the VTK 10 release. - commit = "4746dce06afabf2dc678d3f2eefd87cb41a90b05", - sha256 = "eb2295327504fe6ecb79b7fcf692e9ae969b2a89a2c51bfc3c0bbfa885f5a092", # noqa + commit = "f196cc1e9042b5637928c2ea87570b773b8745c8", + sha256 = "e8147b0073224190c6ad3d0816576d1795bf233e8d7dcd4457890f8217d08872", # noqa build_file = ":package.BUILD.bazel", patches = [ - ":patches/camera_copy.patch", ":patches/common_core_nobacktrace.patch", ":patches/common_core_version.patch", ":patches/fix_illumination_bugs.patch", - ":patches/gltf_parser.patch", - ":patches/gltf_quiet_image_errors.patch", ":patches/io_image_formats.patch", ":patches/mr11117.patch", ":patches/nerf_pegtl.patch", diff --git a/tools/workspace/vtk_internal/rules.bzl b/tools/workspace/vtk_internal/rules.bzl index 25d5643b19d6..21b8535767bd 100644 --- a/tools/workspace/vtk_internal/rules.bzl +++ b/tools/workspace/vtk_internal/rules.bzl @@ -287,6 +287,11 @@ def modules_closure(module_names, *, max_depth = 10): continue if dep_name in ignore: continue + if _VERBOSE: + print("Modules closure: {} uses {}".format( + name, + dep_name, + )) next_worklist.append(dep_name) worklist = next_worklist if worklist: diff --git a/tools/workspace/vtk_internal/settings.bzl b/tools/workspace/vtk_internal/settings.bzl index 379856653a48..33d043b0093e 100644 --- a/tools/workspace/vtk_internal/settings.bzl +++ b/tools/workspace/vtk_internal/settings.bzl @@ -309,6 +309,9 @@ MODULE_SETTINGS = { "VTK::ImagingSources", ], }, + "VTK::FiltersReduction": { + # VTK uses this module internally but we don't need to customize it. + }, "VTK::FiltersSources": { "visibility": ["//visibility:public"], "srcs_glob_exclude": [ @@ -426,6 +429,9 @@ MODULE_SETTINGS = { "IO/Legacy/vtkDataReader.cxx", "IO/Legacy/vtkUnstructuredGridReader.cxx", ], + "module_deps_ignore": [ + "VTK::IOCellGrid", + ], }, "VTK::ImagingCore": { "visibility": ["//visibility:public"], @@ -471,6 +477,8 @@ MODULE_SETTINGS = { "srcs_glob_exclude": [ # This is configure-time setup code, not library code. "**/vtkProbe*", + # This file uses codegen'd embedded vtp files. We don't need it. + "**/*vtkOpenGLAvatar*", # Avoid building unnecessary VTK::RenderingHyperTreeGrid. "**/*HyperTreeGrid*", # Exclude all renderers by default; we'll incorporate the necessary @@ -517,6 +525,7 @@ MODULE_SETTINGS = { ], }), "module_deps_ignore": [ + "VTK::IOXML", "VTK::RenderingHyperTreeGrid", ], },