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 problem switching from Uint16 to Uint32 indices for outlining. #8820

Merged
merged 6 commits into from
May 13, 2020

Conversation

kring
Copy link
Member

@kring kring commented May 3, 2020

Adding edge outlines usually requires adding new vertices, and when we're using a UNSIGNED_SHORT index buffer and the new vertices push us over 65536 vertices, we need to upgrade to an UNSIGNED_INT index buffer. I had previously added this capability, and wrote tests for it, but didn't have any real-world data to test it with at the time. Unfortunately, that code doesn't actually work, and will corrupt rendering of the model when it's triggered. It was all part of my plan to serve as a good illustration for everyone of the dangers of only writing tests for some code and not actually trying it with real data. 😆

The problem is that Model also squirrels away the bufferView's componentType in ModelLoadResources.indexBuffersToCreate and then uses that to create the buffer instead of the accessor's componentType in the glTF. So I'm also helping to illustrate the dangers of denormalization. 👍

@cesium-concierge
Copy link

Thanks for the pull request @kring!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@mramato
Copy link
Contributor

mramato commented May 13, 2020

What the plan for this PR? @lilleyse who would be the best to review this?

@emackey you're the king of glTF edge cases, any thoughts?

@emackey
Copy link
Contributor

emackey commented May 13, 2020

and the new vertices push us over 65536 vertices

Sorry, weird edge case here, but the max is 65535 vertices, index numbers 0 through 65534. The last 16-bit value, 65535 (or -1), is reserved as a "primitive restart" value in some APIs and in glTF. So when you reach that value, you must switch to 32-bit indexing.

@kring
Copy link
Member Author

kring commented May 13, 2020

Oh interesting, I didn't realize that, @emackey! I think it doesn't actually matter here because WebGL doesn't do primitive restart, and this is Cesium's in-memory representation, not a proper glTF. But it can't hurt to change to avoid possible subtle bugs later.

@kring
Copy link
Member Author

kring commented May 13, 2020

Done!

@mramato
Copy link
Contributor

mramato commented May 13, 2020

Like I said, king of glTF edge cases 👑 😄

@emackey emackey merged commit 389770e into master May 13, 2020
@emackey emackey deleted the outline-32 branch May 13, 2020 01:32
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