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

Fix byte boundary issue #279

Merged
merged 4 commits into from
May 9, 2017
Merged

Fix byte boundary issue #279

merged 4 commits into from
May 9, 2017

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented May 9, 2017

There was an issue related to byte offsets when loading models converted here into Cesium. Before an index accessor might look like:

    "accessor_2": {
      "bufferView": "bufferView_1",
      "byteOffset": 2,
      "byteStride": 0,
      "componentType": 5125,
      "count": 784734,
      "min": [
        0
      ],
      "max": [
        169568
      ],
      "type": "SCALAR"
    }

Which is valid since accessor_2 is still 4-byte aligned when bufferView_1 itself has the correct offset from the central buffer.

However since Cesium creates a typed array from the buffer view, and not the buffer as a whole, accessor_2 would begin on byte 2 of a Uint32Array, causing the indices to be incorrect.

The fix here forces the first accessor of each target to start with a byteOffset of 0 to prevent situations like this.

@mramato can you test this?

@mramato
Copy link
Contributor

mramato commented May 9, 2017

Testing now.

@mramato
Copy link
Contributor

mramato commented May 9, 2017

I can confirm this fixes the problem I was seeing. Thanks!

@mramato
Copy link
Contributor

mramato commented May 9, 2017

This also fixes #277, which was also offset related.

@mramato
Copy link
Contributor

mramato commented May 9, 2017

@lilleyse were you going to do more work here, or is this OK to merge? I know this module is still alpha, so not sure if we're doing unit tests yet or not.

@lilleyse
Copy link
Contributor Author

lilleyse commented May 9, 2017

Yeah I should add a unit test, I'll update soon.

@mramato
Copy link
Contributor

mramato commented May 9, 2017

Awesome, thanks. I have a feeling this will fix quite a few issues I've ran into recently.

@lilleyse
Copy link
Contributor Author

lilleyse commented May 9, 2017

Added test.

@mramato
Copy link
Contributor

mramato commented May 9, 2017

Thanks @lilleyse! The new test fails in master and passes with this fix.

@mramato mramato merged commit 4cf0a8e into master May 9, 2017
@mramato mramato deleted the target-byte-offset branch May 9, 2017 16:57
@mramato mramato mentioned this pull request May 9, 2017
@pjcozzi
Copy link
Contributor

pjcozzi commented May 10, 2017

@lilleyse w.r.t the glTF spec does Cesium need a fix? The intent of the spec is to allow creating "pointers" with typed arrays instead of copying buffers.

@lilleyse
Copy link
Contributor Author

Yeah Cesium would need a fix because nothing was wrong with the previous output. Cesium would need to adjust the byte offset of where it creates the index buffer's typed array.

Does the gltf spec allow an accessor to exist on a non-aligned byte offset? That would be a more serious issue, and require a copy.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 10, 2017

Please submit an issue to Cesium.

Does the gltf spec allow an accessor to exist on a non-aligned byte offset? That would be a more serious issue, and require a copy.

@lexaknyazev?

@lexaknyazev
Copy link

@lilleyse @pjcozzi
Here's relevant spec section.
https://github.com/KhronosGroup/glTF/tree/2.0/specification/2.0#data-alignment

The intent is that no copy is needed.

@lilleyse
Copy link
Contributor Author

Ok that is good to know. Thanks for pointing me there @lexaknyazev.

@lilleyse
Copy link
Contributor Author

Closed CesiumGS/cesium#5309

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.

4 participants