KTX2 integration -- Dropping support for KTX #9040

YVin3D · 2020-07-20T16:37:38Z

Adds support for KTX2 textures
Drops support for KTX
Updates two sandcastles that used to contain KTX files (Materials & Image-Based Lighting)
Updates some specs that used to use KTX files.
Currently has unit tests for LoadKTX2.js that do not pass due to a race condition when calling .simulateError()

…cs to handle KTX2, added relevant models/textures

cesium-concierge · 2020-07-20T16:37:42Z

Thanks for the pull request @YoussefV!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ 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.

Reviewers, don't forget to make sure that:

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

YVin3D · 2020-07-20T16:39:41Z

PS. Need to update CHANGES.md and run prettier when everything is done (before merge)

lilleyse · 2020-07-20T17:00:52Z

@YoussefV two high level things:

You can remove crn (Crunch) support as well. Just add it to a breaking changes section in CHANGES.md.
Does the Basis transcoder support Internet Explorer? IE does not support WebAssembly so for Draco we had to create a special build as described here: DracoModuleManagement.
- It might be fine to merge this PR anyways, but we'll have to decide whether we should use KTX2/Basis by default in the tiling pipeline or if we need to provide some fallback.
- We should find the % of devices that don't support any GPU compressed texture formats, also to help us determine if we need to provide fallbacks in ion.

lilleyse · 2020-07-22T12:54:37Z

@YoussefV could you add a KTX2/Basis sample to 3d-tiles-samples-generator? That's where we generate all the test tilesets. I opened CesiumGS/3d-tiles-samples-generator#5 with more info. That sample will replace the balloon tileset in this PR.

YVin3D · 2020-07-22T13:16:42Z

@lilleyse So should we not include the Balloon 3DTile in this PR?

lilleyse · 2020-07-22T13:29:38Z

@YoussefV yes, the balloon tileset can be removed once the other sample is generated

lilleyse · 2020-07-22T13:31:04Z

Why was Apps/Sandcastle/images/whiteShapes.png removed?

YVin3D · 2020-07-22T13:35:59Z

Why was Apps/Sandcastle/images/whiteShapes.png removed?

It showed up as a white thumbnail on my computer, which is exactly how old ktx pictures looked like. So I accidentally deleted it while clearing out old .ktx files. Added it back in the latest commit. My bad!

OmarShehata · 2020-07-22T13:42:06Z

Apps/Sandcastle/gallery/Image-Based Lighting.html

-        // 1 - Download the Filament release (https://github.com/google/filament/releases).
-        // 2 - Run `cmgen --type=ktx --deploy=/path/to/output /path/to/image.hdr`. Other formats are also supported. Run `cmgen --help` for all options.
-        // 3 - Take the generated coefficients and the KTX file and load them in CesiumJS as shown below.
+        // This environment map was processed using Google's Filament project.



Are these instructions out of date/not working?

One problem we had when initially creating this Sandcastle is it was completely opaque what all these coefficients meant, or how to get your own. The user was never supposed to do this process manually. The glTF IBL extension just wasn't ready at the time.

But now it is: https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Vendor/EXT_lights_image_based#ext_lights_image_based.

This example can be greatly simplified into essentially one line if we just add support for that extension. I would suggest we do that (not necessarily in this PR, open an issue for it?), and/or just remove this example because it isn't very useful to Cesium users as-is.

Yeah, Google's Filament hasn't been updated to support KTX2. I had to rebuild this cubemap by using this website, which gave six PNG images, which I then had adjust the opacity of and then convert to KTX2 using the command line tool from this repo.

We could always open up an issue to build a tool that automates this?

One limitation of using EXT_lights_image_based is that it needs to be added to the glTF's individually whereas Cesium users might want to set an environment map for the entire scene.

Yes. It's critical for users to be able to select their own HDR, IBL environments. Can't expect ordinary models to bundle them.

Would most users know how to/want to manually specify the 9 coefficients as shown here? That was the part I was thinking we could omit from this sandcastle, and fallback to the procedural.

