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

Use GetAttribute to enable Draco quantization when possible #9904

Merged
merged 3 commits into from
Oct 29, 2021

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Oct 29, 2021

This is a temporary workaround for #9847. See google/draco#742 (comment) for more info. Since SkipAttributeTransform breaks lookups with GetAttributeByUniqueId, this PR uses the alternative approach with GetAttributeId and GetAttribute to preserve quantization when possible.

The downside is that this might not work when there are multiple sets of an attribute, i.e. TEXCOORD_0 and TEXCOORD_1. Because of this, we only call SkipAttributeTransform with the new GetAttribute approach for "single" attributes. Otherwise, we continue without quantization and use the GetAttributeByUniqueId call as before.

I'm working on adding back some of the specs that were removed previously, but the core change is all in decodeDraco.js.

@ebogo1 ebogo1 requested a review from lilleyse October 29, 2021 21:40
@cesium-concierge
Copy link

Thanks for the pull request @ebogo1!

  • ❕ There was an error checking the CLA! If this is your first contribution, please send in a Contributor License Agreement.
    • Maintainers, this was the error I ran into while attempting to process the CLA check. Please resolve it to continue CLA checking.
    • You'll need to manually check for a submitted CLA
    Error: The service is currently unavailable.
    
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

Comment on lines +282 to +289
var attributesToSkip = ["POSITION", "NORMAL"];
var compressedAttributes = parameters.compressedAttributes;
if (!defined(compressedAttributes["COLOR_1"])) {
attributesToSkip.push("COLOR");
}
if (!defined(compressedAttributes["TEXCOORD_1"])) {
attributesToSkip.push("TEX_COORD");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glTF does allow for TEXCOORD_2, TEXCOORD_3, etc so some of this glTF attribute name -> draco name mapping could be organized in a more general way. Though to be fair, TEXCOORD_2 and higher are only used in ModelExperimental and if we're expecting a bug fix in draco soon then maybe we don't have to worry about it here.

Copy link
Contributor Author

@ebogo1 ebogo1 Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that if there were more than one TEXCOORD, the numbering would go in order, meaning that at least TEXCOORD_1 would exist - in which case, we would not call SkipAttributeTransform(). Is it possible to just have one TEXCOORD and call it TEXCOORD_2? I'm not sure I'm following your comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's true, never mind then. From the glTF spec:

All indices for indexed attribute semantics MUST start with 0 and be consecutive positive integers

@lilleyse lilleyse merged commit 09981f0 into main Oct 29, 2021
@lilleyse lilleyse deleted the draco-workaround branch October 29, 2021 22:56
@lilleyse
Copy link
Contributor

Thanks for the quick turnaround @ebogo1. For 99% of cases we can use quantization again!

@lilleyse
Copy link
Contributor

Forgot to mention, I tested all the 3D models and 3D Tiles related sandcastles and ran the unit tests locally.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Oct 29, 2021

Thanks @lilleyse. I tested microcosm-draco.gltf.zip (converted from https://github.com/CesiumGS/cesium/tree/main/Specs/Data/Models/GltfLoader/Microcosm/glTF with gltf-pipeline), which has multiple TEXCOORD attributes and all seems to work as expected.

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.

3 participants