Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[render_client] Exports non-compliant glTF files #20151

Open
7 tasks
Tracked by #20152
SeanCurtis-TRI opened this issue Sep 7, 2023 · 2 comments
Open
7 tasks
Tracked by #20152

[render_client] Exports non-compliant glTF files #20151

SeanCurtis-TRI opened this issue Sep 7, 2023 · 2 comments
Assignees
Labels
component: geometry perception How geometry appears in color, depth, and label images (via the RenderEngine API) priority: medium type: bug

Comments

@SeanCurtis-TRI
Copy link
Contributor

What happened?

Drake uses a z-up world. glTF uses a y-up world. We use vtkGLTFExporter to generate a glTF file from the Drake geometry state. However, VTK is agnostic of what kind of world you live and writes out the state verbatim. That means that RenderEngineGltfClient is not creating a compliant glTF file (the scene is rotated 90 degrees).

Short-term fix: this discrepancy should be documented.
Long-term fix: this should be fixed; we should simply output a compliant glTF file.

=====================================================

Notes on implementation:

  • Introduce versioning on RenderEngineGltfClient requests so we know whether we've exported a y-up or z-up glTF.
  • VTK does not offer any affordances for exporting with transformation.
    • As we're about to start building VTK from source, we can write a local patch (with the intent to upstream it as a PR) allowing transformation to be applied on output.
    • The same thing should be done when importing as we need to go from a glTF y-up world to a Drake z-up world.
  • Once VTK does this, we'll need to change several artifacts which have been doing work to account for this:
    • RenderEngineVtk::ImplementGltf will no longer have to apply the rotation.
    • internal_render_engine_gltf_client.cc modify the SetRootPoses to eliminate the R_GF rotation (we don't need the 90 degree rotation anymore).
    • drake_blender/sever.py should rotate the import glTF conditionally based on version.

Version

No response

What operating system are you using?

No response

What installation option are you using?

No response

Relevant log output

No response

@SeanCurtis-TRI SeanCurtis-TRI added type: bug component: geometry perception How geometry appears in color, depth, and label images (via the RenderEngine API) labels Sep 7, 2023
@SeanCurtis-TRI SeanCurtis-TRI self-assigned this Sep 7, 2023
@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Sep 10, 2023

I do think we should add a protocol version (in case we ever change the protocol), but I'd argue that the protocol has not changed here. The only change will be fixing two broken implementations of the protocol.

As a rule of thumb -- if the https://drake.mit.edu/doxygen_cxx/group__render__engine__gltf__client__server__api.html documentation did not change, it's not a new protocol version.

The protocol was always "glTF y-up", because what else could it be? The world coordinate system is not configurable in glTF.

Drake had a bug (this one) by sending broken glTF export, and the blender server had a bug (RobotLocomotion/drake-blender#39) by gratuitously rotating the input so that everything "works".

Counter-proposal:

(1) Add a command line flag to drake-blender for extra rotation when loading glTF. The default would start out as --gltf_extra_rotate=X:90. Immediately tag a drake-blender release with this feature, so there is a server that is compatible with both conventions.

(2) Fix Drake to serialize to glTF correctly. Document in the commit message that server.py should be run using --gltf_extra_rotate=0 as of this commit, but =X:90 prior to it.

(3) When the commit (2) is released in a Drake tag, also tag a new drake-blender release that changes the default value in server.py to =0. In the release notes, explain about how to set the argument based on which version of Drake is sending the file.

The has the downside that users will need to be a little bit careful as they mix and match, but the upside that the protocol license firewall remains intact. Hopefully most users have automation (or at least documentation) for how they pin and run Drake + Drake-Blender, so it will not be a big problem in practice. I think so long as they are aware of this general family of fixing happening, they will notice when their whole world is rotated incorrectly and look to find the setting to fix it.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Sep 10, 2023

As another thought for the future...

As we get better at Drake knowing its own version number, we could imagine amending the http client to send the Drake version number in the "user agent" or similar field. It's not atypical for RPCs to include that kind of information to help with debugging. We could imagine that the server could warn about too-old versions of Drake feeding it.

That's a bit too much of a reach within this bug's timeline, though. It will probably be at least a few more months still before Drake can self-identify which version it is.

@jwnimmer-tri jwnimmer-tri changed the title RenderEngineGltfClient exports non-compliant glTF files [render_client] Exports non-compliant glTF files Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: geometry perception How geometry appears in color, depth, and label images (via the RenderEngine API) priority: medium type: bug
Projects
None yet
Development

No branches or pull requests

2 participants