Skip to content

RenderingDevice: Pass mipmap count to texture_create_from_extension()#105570

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
kroketio:texture-extension-mipmaps
May 8, 2025
Merged

RenderingDevice: Pass mipmap count to texture_create_from_extension()#105570
Repiteo merged 1 commit intogodotengine:masterfrom
kroketio:texture-extension-mipmaps

Conversation

@kroketio
Copy link
Contributor

@kroketio kroketio commented Apr 19, 2025

Godot hardcodes mipmap count in texture_create_from_extension()

texture.mipmaps = 1;

My texture is shown in 3D, and with Godot ignoring mips, it looks worse than needed. For that reason I introduced parameter p_mipmaps to support specifying mipmap count.

Changes the function signature, however I suspect not many are using it, apart from the internal openxr module.

@kroketio
Copy link
Contributor Author

Alternatively, to maintain compatibility., parameter p_mipmaps can be given a default of 1.

@kroketio kroketio force-pushed the texture-extension-mipmaps branch from fcf71bf to 5facc51 Compare April 19, 2025 16:19
@kroketio kroketio requested a review from a team as a code owner April 19, 2025 16:19
@clayjohn
Copy link
Member

You will need to add compatibility bindings since this breaks compatibility. How to do so is described here https://docs.godotengine.org/en/latest/contributing/development/handling_compatibility_breakages.html

@kroketio kroketio force-pushed the texture-extension-mipmaps branch from 5facc51 to f17dbd8 Compare April 19, 2025 17:32
@kroketio kroketio requested review from a team as code owners April 19, 2025 17:32
@kroketio kroketio force-pushed the texture-extension-mipmaps branch 4 times, most recently from b2f0881 to 6d14273 Compare April 20, 2025 05:29
@kroketio
Copy link
Contributor Author

Thanks, PR updated.

@kroketio kroketio force-pushed the texture-extension-mipmaps branch from 6d14273 to b9aac3c Compare May 5, 2025 16:17
@Mickeon
Copy link
Member

Mickeon commented May 5, 2025

Given the breaking nature of this, this could be further discussed by opening a proposal. Although, I'm not hearing objections, and I do understand that talks about RenderingDevice will probably fly over most people's heads

Copy link
Member

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Description is okay. Even without this PR it's very, very difficult to localize, but it's good to mention the new parameter at least.

@kroketio
Copy link
Contributor Author

kroketio commented May 5, 2025

The change looks scary, but this function in general was only exclusively used by the internal openxr module up until last September when I exposed it for use from a gdextension #96860, as up till then, that was not possible. This is why I suspect I am the only user. Regardless of the number of users, anyone who imports an external texture would also like to specify the mipmap count I believe, especially if that texture is to be shown in 3D.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

This looks OK to me!

As far as compatibility, I think this is fine with the compatibility methods for GDExtension. Given that you need to pass in a native handle, this API can't really be used from anything except GDExtension, so we shouldn't need to worry about GDScript breakage.

@Mickeon Mickeon modified the milestones: 4.x, 4.5 May 7, 2025
@kroketio kroketio force-pushed the texture-extension-mipmaps branch from b9aac3c to f4fd23d Compare May 7, 2025 12:15
@kroketio kroketio force-pushed the texture-extension-mipmaps branch from f4fd23d to 6ae50ca Compare May 7, 2025 12:16
@kroketio
Copy link
Contributor Author

kroketio commented May 7, 2025

Sorry, missed the review(s) by @AThousandShips
PR updated.

@Repiteo Repiteo requested a review from clayjohn May 7, 2025 16:53
@akien-mga akien-mga changed the title RenderingDevice: pass mipmap count to texture_create_from_extension() RenderingDevice: Pass mipmap count to texture_create_from_extension() May 8, 2025
@Repiteo Repiteo merged commit 288822e into godotengine:master May 8, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented May 8, 2025

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.

6 participants