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

Expected positive integer for width but received 0 of type number when running optimize #1500

Open
hybridherbst opened this issue Sep 10, 2024 · 4 comments
Labels
feature New enhancement or request package:functions

Comments

@hybridherbst
Copy link

Describe the bug
The attached model causes an error during gltf-transform optimize.
My understanding is that the textures use non-standard ASTC6x6 which is understood by three.js, but maybe not yet in gltf-transform.

To Reproduce
Steps to reproduce the behavior:

  1. Download d9836c94f70dcebf0bc6.glb.zip
  2. Run gltf-transform optimize d9836c94f70dcebf0bc6.glb d9836c94f70dcebf0bc6.opt.glb
  3. Note error
image

Expected behavior
Can be optimized and/or meshopt compressed

Versions:
4.0.8

Additional context
The model is non-standard in some ways, see https://x.com/hybridherbst/status/1833281592351166901 for a bit of a breakdown

@hybridherbst hybridherbst added the bug Something isn't working label Sep 10, 2024
@donmccurdy
Copy link
Owner

Seems that gltf-transform inspect fails too:

error: Unexpected KTX2 colorModel, "162".

I don't think this is technically a valid file per spec, but agree that glTF Transform should be able to ignore unknown texture types safely, and will look into fixing that!

@hybridherbst
Copy link
Author

Thank you! I opened a similar issue for gltf.report as well by the way, which logs that same error message regarding the color model.

@donmccurdy
Copy link
Owner

donmccurdy commented Sep 11, 2024

Currently in textureCompress we filter out textures by MIME type:

if (!SUPPORTED_MIME_TYPES.includes(texture.getMimeType())) {
logger.debug(`${prefix}: Skipping, unsupported texture type "${texture.getMimeType()}".`);
return;

I think we may need an additional sanity-check with ImageUtils#getMimeType on the bytes to check that the specified MIME type is actually correct, and perhaps also attempt to read the width/height of the image before we continue to compression.

Similar for inspect.ts. I'm traveling this week but will continue on this when I'm back!

@donmccurdy
Copy link
Owner

donmccurdy commented Sep 15, 2024

invalid_mime_type

Hm, there are a few issues:

  1. File contains images with .jpg extensions, but which declare image/ktx2 as the MIME type. This breaks texture-compress.ts. At least some of the textures are really KTX2, I didn't check them all.
  2. File fails validation with additional Recognized image format 'image/jpeg' does not match declared image format 'image/png' errors. I can't tell if that affects glTF Transform.
  3. KTX2 files use plain ASTC, as you mention, which isn't allowed by KHR_texture_basisu and isn't supported by glTF Transform. For three.js it might be OK, but would require the viewer to have an ASTC-compatible GPU.

I support fixing (3); glTF Transform should be able to process glTF files containing .ktx2 textures that don't use or conform to KHR_texture_basisu, ignoring the textures as needed. That's necessary to support userland implementation of extensions like KHR_texture_astc. But issues (1) and (2) I'm not so sure about, trying to sniff the correct MIME type and override what the .gltf file claims does not feel great.

I'd be happy to accept a PR for (3), or to try an implementation if we have a .gltf sample containing an ASTC .ktx2 texture without these other issues. But in the meantime, I may leave this open at lower priority. Marking this as "enhancement" since the required changes would involve adding more checks and logic for previously unsupported types of KTX2 data.

@donmccurdy donmccurdy added feature New enhancement or request and removed bug Something isn't working needs investigation labels Sep 15, 2024
@donmccurdy donmccurdy modified the milestones: v4.0, 🗄️ Backlog Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New enhancement or request package:functions
Projects
None yet
Development

No branches or pull requests

2 participants