-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
Thanks for the pull request @ebogo1!
Reviewers, don't forget to make sure that:
|
As a note, I have been using Khronos's Edit 4/30 - I had better luck using the |
There are two libraries here that I had in mind for this to-do item:
The latter might simplify the code in @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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4d0682a adds KTX-Parse to Source/ThirdParty/Workers to cut down on the file size of |
@lilleyse Did a first pass over some of your feedback; haven't gotten to everything yet. |
One of the KTX2 models isn't working in Sandcastle
It's because the texture is 400x400 and the default sampler uses KHR_texture_basisu doesn't have a power-of-two requirement but it does recommend it for maximum compatibility. We should either
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
The solution here would be to set the Make the same fixes in |
There was a problem hiding this 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 -
@lilleyse Think I got everything with the |
Looks good. I just modified the Should be ready to merge after we update the hosted environment map. |
Thanks. The ibl file has been updated. |
Merged. Thanks for the awesome work here @ebogo1! |
@ebogo1 I noticed an issue with IE11 that we should fix before the July release.
This should be fixable by wrapping the |
For anyone else who reads this in the future, I opened #9650 to fix the IE error. |
Add KTX2 support and remove KTX/CRN
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:
loadCRN
andloadKTX
and replace withloadKTX2
loadKTXSpec
->loadKTX2Spec
parseKTX2.js
, which is called from thetranscodeKTX2
worker'stranscode()
functionTo-do:
Investigate alternative transcoder module - https://github.com/KhronosGroup/Universal-Texture-Transcoders(this will be a follow-up PR)loadKTX2
/KTX2Transcoder
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.