[render_vtk] Obey extensionsUsed and extensionsRequired #21090
Conversation
jwnimmer-tri
added
the
release notes: fix
|
+@SeanCurtis-TRI for feature review, please. |
SeanCurtis-TRI
left a comment
SeanCurtis-TRI
left a comment
There was a problem hiding this comment.
First pass done. One issue with the patch not quite doing what I believe it's intended to accomplish.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers
a discussion (no related file):
BTW Is updating the documentation on using glTF part of your roadmap?
geometry/render/test/meshes/fully_textured_pyramid.gltf line 71 at r1 (raw file):
{ "sampler":0, "source":0,
BTW Something to consider. What behavior would you want if someone specified a texture with the "used-but-not-required" extension but didn't provide a back up texture (like you've done here)?
Currently, if you remove this line, Drake dies with the message:
ERROR: In vtkGLTFImporter.cxx, line 85
(nullptr): Image not found
I can imagine we'd like something more, but I haven't poked into the code enough to determine if it's practical to provide something more helpful. (See related note below on the vtk patch.)
tools/workspace/vtk_internal/patches/gltf_quiet_image_errors.patch line 20 at r1 (raw file):
BTW This comment might benefit from further elaboration. If a reader only looks at the glTF specification which clearly claims mimetype only allwos "image/jpeg" or "image/png", they might disagree.
The existence of extensions relaxes that documented requirement. A small tweak, perhaps?
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.
I also tweaked the phrase "referenced by the scene" because it's ambiguous (are you referring to the contents of the glTF file, or the parsed result?) The scene can reference them, it just can't require them, as your test shows.
tools/workspace/vtk_internal/patches/gltf_quiet_image_errors.patch line 54 at r1 (raw file):
+ { + vtkErrorWithObjectMacro(this, + "A required image had an unsupported mimeType.");
BTW Your test doesn't trip over this error message. It appears we're tripping over a null image earlier in the code (see the note above).
When given a glTF with an unsupported extensionsRequired, throw an error. (Previously, we would print an error to stderr but bash on regardless.) When given a glTF with an unsupported extensionsUsed (not required), don't report errors about known image formats (e.g., ktx2), rather only about images we need but cannot actually load. The warning about the unsupported texture extension is sufficient on its own.
jwnimmer-tri
left a comment
jwnimmer-tri
left a comment
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Is updating the documentation on using glTF part of your roadmap?
Can you be more specific?
I do have a little checklist under #20935, in case that answers the question.
geometry/render/test/meshes/fully_textured_pyramid.gltf line 71 at r1 (raw file):
Currently, if you remove this line ...
Well, lacking a "source" image entirely for an optional extension is a very broken glTF, so I don't much care what the error looks like if we remove that line. An artist tool is unlikely to produce such a thing.
I think the more relevant question is what happens when the primary texture image is something other than png or jpeg (e.g. image/x-exr). I've explored that with some extra manual testing, and adjusted the code to match now.
tools/workspace/vtk_internal/patches/gltf_quiet_image_errors.patch line 54 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Your test doesn't trip over this error message. It appears we're tripping over a null image earlier in the code (see the note above).
Removed, and replace with more patching to the Importer.cxx now, which is the code that's most relevant.
SeanCurtis-TRI
left a comment
SeanCurtis-TRI
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Can you be more specific?
I do have a little checklist under #20935, in case that answers the question.
I was thinking in terms of the file format documentation. Particularly as regards to what can/cannot be done w.r.t. extensions and the guidance along the lines of providing ktx and parallel png files and the like.
geometry/render/test/meshes/fully_textured_pyramid.gltf line 71 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Currently, if you remove this line ...
Well, lacking a "source" image entirely for an optional extension is a very broken glTF, so I don't much care what the error looks like if we remove that line. An artist tool is unlikely to produce such a thing.
I think the more relevant question is what happens when the primary texture image is something other than png or jpeg (e.g.
image/x-exr). I've explored that with some extra manual testing, and adjusted the code to match now.
Well, it is a valid glTF to do omit it. I don't think you realize how permissive the spec is. source is not required. However, if absent, and no alternative texture source is provided, behavior is undefined.
My question is, how did you make this glTF? I've never used blender to export a glTF file that would create the extension-source and the backup source. So, I'm not sure how you can be so confident that "an artist tool is unlikely to produce such a thing." If this target format is achieved by after-the-fact manipulation, the odds of making this mistake seems to go up in my estimation. Presumably, the solution is to claim that people should use approved tooling to create this effect and never touch the file directly. And, if they do, they deserve unhelpful error messages.
I'm not in love with it, but, meh.
jwnimmer-tri
left a comment
jwnimmer-tri
left a comment
There was a problem hiding this comment.
+@xuchenhan-tri for platform review per schedule (tomorrow), please.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I was thinking in terms of the file format documentation. Particularly as regards to what can/cannot be done w.r.t. extensions and the guidance along the lines of providing ktx and parallel png files and the like.
Yup. That's what I intent for the first checkbox in that issue.
geometry/render/test/meshes/fully_textured_pyramid.gltf line 71 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Well, it is a valid glTF to do omit it. I don't think you realize how permissive the spec is.
sourceis not required. However, if absent, and no alternative texture source is provided, behavior is undefined.My question is, how did you make this glTF? I've never used blender to export a glTF file that would create the extension-source and the backup source. So, I'm not sure how you can be so confident that "an artist tool is unlikely to produce such a thing." If this target format is achieved by after-the-fact manipulation, the odds of making this mistake seems to go up in my estimation. Presumably, the solution is to claim that people should use approved tooling to create this effect and never touch the file directly. And, if they do, they deserve unhelpful error messages.
I'm not in love with it, but, meh.
The next PR (the one I was rubber ducking in slack) will produce a tool to automatically rewrite a glTF file akin what I did in this PR. Here, I did it manually but my objective is that all of the future conversions will be fully automatic.
xuchen-han
left a comment
xuchen-han
left a comment
There was a problem hiding this comment.
Platform
Reviewed 9 of 10 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)
…ion#21090) When given a glTF with an unsupported extensionsRequired, throw an error. (Previously, we would print an error to stderr but bash on regardless.) When given a glTF with an unsupported extensionsUsed (not required), don't report errors about known image formats (e.g., ktx2), rather only about images we need but cannot actually load. The warning about the unsupported texture extension is sufficient on its own.
…ion#21090) When given a glTF with an unsupported extensionsRequired, throw an error. (Previously, we would print an error to stderr but bash on regardless.) When given a glTF with an unsupported extensionsUsed (not required), don't report errors about known image formats (e.g., ktx2), rather only about images we need but cannot actually load. The warning about the unsupported texture extension is sufficient on its own.
3 participants
When given a glTF with an unsupported extensionsRequired, throw an error. (Previously, we would print an error to stderr but bash on regardless.)
When given a glTF with an unsupported extensionsUsed (not required), don't report errors about known image formats (e.g.,
image/ktx2), rather only about images we need but cannot actually load. The warning about the unsupported texture extension is sufficient on its own.Towards #20935.
FYI on what the warning looks like as of this PR:
I created the ktx2 files with the
ktxtool from https://github.com/KhronosGroup/KTX-Software, e.g.,ktx create --encode=basis-lz --format=R8G8B8_UNORM --assign-oetf=linear /home/jwnimmer/jwnimmer-tri/drake/geometry/render/test/meshes/fully_textured_pyramid_omr.{png,ktx2}. Integrating that tool into Drake is future work.This change is