Skip to content

Optionally provide EXT_meshopt_compression #22935

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wrangelvid
Copy link
Contributor

@wrangelvid wrangelvid commented Apr 25, 2025

For consistency sake, all valid gltf files were converted with gltfpack using the options:
gltfpack -i PATH -o PATH -tr -noq -cf -kn -km -ke

Note that the following files were not valid gltfs:
geometry/render_gltf_client/test/test_color_scene.gltf
geometry/render_gltf_client/test/test_depth_scene.gltf
geometry/render_gltf_client/test/test_label_scene.gltf

This is intended to land after:
meshcat-dev/meshcat#190

Let's see if CI will hate me.


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator

I would say, as we did with #20935, for meshes inside of Drake itself (as opposed to drake_models) we should take a light hand with conversion. Most models in drake should remain simple -- no basisu, no meshopt. It's more important that they be easy to modify for developers, than they be performant for users. Models in this repo are mostly for test cases, not for users or demos. We also need to be sure that some tests have neither compression option in use, so that we know (prove) users' vanilla files will remain working.

I think we just need three changes:

(1) Along the lines of #21090, we should have 1 test file for render engines. Adding a copy of the "fully textured pyramid" for that purpose is probably the right idea, so that all of the "gltf option variant" flavors are in one place.

(2) Along the lines of #21249, changing the scene_graph example sun.gltf to use meshopt compression serves as a nice demo.

(3) Along the lines of #21313, we should have a meshcat_manual_test show an asset (possibly from drake_models) that uses the new feature in a way that eyeball inspection would notice if the feature got broken. Possibly the existing mustard bottle is good enough (assuming it gets changed in drake_models to use the new compression).

Also note that, as with #21193, we will need to update Drake's documentation to explain the newly-available file format.

Copy link
Contributor Author

@wrangelvid wrangelvid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I jumped the gun on the converter too soon.
Thank you for the references! Let me convert this PR back to draft, and take some time to bring in the changes more carefully.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @wrangelvid)

@jwnimmer-tri
Copy link
Collaborator

(It's also OK to close this PR, and re-open once you have a new one ready.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants