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 support for extending GLTF with more texture formats and support WebP #76895

Merged
merged 1 commit into from
May 26, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented May 10, 2023

This PR improves the GLTF importer by allowing extensions to add support for image and texture formats. This PR also adds support for WebP importing via EXT_texture_webp as an example to prove that it works, and since WebP support is already a part of Godot it's pretty simple to say this should be core.

These same extension hooks can be used to implement other formats, so #76572 can be updated after this PR is merged to use this API. This also gives us the option to have KTX be an extension in the asset library instead of core. I don't have an opinion of whether it should be core or not, but regardless this extension system is cleaner.

Two new methods have been added to GLTFDocumentExtension:

  • _parse_image_data: Passes in the image bytes and MIME type as parameters, which can be used to import an image by setting data of the Image passed into the r_image return parameter.
  • _parse_texture_json: Passes in the texture JSON, which can be used to set the parameters of the GLTF texture.

Some notes and changes to GLTFDocument:

  • The default value of the source image index in GLTFTexture was changed from 0 to -1. It was a mistake in the old code to have 0 be the default since 0 is a valid image index, the default should be invalid.
  • In the current master, if a MIME type or file extension is not recognized the import will prematurely stop. Now it will continue, trying all available paths before giving up. I have done some testing with several types of files with invalid data to confirm that this does not crash the engine.
  • I removed Vector<Ref<Image>> source_images; since it was unnecessary. It was not used consistently, and only read in one place, which I have replaced with just reading Vector<Ref<Texture2D>> images;
  • The code in the _parse_image_bytes_into_image and _parse_image_save_image methods used to be inside of _parse_images, but that method was very long and hard to read, so I split it up. This isn't strictly necessary for adding support for more texture formats but it significantly helps with readability.
  • These changes only add support for importing multiple formats, not exporting them. In the future if we want to support that we will first need an export dialog (I plan to make a proposal for this... eventually).
  • Both on the current master and in this PR, the extract textures option always saves as .png, regardless of what was actually contained in the GLTF file (so a GLTF file with a JPEG will save a .png when extracting). I asked @fire about this and he does not think it's a problem. Even if we want to change this, it could wait for another PR, I just wanted to make a note here in case people are wondering why a GLTF using WebP is placing .png on the file system.

Test project (includes both PNG and WebP, stored as embedded, separate, and base64): gltf-webp-import-test.zip

This work was sponsored by The Mirror.

@aaronfranke aaronfranke added this to the 4.x milestone May 10, 2023
@aaronfranke aaronfranke requested a review from a team as a code owner May 10, 2023 03:18
@RevoluPowered RevoluPowered requested a review from fire May 10, 2023 08:54
Copy link
Contributor

@RevoluPowered RevoluPowered left a comment

Choose a reason for hiding this comment

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

Checked LGTM but needs wide review :)

@fire
Copy link
Member

fire commented May 10, 2023

To add color to the JPG -> PNG -> Godot texture format note.

I think that JPG -> PNG is a lossless conversion, while JPG -> JPG is not, as I was not aware of a way to lossless encode jpg. I think it's theoretically possible to lossless encode JPG -> JPG.

It should be possible to directly byte array copy to the filesystem from the gltf buffer which is preferred. The extension is guessed from the required mime type.

JPG -> WEBP can also be a lossless conversion, but JPG -> WEBP can also be a narrowing conversion (do not like narrowing conversions).

@fire
Copy link
Member

fire commented May 10, 2023

Vector<Ref<Image>> source_images; was used so that the gltf document extension writers have the original image, I was using it to give Basisu a normal map version in a pull request to fix a bug. I recall it was a second conversion for normal maps.

So removing it means extension writers will overwrite generation wise every time it processes the images. I prefer to keep it.

@fire
Copy link
Member

fire commented May 10, 2023

What was changed?

@aaronfranke
Copy link
Member Author

aaronfranke commented May 10, 2023

@fire I updated the PR to re-add Vector<Ref<Image>> source_images;. Builds are failing because GitHub Actions outage not anything in the PR.

It should be possible to directly byte array copy to the filesystem from the gltf buffer which is preferred.

Yeah that's the idea (for a future PR).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Would be good to have it checked by @fire or @lyuma.

@fire fire requested a review from a team May 22, 2023 12:14
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Added some comments on my first pass of review.

modules/gltf/doc_classes/GLTFTexture.xml Outdated Show resolved Hide resolved
modules/gltf/gltf_document.cpp Outdated Show resolved Hide resolved
@fire fire modified the milestones: 4.x, 4.1 May 22, 2023
@fire fire requested a review from a team May 22, 2023 17:58
@akien-mga akien-mga merged commit cb711a9 into godotengine:master May 26, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants