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

Expose Image::get_mipmap_count() and add GLTFState::get_source_images() #79368

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wojtekpil
Copy link
Contributor

I noticed thatImage::get_mipmap_count() and GLTFState::source_images were not exposed to GDScript. First one can be quite useful for plugins that are displaying Image information etc. Second one is performance related and allows to access GLTF source images without the need of copying it from an ImageTexture. Very useful for batch tasks on GLTF files (e.g conversions)

@wojtekpil wojtekpil requested review from a team as code owners July 12, 2023 11:15
@kleonc kleonc added this to the 4.2 milestone Jul 12, 2023
@wojtekpil wojtekpil requested a review from a team as a code owner July 12, 2023 15:23
@bitsawer
Copy link
Member

bitsawer commented Jul 12, 2023

Image::get_mipmap_count() is also exposed by #74142 along with a new method and some extra documentation. I agree that Image::get_mipmap_count() is an useful method to expose, so I'm fine with either PR, atlhough the documentation in this PR could be improved a bit. There have been a few bugs where people don't remember to add 1 or use <= comparison when iterating through mipmap levels.

Also remember to squash your commits into one, see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

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.

@lyuma looks good to me, but you wanted the type changed to TypedArray? Let me know.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Previously I suggested removing source_images since it was not used very much, but it was decided to keep it. Since we are keeping source_images in the engine, it makes sense to expose getting it. As for mipmap I can't speak for that but it looks good to me too.

@lyuma
Copy link
Contributor

lyuma commented Jul 12, 2023

I would rather hold off on exposing source_images because it's not being accessed consistently in c++ and the one place that does seems inefficient and without bounds check, so maybe we need to revisit how this array is being used.

From what I see, the only purpose behind source_images is in GLTFDocument::_get_texture to recompress basis universal images every time they are accessed as a texture, without caching. It seems to me it would be easier to have GLTFDocument::_parse_image_save_image determine whether or not to create mipmaps.

It would be nice to have a way of preserving images more precisely on export without compression artifacts (for example, referencing the originally imported .png or .jpg from the project filesystem) but I think that should be done a different way.

@aaronfranke do you have a reference to the discussion that "we are keeping source_images in the engine"? I don't understand how it is intended to be used, nor how it avoids texture compression artifacts (keeping the actual source images).

@aaronfranke
Copy link
Member

@lyuma #76895 (comment)

@wojtekpil
Copy link
Contributor Author

Regarding source_images I think we just need something more than generating mipmaps. GLTFDocument::_parse_image_save_image could optionally create mipmaps and that would be great. But if I want to preprocess textures in GLTF, like even changing the saturation, and save it back to different GLTF I would really need some direct access to Image. Reading it back from Texture is just too slow from my tests. Also doesn't seems to be working well in multithreaded environment. If there is a better way of accessing source_images than it's fine for me. I just exposed source_images so it works in similar way to already exposed images (also images name is misleading as they are returning textures) . As for get_mipmap_count() I am fine with either PR but work from @bitsawer brings more functionality and better documentation so I guess it should be obvious choice.

@akien-mga akien-mga changed the title Expose Image::get_mipmap_count() and add GLTFState::get_source_images() Expose Image::get_mipmap_count() and add GLTFState::get_source_images() Aug 2, 2023
@akien-mga
Copy link
Member

Note that when a consensus is reached, the commits should be squashed. The documentation should be added in the same commit that exposes the methods.

But I guess we might hold off on merging to see if #74142 gets merged first, and the discussion about source_images reaches a consensus on what to do.

@akien-mga akien-mga modified the milestones: 4.3, 4.x Jun 12, 2024
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.

9 participants