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

Support sparse accessors in Model #10284

Open
ptrgags opened this issue Apr 11, 2022 · 6 comments
Open

Support sparse accessors in Model #10284

ptrgags opened this issue Apr 11, 2022 · 6 comments

Comments

@ptrgags
Copy link
Contributor

ptrgags commented Apr 11, 2022

The old Model class didn't handle sparse accessors from the glTF spec. For better spec compliance, the new ModelExperimental should add this.

Not sure the exact scope of how to implement this, but some thoughts:

  • Certainly GltfLoader (or related loaders) would need more parsing logic for sparse accessors
  • If it's possible, handle this completely at load time so ModelComponents and runtime code such as the GeometryPipelineStage don't need to worry about whether the accessor was sparse or dense.
@j9liu
Copy link
Contributor

j9liu commented Apr 25, 2022

This would also be useful for animation with morph targets, since the weights are packed per keyframe. It gets even more cumbersome to pull the data when CUBICSPLINE interpolation is involved, since there are three values per morph weight per keyframe.

@j9liu j9liu changed the title Support sparse accessors in ModelExperimental Support sparse accessors in Model Aug 23, 2022
@ggetz
Copy link
Contributor

ggetz commented Apr 29, 2024

In #11961, @mramato pointed out a glTF which fails to load, and @javagl Identified lack of support for sparse accessors as the cause for the failure.

house.glb.txt is a draco encoded glTF model that loads fine in all viewers I tried except for CesiumJS. It validates cleanly with no errors.

When loaded into CesiumJS, it reports:

RuntimeError: Failed to load model: ../../SampleData/models/house.glb
Failed to load glTF
One of options.bufferViewId and options.draco must be defined.

... the way CesiumJS is failing here is not very useful and a clear error message or warning about lack of sparse accessors would be the least we could do. But it definitely bugs me that there is part of the glTF standard we seem to not support, especially since tools like gltf-transform's optimize function produces sparse accessors where applicable.

@ggetz @lilleyse What is the level of effort to support them?

@ggetz
Copy link
Contributor

ggetz commented Apr 29, 2024

What is the level of effort to support them?

Correct me if I'm wrong @lilleyse, but supporting sparse accessors is a matter of updating the loader parsing and shouldn't be complex to support.

@lilleyse
Copy link
Contributor

Yeah it seems pretty straightforward. The simplest approach would be to expand the sparse accessor at load time.

@javagl
Copy link
Contributor

javagl commented Dec 21, 2024

An example that I created for #12379 :

MorphTestWithSparseAccessors 2024-12-21.zip

It uses an artificially and intentionally sparse morph target for the POSITION attribute, and should look like this:

Cesium Sparse Example

It currently bails out with this message:

An error occurred while rendering.  Rendering has stopped.
DeveloperError: attribute.value.length must be in the range [1, 4].
Error
    at new DeveloperError (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:9080:13)
    at addAttribute (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:34733:15)
    at new VertexArray (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:34863:7)
    at buildDrawCommandForModel (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:106182:25)
    at ModelDrawCommands.buildModelDrawCommand (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:106154:21)
    at ModelSceneGraph.buildDrawCommands (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:106589:55)
    at buildDrawCommands (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:109707:25)
    at Model.update (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:109530:5)
    at PrimitiveCollection.update (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:147560:21)
    at updateAndRenderPrimitives (https://sandcastle.cesium.com/CesiumUnminified/Cesium.js:241551:23)

@javagl
Copy link
Contributor

javagl commented Dec 21, 2024

Things like

// Cesium only supports morph targets for positions, normals, and tangents
should be tracked in an issue, and at least generate a warning instead of failing silently or even "crashing silently"...

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

No branches or pull requests

5 participants