If you fall back to procedural, the diffuse lighting on the model won't match the specular lighting environment, which is not what a user who cares about lighting environments is going to want to see. That said, I'm no fan of manually specifying spherical harmonics in code, that seems ridiculous and error-prone. But is the alternative to force all users to always accept a default procedural diffuse lighting environment?

By comparison, BabylonJS stores pre-filtered lighting environments in .env files of their own design, which users can create in their Sandbox (instructions). The .env file contains both diffuse and specular lighting pre-filtered, making it ready to render models with. I don't know if the format is standard or open, but I suppose we could ask. For users this is very easy, just load the env and assign it to the scene, and all your models will have the correct lighting and reflections of the environment.

In ThreeJS, they can now load *.hdr files directly. But these need some pre-filtering at load time, and they have a class called PMREMGenerator that will do exactly this. This costs some load-time performance, but offers users the convenience of directly using the most common HDR environment format found online.

I like the KTX file, but it looks like the file is only storing the pre-filtered specular lighting environment, and dumping out the diffuse as a bunch of SH coefficients for users to deal with separately. Unless maybe KTX2 has some way to include the diffuse environment as well? Ideally there should be a standard environment format that includes both.

I've been working on the VSCode glTF preview, trying to get nice IBLs in all the preview windows, and it would be nice to include Cesium in this too.

In this commit I hacked up a little sh.txt file reader, which is the text file that cmgen writes out when it processed an IBL into a KTX. It loads the Spherical Harmonics and builds the array of Cartesians for them. Not sure if this will actually get published, but it seems to work.

Watching a Virtual SIGGRAPH presentation on Filament (Moving Mobile Graphics, ~2:28:00). Romain says they're not using spherical harmonics at all anymore. Instead, they stop the specular IBL mipmaps at 16x16 smallest size with roughness == 1.0 at that size, and they re-use that particular mip level as their diffuse IBL.

I think Cesium should do that too, and ditch the SH coefficients.

lilleyse · 2020-07-22T13:13:07Z

Apps/Sandcastle/gallery/Image-Based Lighting.html

-        // 1 - Download the Filament release (https://github.com/google/filament/releases).
-        // 2 - Run `cmgen --type=ktx --deploy=/path/to/output /path/to/image.hdr`. Other formats are also supported. Run `cmgen --help` for all options.
-        // 3 - Take the generated coefficients and the KTX file and load them in CesiumJS as shown below.
+        // This environment map was processed using Google's Filament project.


The environment map looks different than master. Is this because the image changed or did the IBL implementation change? The environment map should come with a prefiltered mipmap so that rougher materials produce blurrier reflections. Filament was doing this previously but I'm not sure what the new KTX2 generation process is like.

More background: https://learnopengl.com/PBR/IBL/Specular-IBL

From the Image-Based Lighting sandcastle:

master

ktx2-integration

Also the new image is 4.28MB compared to the previous 2MB. The smaller the better so that we don't bloat the CesiumJS zip release too much.

@YoussefV you mentioned that a straight up KTX -> KTX2 conversion failed. How quick would it be to write our own script just for this? I guess this depends on the number of breaking changes between KTX and KTX2.

I think it would take a while because the spec has changed a decent amount. It might be worth it to open an issue in the KTX2 repo about it if we're 100% confident the original KTX file is valid.

The reason why the KTX2 file is larger is because the mip map is uncompressed, so it unpacks the RGB values.

So I was the one who created the KTX2 image, the mip map didn't make the reflections blurrier because it autogenerated the levels without taking into account it was a specular IBL. I think the best approach here is to fix the existing converter since the problem may most likely be that the converter has a bug.

I'll open up an issue there.

Source/Core/VulkanConstants.js

Source/Scene/Model.js

Specs/Renderer/TextureSpec.js

lilleyse · 2020-07-22T14:50:04Z

Specs/Scene/OctahedralProjectedCubeMapSpec.js

