Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Update Draco gltf models using ratified bitstream #145

Merged

Conversation

FrankGalligan
Copy link
Contributor

@FrankGalligan
Copy link
Contributor Author

@donmccurdy Can you try these files? I might redo them with more compression later. But I just want to make sure they work for you.

@donmccurdy
Copy link
Contributor

These are working with donmccurdy/three.js#6, thanks 👍

@donmccurdy
Copy link
Contributor

In ./javascript, should there be a glTF flavour of the encoder? Or are they the same?

@lexaknyazev
Copy link
Member

@FrankGalligan
Copy link
Contributor Author

@pjcozzi
Copy link
Member

pjcozzi commented Feb 8, 2018

What's left before we can merge this?

@lexaknyazev
Copy link
Member

How does this PR relate to #116?

@donmccurdy
Copy link
Contributor

How does this PR relate to #116?

Those models were created with a pre-spec version of the encoder. I believe this PR can replace #116.

@donmccurdy
Copy link
Contributor

What's left before we can merge this?

I've tested 4-5 of these in the three.js Draco implementation, and they looked good to me.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 8, 2018

Some of of these - and soon all - have also been tested with Cesium. CC CesiumGS/cesium#6191

@bghgary
Copy link
Contributor

bghgary commented Feb 12, 2018

The fallback attributes are not working for me. Looks like bufferView is missing from the accessors.

https://github.com/FrankGalligan/glTF-Sample-Models/blob/be470ea72ecb7adf2b30932349d1b12164af07b7/2.0/Box/glTF-Draco/Box.gltf#L47

https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/schema/accessor.schema.json#L11

Edit: Even if I add the missing bufferView, the accessors fail validation. I think something is wrong with the data itself?

@bghgary
Copy link
Contributor

bghgary commented Feb 12, 2018

I just noticed that extensionsRequired is set for these. If the fallback buffers are intended to work, then this should be removed. If not, then the fallback buffers should be removed.

@donmccurdy
Copy link
Contributor

I believe accessors with missing bufferviews is the expected Draco-required output.

@donmccurdy
Copy link
Contributor

From the spec:

Then the loader must process attributes and indices properties of the primitive. When loading each accessor, you must ignore the bufferView and go to the previously decoded Draco geometry in the primitive to get the data of indices and attributes.

We implemented it this way in three.js:

if ( accessorDef.bufferView === undefined && accessorDef.sparse === undefined ) {

  // Ignore empty accessors, which may be used to declare runtime
  // information about attributes coming from another source (e.g. Draco
  // compression extension).
  return null;

}

/cc @lexaknyazev

@bghgary
Copy link
Contributor

bghgary commented Feb 12, 2018

Yes, sorry for my confusion. I reread the spec and I now understand what it is supposed to look like when the extension is required.

I think we can add a couple of minor things to avoid confusion.

  • Add an short explanation to the extension required part in the spec and indicate that the only difference is that bufferView is missing from the accessor.
  • Add a sample model of Draco with a fallback where Draco is only in extensionsUsed. We can also add this to the asset generator project instead if that's better.

@donmccurdy
Copy link
Contributor

Add an short explanation to the extension required part in the spec and indicate that the only difference is that bufferView is missing from the accessor.

+1 for adding the explanation. Let's do that after the PR is merged.

Add a sample model of Draco with a fallback where Draco is only in extensionsUsed. We can also add this to the asset generator project instead if that's better.

Also +1. I tried to keep this case in mind with three.js implementation, but haven't tested it and would be surprised if it works yet.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 12, 2018

@bghgary @donmccurdy submitted KhronosGroup/glTF#1244.

Please hit merge if this is otherwise ready. 🚀 🚀 🚀

@bghgary
Copy link
Contributor

bghgary commented Feb 12, 2018

I tried all the models with a glTF-Draco folder. All of them are working for me except:

  • CesiumMan
  • Monster
  • RiggedFigure

Does this work for anyone else? It would be good to know if it's the model before I dig in to why.

@donmccurdy
Copy link
Contributor

donmccurdy commented Feb 12, 2018

CesiumMan and Monster are working for me. I haven't tried RiggedFigure. See: mrdoob/three.js#13194

@bghgary
Copy link
Contributor

bghgary commented Feb 12, 2018

I think I figured it out, but would like confirmation. It looks like the unique ids are mapped wrong in RiggedFigure. I haven't checked the rest of them.

Edit: Scratch that. VSCode debugger is reporting the wrong values.

@bghgary
Copy link
Contributor

bghgary commented Feb 12, 2018

The JOINTS_0 attribute is returning floats that are not integers.

0: 2.000000238418579
1: 3.0021979808807373
2: 4.000000476837158
3: 0

Is this expected?

@bghgary
Copy link
Contributor

bghgary commented Feb 12, 2018

I fixed this issue from my end and all of the models are passing for me now. I am okay with merging if the JOINTS_0 issue is expected.

@donmccurdy
Copy link
Contributor

donmccurdy commented Feb 12, 2018

Confirmed Rigged Figure is working in three.js, but as Gary mentions the joint indices are floats not ints, unlike the original file.

@FrankGalligan
Copy link
Contributor Author

I think the spec is fine as it should be int. Even the gltf for the draco version says the data should be int:
https://github.com/FrankGalligan/glTF-Sample-Models/blob/be470ea72ecb7adf2b30932349d1b12164af07b7/2.0/RiggedFigure/glTF-Draco/RiggedFigure.gltf#L2481

I think this is an implementation bug.

@donmccurdy
Copy link
Contributor

Thanks all! Merging this, and we can address the JOINTS_0 implementation bug with a followup. Meanwhile, opened another issue here: #147

@FrankGalligan FrankGalligan deleted the update_draco_gltf_models branch September 11, 2018 19:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants