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

Add KTX2 support and remove KTX/CRN #9513

Merged
merged 93 commits into from
Jun 28, 2021
Merged

Add KTX2 support and remove KTX/CRN #9513

merged 93 commits into from
Jun 28, 2021

Conversation

ebogo1
Copy link
Contributor

@ebogo1 ebogo1 commented Apr 27, 2021

This PR is very similar to #9040. There are a few big to-do items before this is ready, but eslint passes and the KTX2 balloon in the "3D Models" sandcastle works so I figured it was a good time to get the code out there again.

Changes include:

  • Remove loadCRN and loadKTX and replace with loadKTX2
    • rework loadKTXSpec -> loadKTX2Spec
  • Add Binomial transcoder (basis_transcoder.wasm)
    • Binomial module is used to parse KTX2 files in parseKTX2.js, which is called from the transcodeKTX2 worker's transcode() function
  • Updated Image-Based Lighting, 3D Models, and Materials Sandcastles with .ktx2 data

To-do:

  • Investigate alternative transcoder module - https://github.com/KhronosGroup/Universal-Texture-Transcoders (this will be a follow-up PR)
  • UASTC/ASTC/BC7 support
  • Update sample data glTFs
  • Support for compressed .ktx2 with embedded mipmap (needs ModelSpec test)
  • Update IBL instructions in Sandcastle
  • Add check for SFLOAT vs UNORM uncompressed formats
  • Finish specs for loadKTX2/KTX2Transcoder
  • Fix Image-Based Lighting
  • Fix Materials sandcastles (ktx2 pixel format not supported)
  • Remove unused transcoder modules
  • Update specs using .ktx2 sample data
  • Test Sandcastles on different OS
    • iOS
    • Android (Chrome)
    • Linux (FF)
    • Windows
    • macOS
  • Update LICENSE.md
    • Binomial Transcoder module
    • KTX-Parse
  • Update CHANGES.md

cc @lilleyse - not sure if you wanted to take a closer look at this yet, especially with some of the to-do items left. thought it wouldn't hurt to open the PR to track progress.

@cesium-concierge
Copy link

Thanks for the pull request @ebogo1!

  • ✔️ Signed CLA found.
  • ❔ Changes to third party files were made.
    • Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.
  • ❔ 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.
  • Works (or fails gracefully) in IE11.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Apr 27, 2021

As a note, I have been using Khronos's ktx2ktx2 command-line tool for converting our KTX models to KTX2 - see https://github.com/KhronosGroup/KTX-Software/blob/master/tools/ktx2ktx2/. The repo has instructions for building them with cmake. So far, LogoETC1.ktx2 is the only file created with their tool, but I think it is more convenient than meshopt's gltfpack here because it operates on .ktx files rather than .gltf/.glb.

Edit 4/30 - I had better luck using the toktx tool instead to convert Cesium_Logo_Color.png to ktx2 with the --t2 flag.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Apr 30, 2021

Investigate alternative transcoder module

There are two libraries here that I had in mind for this to-do item:

The latter might simplify the code in parseKTX2.js and seems like it fits with any of the available transcoder modules. But, it looks like the Khronos basis transcoder doesn't support transcoding from ETC1S whereas the Binomial transcoder supports both ETC1S and UASTC. The Khronos transcoder readme does state that there are plans to implement the missing formats.

@lilleyse - maybe it's a good idea to compare both the Binomial and Khronos transcoders' performance, and maybe use one over the other depending on format? Any chance we decide not to support a particular format in the first pass here?

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Good start so far @ebogo1!

Similar to my review of #9040 I mostly just left feedback on the files surrounding the KTX2 code. I'll take a pass at the transcoder / worker code after the smaller fixes are addressed.

.vscode/settings.json Outdated Show resolved Hide resolved
Apps/Sandcastle/gallery/Materials.html Outdated Show resolved Hide resolved
Source/Renderer/Context.js Outdated Show resolved Hide resolved
Source/Scene/Model.js Outdated Show resolved Hide resolved
Source/Scene/Model.js Outdated Show resolved Hide resolved
Source/Scene/parseKTX2.js Outdated Show resolved Hide resolved
Specs/Renderer/TextureSpec.js Outdated Show resolved Hide resolved
@ebogo1
Copy link
Contributor Author

ebogo1 commented Apr 30, 2021

4d0682a adds KTX-Parse to Source/ThirdParty/Workers to cut down on the file size of parseKTX2.js. It also removed the need for getUint64FromDataView.js. Per @mramato's suggestion, I moved ktx-parse.modern.js from the node module's dist folder into our Workers folder so that it only got included from parseKTX2.js. I still have to update LICENSE.md.