@@ -113,7 +113,7 @@ describe(
        sampleOctahedralMap(octahedralMap, directionFlipY, lod, function (
          octahedralMapColor
        ) {
-          return expect(cubeMapColor).toEqualEpsilon(octahedralMapColor, 5);
+          return expect(cubeMapColor).toEqualEpsilon(octahedralMapColor, 7);


I remember talking about this offline. Once #9040 (comment) is addressed it should be fine to use 5 again.

That probably explains that. Ok will do!

Update to this: the bug currently seems to be with Filament generating an invalid KTX file. More Here

lilleyse · 2020-07-22T14:51:57Z

Few miscellaneous things:

Probably best to remove compressedImage3DTiles everywhere like what we did in Added support for KTX2 texture files, dropped support for KTX and CRN… gltf-pipeline#550. Include this change in the breaking changes section in CHANGES.md.
In the breaking changes section also mention that KTX and CRN support is removed. Point to the KTX->KTX2 converter as a suggested fallback for users with KTX textures.
Remove crunch references from gulpfile.cjs. Here and here.
The KTX models in Specs/Data/Models need to be cleaned up. There's a weird mix of things going on.
- Box-Textured-KTX is a glTF 1.0 model with the KHR_texture_basisu extension, which isn't valid
- Anything ending with OLD can be removed
- It looks like Box-Textured-KTX-Binary and Box-Textured-Embedded were converted from their glTF 1.0 counterpart and now use the KHR_techniques_webgl extension (when gltf-pipeline upgrades glTF 1.0 to 2.0 it adds the KHR_techniques_webgl to preserve the glTF 1.0 shaders) . Technically valid, but it would be simpler if it was just a normal glTF model with the basis extension.
- Overall suggestion here: let's follow what Box-Textured-Webp does - for KTX2, remove the existing KTX test models and add just a single test model called Box-Textured-KTX2-Basis that is glTF 2.0, doesn't use KHR_techniques_webgl, and uses KHR_texture_basisu.
Add KHR_texture_basisu to the list of supported extensions in the documentation above the Model constructor and Model.fromGltf

Youssef's Comment: All of the above comments have been addressed in latest commit.

lilleyse · 2020-07-22T14:55:44Z

@YoussefV do you by chance have a code example for testing KTX2 as a SingleTileImageryProvider? I'm curious to see if it works there.

lilleyse · 2020-07-22T14:58:50Z

Ok that should be it for my first round of reviews. I haven't actually looked at loadKTX2 yet, just want to get the miscellaneous stuff out of the way first.

Aside from mostly minor comments, so far so good.

YVin3D · 2020-07-22T16:38:17Z

* Add `KHR_texture_basisu` to the list of supported extensions in the documentation above the `Model` constructor and `Model.fromGltf`

Technically, this isn't released yet, so I have no "official" link for this as in here, would I just include the link to the relevant PR?

lilleyse · 2020-07-22T16:52:02Z

Technically, this isn't released yet, so I have no "official" link for this as in here, would I just include the link to the relevant PR?

Linking to the PR is fine. If this PR gets merged before KHR_texture_basisu is added to https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos you should open a CesiumJS issue that reminds us to update the link once ready.

YVin3D · 2020-07-22T17:21:57Z

@lilleyse made an incremental push with most of your comments addressed, there still remains a few which are not struck out above. I removed the commented code in TextureSpec.js, the failing tests are in LoadKTX2Spec.js:

This test: returns a promise that rejects when the request errors is the best example.

YVin3D · 2020-07-22T18:23:52Z

@YoussefV do you by chance have a code example for testing KTX2 as a SingleTileImageryProvider? I'm curious to see if it works there.

Talked about this offline, it works with the Imagery Layers sandcastle, just replace the filename to Logo.ktx2

cesium-concierge · 2020-08-25T02:00:03Z

Thanks again for your contribution @YoussefV!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

lilleyse · 2020-08-25T02:03:00Z

@cesium-concierge stop

likangning93 · 2020-10-07T17:45:24Z

Source/Scene/Model.js

@@ -173,6 +172,8 @@ var uriToGuid = {};
 * {@link https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_techniques_webgl/README.md|KHR_techniques_webgl}
 * </li><li>
 * {@link https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_texture_transform/README.md|KHR_texture_transform}
+ * </li><li>
+ * {@link https://github.com/KhronosGroup/glTF/pull/1751|KHR_texture_basisu}


As of this writing the PR has been accepted: https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_texture_basisu

I closed #9054, we can get the fix into this PR now.

likangning93 · 2020-10-12T20:30:09Z

the tale of `box-textured-basis.gltf`

@lilleyse asked me to take a look at what's still needed to get this PR production-ready. Part of that was investigating why a model with KTX2 texture (ETC1S) from gltfpack was loading in Don Mccurdy's three-gltf-viewer but not in this branch.

box-textured.zip
localhost sandcastle - assumes model is in SampleData/models/KTX2

Specifically, this branch was getting:

KTX2 Loading Error:
RuntimeError: Error transcoding Image

Here are my observations and larger questions from my investigation:

transcoder interface change!

I started reading the code here and the doc for the transcoder wrapper, and I noticed that the interfaces seemed considerably different. As far as I can tell the interface changed somewhere around KhronosGroup/KTX-Software@5366cd1#diff-d620c588208646e4731bb46c099148e8L454

~~I have a WIP commit here that brings a newer build in and fixes some other bugs that were causing transcoding specifically to fail: 1addfe6~~
~~I tracked some of these down by debugging Cesium side-by-side with three-gltf-viewer.~~

[EDIT] fixed in d4a02b7

The newest version of the transcoder also looks to support UASTC encoding, which this PR currently does not support.

need to handle mip levels

My testing branch seems to transcode correctly but still has trouble making the texture with all the mip levels. Looking at our Texture.js it looks like we support pre-generated mip levels in Cesium, so hopefully this is just a wiring thing.

Note that box-textured doesn't have any samplers - we should follow the spec: https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_texture_basisu

[EDIT] fixed in d4a02b7

should we check for more compressed texture extensions?

cesium context doesn't check for as many compressed texture extensions as three.js seems to. See:

https://github.com/CesiumGS/cesium/blob/master/Source/Renderer/Context.js#L308 vs. https://github.com/mrdoob/three.js/blob/dev/examples/jsm/loaders/KTX2Loader.js#L81.

This may need additional investigation and might not be actionable in this PR, but any changes to the compressed texture formats that we check for will probably necessitate changes to the logic in this branch for picking target texture formats.

how can we avoid monkeypatching the transcoder's built files?

Building the KTX2 web transcoder produces msc_basis_transcoder.js and msc_basis_transcoder.wasm.

The msc_basis_transcoder.js in this branch (and also in my WIP branch) has been monkeypatched to include export default and a modified path to the .wasm file in Cesium's project structure, which seems unsafe. I don't know how/whether this works with built Cesium.

~~[EDIT] My testing seems to show that the deployed, built version of this branch isn't working due to wasm loading problems, see here on Sandcastle.~~

Fixed in 6358371

other TODOs I'd like to get in:

implement support for UASTC encoding
update sample models to be spec-compliant and cover all codepaths here
handle spec cases like https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_texture_basisu
update unit tests for coverage
look into a asm.js fallback for platforms that don't support WASM
additional build options for the transcoder (smaller? faster?)
write a guide similar to https://github.com/CesiumGS/cesium/tree/master/Documentation/Contributors/DracoModuleManagement
- I'd also like it if future updates included the build commit hash
- so future devs will know more immediately what version of documentation to reference
test on a couple different devices, transcode to uncompressed instead of throwing if no compressed format is available
fall back to uncompressed target for platforms without compressed texture support on the GPU

[ci skip]

(built off commit 78790b5)

…e 78790b5

ebogo1 · 2021-04-27T22:28:03Z

continued in #9513...

YVin3D added 6 commits June 3, 2020 13:04

KTX2 support added

bb98300

Added Fix for ETC1

3fb2094

Weird then chaining bug

94a8081

Added transcoding testing code

d5b3893

Reverted back to production build for testing GLTF-KTX2 files

f47b6d5

Dropped KTX, Updated sandcastles to support KTX2 instead, updated spe…

bf5bc7b

…cs to handle KTX2, added relevant models/textures

YVin3D requested review from lilleyse and OmarShehata July 20, 2020 16:37

Updated specs to remove CRN, also updated LICENSE.md

04e58bf

lilleyse mentioned this pull request Nov 21, 2022

Add KTX2/Basis sample CesiumGS/3d-tiles-samples-generator#5

Open

Added back missing whiteShapes.png file

f808f11

OmarShehata reviewed Jul 22, 2020

View reviewed changes

lilleyse reviewed Jul 22, 2020

View reviewed changes

Incremental changes made based on PR, loadKTX2Spec still fails

b03dc27

Updated Spec models with GLTF 2.0 embedded KTX2 model as per PR

fda4f2f

Removed compressedImage3DTiles

6dda57e

YVin3D mentioned this pull request Jul 22, 2020

Update docs in Model.js to point to correct url for KHR_texture_basisu extension #9054

Closed

YVin3D added 3 commits July 22, 2020 17:51

Updated GltfPipeline

cf3d392

added support for Env map pixel type

b3c83f3

Fixed environment map parsing

008e5ef

lilleyse mentioned this pull request Aug 10, 2020

Add support for R11F_G11F_B10F, and/or fix IBL instructions in Sandcastle #9088

Open

likangning93 reviewed Oct 7, 2020

View reviewed changes

likangning93 and others added 3 commits October 13, 2020 13:48

update transcoder to 78790b5, fix some bugs with KTX2 (ECT1S) mipmaps

d4a02b7

[ci skip]

fix typescript error

fdfa914

move KTX2 parsing to a webworker, un-monkeypatch msc_basis_transcoder.js

6358371

(built off commit 78790b5)

likangning93 force-pushed the ktx2-integration branch from 58ad10f to 6358371 Compare October 15, 2020 18:44

likangning93 and others added 2 commits October 16, 2020 15:53

add IE11 support with a Javascript fallback built against KTX-Softwar…

19e9247

…e 78790b5

fix parseKTX2 bug with non-square textures

a66e5e0

ebogo1 mentioned this pull request Apr 27, 2021

Add KTX2 support and remove KTX/CRN #9513

Merged

21 tasks

ebogo1 closed this Apr 27, 2021

ebogo1 deleted the ktx2-integration branch April 29, 2021 14:47

lilleyse restored the ktx2-integration branch April 30, 2021 20:37

ebogo1 mentioned this pull request May 12, 2021

Ktx2 integration #9536

Closed

KTX2 integration -- Dropping support for KTX #9040

KTX2 integration -- Dropping support for KTX #9040

Conversation

YVin3D commented Jul 20, 2020 • edited Loading

cesium-concierge commented Jul 20, 2020

YVin3D commented Jul 20, 2020

lilleyse commented Jul 20, 2020

lilleyse commented Jul 22, 2020

YVin3D commented Jul 22, 2020 • edited Loading

lilleyse commented Jul 22, 2020

lilleyse commented Jul 22, 2020

YVin3D commented Jul 22, 2020 • edited Loading

OmarShehata Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Jul 22, 2020 • edited by YVin3D Loading

lilleyse commented Jul 22, 2020

lilleyse commented Jul 22, 2020

YVin3D commented Jul 22, 2020

lilleyse commented Jul 22, 2020

YVin3D commented Jul 22, 2020

YVin3D commented Jul 22, 2020

cesium-concierge commented Aug 25, 2020

lilleyse commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likangning93 commented Oct 12, 2020 • edited Loading

the tale of box-textured-basis.gltf

transcoder interface change!

need to handle mip levels

should we check for more compressed texture extensions?

how can we avoid monkeypatching the transcoder's built files?

other TODOs I'd like to get in:

ebogo1 commented Apr 27, 2021

YVin3D commented Jul 20, 2020 •

edited

Loading

YVin3D commented Jul 22, 2020 •

edited

Loading

YVin3D commented Jul 22, 2020 •

edited

Loading

OmarShehata Jul 22, 2020 •

edited

Loading

lilleyse commented Jul 22, 2020 •

edited by YVin3D

Loading

likangning93 commented Oct 12, 2020 •

edited

Loading

the tale of `box-textured-basis.gltf`