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

(WIP) KTX2Loader: Return DataTexture when transcoding to uncompressed format #29926

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Nov 19, 2024

Work in progress. Tested and working for:

  • DataTexture
  • DataArrayTexture
  • DataCubeTexture ❌
  • CompressedTexture
  • CompressedArrayTexture
  • CompressedCubeTexture

Related:

Copy link

github-actions bot commented Nov 19, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.13
79
339.14
79
+16 B
+2 B
WebGPU 478.72
132.72
478.73
132.72
+16 B
+1 B
WebGPU Nodes 478.18
132.6
478.2
132.61
+16 B
+4 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 464.59
111.96
464.59
111.96
+0 B
+0 B
WebGPU 546.88
148.18
546.88
148.18
+0 B
+0 B
WebGPU Nodes 502.76
137.89
502.76
137.89
+0 B
+0 B

@donmccurdy
Copy link
Collaborator Author

@Mugen87 I'm running into the old problems from #26642 (comment) here, I'm just not sure how to properly set up mipmaps for DataTexture, DataArrayTexture, and DataCubeTexture classes. I've left Data3DTexture out of scope for this PR. I see what looks like a TODO about it here:

if ( useTexStorage && allocateMemory ) {
// TODO: Uniformly handle mipmap definitions
// Normal textures and compressed cube textures define base level + mips with their mipmap array
// Uncompressed cube textures use their mipmap array only for mips (no base level)
if ( mipmaps.length > 0 ) levels ++;

I also feel worried about this pattern...

texture = new DataCubeTexture( mipmaps[ 0 ].data, format, type );
texture.mipmaps = mipmaps.map( ( mip ) => new DataCubeTexture( mip.data, mip.width, mip.height ) );

... since the other existing pattern is to initialize a CubeTexture with an array of 6 DataTextures, but here we have one DataCubeTexture per mip, not per face, and it's pretty inconsistent. Is this something we could work out for the Data*Texture classes, independently of KTX2Loader?

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 20, 2024

Is this something we could work out for the Data*Texture classes, independently of KTX2Loader?

Yes, it think this TODO should eventually be resolved. Ideally, the image and mipmaps property behaves the same for all texture types.

The policy should be: If no mipmaps are present, the engine evaluates the image property. If mipmaps are defined, the first entry in the mipmaps array is the base level, the subsequent entries represent the mips. Cube textures should be changed so they honor this policy.

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.

2 participants