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

Basic CESIUM_primitive_outline support #631

Merged
merged 4 commits into from
Mar 30, 2023

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Mar 15, 2023

The removeUnusedElements family of functions considered the accessors that are used by CESIUM_primitive_outline to be "unused", and removed them.

This PR is a small fix that causes these accessors to be preserved, and the CESIUM_primitive_outline.indices to be updated (no, not the accessor contents, only that accessor index).

This is implemented in a somewhat copy-and-paste-ish style: It uses the same approach that was used for other extensions. But one could consider to generalize the approach there: Having to support each extension manually like this does not scale indefinitely....

This might fix #630

@javagl
Copy link
Contributor Author

javagl commented Mar 15, 2023

Attached here is a ZIP that contains some stuff that may be useful for testing:

  • A simple unit cube with the CESIUM_primitive_outline extension
  • The compressed versions of that file, BEFORE this fix
  • The compressed versions of that file, AFTER this fix
  • One tileset*.json for each of them
  • A Sandcastle that allows selecting and loading them (causing crashes for the BEFORE ones, of course!)

cesiumPrimitiveOutline-2023-04-15.zip

EDIT: If all this looks OK, I'd add a basic test, analogous to https://github.com/javagl/gltf-pipeline/blob/3a5a71d5d2803831a477d35fa87b91f0fc6a8807/specs/lib/removeUnusedElementsSpec.js#L910

@javagl
Copy link
Contributor Author

javagl commented Mar 15, 2023

A random note: The README at https://github.com/CesiumGS/gltf-pipeline/blob/dc213115d2ca12bd5afae00770e48c019ac7d2d5/README.md#command-line-flags says

--draco.quantizePositionBits... No, default 14

but the actual default seems to be 11, as of

quantizePositionBits: 11,

(I can fix that in this PR, if desired)

@javagl
Copy link
Contributor Author

javagl commented Mar 23, 2023

  • Added as basic spec that checks whether the accessor that is used for the indices in CESIUM_primitive_outline is not removed.
    It also checks whether the accessor index is updated, as in d38c47d#diff-5650181c5f5a0ddc9ad5e9b8212725e67ab7fd62e5a1644a8d01e22629edcc2dR1009 . This does not seem to be done in (all) other specs. And one could argue about this: On the one hand, test like this are "overconstrained". The pipeline could do whatever it wanted with these accessors - even reorder them completely. But on the other hand, the check does (similar to all existing ones) already check the resulting list of accessors solely based on their name, so that's probably OK...

  • Updated the README.md with the values for the draco defaults that are actually set in compressDracoMeshes.defaults.
    It also adds the uncompressedFallback option there, which was not mentioned in the README yet (I assume that this was an oversight. If it should not be mentioned, I'll remove it)

@lilleyse
Copy link
Contributor

Thank @javagl. The doc updates look good too.

@lilleyse lilleyse merged commit 8751527 into CesiumGS:main Mar 30, 2023
mramato added a commit to CesiumGS/cesium that referenced this pull request Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Draco compression probably breaks CESIUM_primitive_outline extension
2 participants