@ebogo1
Copy link
Contributor Author

ebogo1 commented May 2, 2021

@lilleyse Did a first pass over some of your feedback; haven't gotten to everything yet. parseKTX2.js should be good for review now that KTX-Parse is included.

@lilleyse
Copy link
Contributor

One of the KTX2 models isn't working in Sandcastle

Specs/Data/Models/Box-Textured-KTX2-Basis/CesiumTexturedBoxTest.gltf

image

It's because the texture is 400x400 and the default sampler uses REPEAT which requires power-of-two textures in WebGL 1. We don't have a way of turning compressed textures into power-of-two textures like we do for regular textures. It works in WebGL 2 which doesn't have the same restriction: Sandcastle

image

KHR_texture_basisu doesn't have a power-of-two requirement but it does recommend it for maximum compatibility.

We should either

  • Update the test models to be 256 x 256
  • Change the wrap mode to CLAMP_TO_EDGE for non-power-of-two compressed textures.

The second option isn't great if the model is supposed to use repeat texturing, like a brick texture repeated across the side of a building. I think the first option is better for now. We should encourage power-of-two compressed textures in the Model.js documentation and explain why. We should also print a warning when this situation happens.

Another problem I noticed is that if the texture is power-of-two but the sampler requires a mipmap (like if minFilter is 9986 aka NEAREST_MIPMAP_LINEAR) and the texture does not have a mipmap it will appear black as well. This is discouraged (but not disallowed) in KHR_texture_basisu

When a texture refers to a sampler with mipmap minification or when the sampler is undefined, the KTX image SHOULD contain a full mip pyramid

The solution here would be to set the minFilter to either TextureMinificationFilter.NEAREST or TextureMinificationFilter.LINEAR like here. There's also a mistake with that code - textureMinificationFilter should be minificationFilter and textureMagnificationFilter should be magnificationFilter.

Make the same fixes in GltfLoaderUtil.createSampler which is used by the new model loading code.

image

Copy link
Contributor Author

@ebogo1 ebogo1 left a comment

Choose a reason for hiding this comment

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

Good finds - I responded to both in 4325679 . Had a couple questions -

Source/Scene/GltfLoaderUtil.js Outdated Show resolved Hide resolved
Source/Scene/GltfLoaderUtil.js Outdated Show resolved Hide resolved
Source/Scene/Model.js Show resolved Hide resolved
Source/Scene/GltfLoaderUtil.js Outdated Show resolved Hide resolved
Source/Scene/Model.js Outdated Show resolved Hide resolved
Source/Scene/Model.js Outdated Show resolved Hide resolved
Source/Scene/Model.js Outdated Show resolved Hide resolved
Source/Scene/GltfLoaderUtil.js Outdated Show resolved Hide resolved
Specs/Scene/GltfLoaderUtilSpec.js Show resolved Hide resolved
@ebogo1
Copy link
Contributor Author

ebogo1 commented Jun 28, 2021

@lilleyse Think I got everything with the GltfLoader changes.

@lilleyse
Copy link
Contributor

Looks good. I just modified the GltfTextureLoaderSpec test to piggy back off the existing glTF. I also remembered to update the models in the GltfLoader folder so that they are also 256x256.

Should be ready to merge after we update the hosted environment map.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Jun 28, 2021

Thanks. The ibl file has been updated.

@lilleyse
Copy link
Contributor

lilleyse commented Jun 28, 2021

Thanks @ebogo1. I pushed what I hope is the last commit.

I realized I forgot to change 400 to 256 in the GltfLoader tests. Also one of the texture tests was giving a slightly different result on my Mac so I changed it to use an epsilon comparison. See 5b45436.

Will merge after CI finishes.

@lilleyse lilleyse merged commit fdf1e15 into master Jun 28, 2021
@lilleyse lilleyse deleted the add-ktx2 branch June 28, 2021 17:44
@lilleyse
Copy link
Contributor

Merged. Thanks for the awesome work here @ebogo1!

@lilleyse
Copy link
Contributor

@ebogo1 I noticed an issue with IE11 that we should fix before the July release.

KTX2Transcoder.js is automatically pulling in transcodeKTX2 which pulls in parseKTX2 which pulls in ktx-parse.modern.js which uses non ES5 features (which we were aware of #9513 (comment))

This should be fixable by wrapping the _transcodeTaskProcessor creation in a getter like what DracoLoader does. That way apps that don't use KTX2 will still function in IE11 for another month.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Jun 29, 2021

For anyone else who reads this in the future, I opened #9650 to fix the IE error.

j9liu pushed a commit that referenced this pull request Jul 1, 2021
Add KTX2 support and remove KTX/CRN
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.

None yet

4 participants