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 external texture support #83519

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Oct 17, 2023

Starting a draft to discuss the proper way to integrate support for external texture in Godot 4.x.

This is feature that's forward-ported from Godot 3.x and is key to enable support for texture that can be painted by the target platform (see the original PR for target use-cases).

As mentioned in #36342 (review), this was punted for the 4.x release due to GLES reimplementation and the need to figure out how this applies to Vulkan.

This draft aims to restart the conversation in order to properly integrate it in Godot 4.x

Addresses #65778
Addresses godotengine/godot-proposals#4902

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 17, 2023

cc @maunvz

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 17, 2023

@paddy-exe FYI, this is required to support ARCore in Godot

@m4gr3d m4gr3d force-pushed the add_support_for_opengl_external_textures_main branch from b8d233e to 460fb4e Compare October 17, 2023 19:47
@dsnopek
Copy link
Contributor

dsnopek commented Oct 17, 2023

How does this relate to textures created via TextureStorage::texture_create_external() (in the OpenGL renderer)? That function is being used for OpenXR (on OpenGL) and WebXR.

Or, if this doesn't relate to that (like, if it's a totally different thing) then we should probably rename one of them, because having two unrelated types of textures both called "external" is going to be confusing :-)

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 17, 2023

How does this relate to textures created via TextureStorage::texture_create_external() (in the OpenGL renderer)? That function is being used for OpenXR (on OpenGL) and WebXR.

Or, if this doesn't relate to that (like, if it's a totally different thing) then we should probably rename one of them, because having two unrelated types of textures both called "external" is going to be confusing :-)

@dsnopek they're indeed different things; see this comment that goes on the difference between the two: #65778 (comment)

Renaming is made complex by the fact the one I'm referring to is part of the OpenGL spec, so arguably the current Godot one should be renamed to be consistent with external references, but I don't have a strong preferences either way so long as it's clear which one we're referring to.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 17, 2023

As mentioned in this StackOverlfow thread, this may be supported in Vulkan using the VK_ANDROID_external_memory_android_hardware_buffer extension.

@m4gr3d m4gr3d force-pushed the add_support_for_opengl_external_textures_main branch 4 times, most recently from 1926f1c to 5fd411f Compare October 17, 2023 22:50
Co-authored-by: Mauricio Narvaez <nvz@meta.com>
@m4gr3d m4gr3d force-pushed the add_support_for_opengl_external_textures_main branch from 5fd411f to d9309cb Compare October 18, 2023 03:55
@dsnopek
Copy link
Contributor

dsnopek commented Oct 18, 2023

they're indeed different things; see this comment that goes on the difference between the two: #65778 (comment)

Renaming is made complex by the fact the one I'm referring to is part of the OpenGL spec, so arguably the current Godot one should be renamed to be consistent with external references, but I don't have a strong preferences either way so long as it's clear which one we're referring to.

I think it'd be fine to rename the existing TextureStorage::texture_create_external() to something else - it's not exposed publicly anywhere, and only used in those two places. I don't have any brilliant naming ideas, though. :-)

Maybe:

  • TextureStorage::texture_create_foreign()
  • TextureStorage::texture_create_reference() -- the idea being it "references" a texture created elsewhere
  • TextureStorage::texture_create_remote()
  • TextureStorage::texture_create_third_party()
  • TextureStorage::texture_create_independent()

Naming is hard :-)

EDIT: I forgot to add that I'm the one who originally named TextureStorage::texture_create_external() (in #67775), so I don't think anyone else is likely to object to a name change here

@BastiaanOlij
Copy link
Contributor

cc @clayjohn , you should have a look at this too.

@myselfuser1
Copy link

Is this android specific or will it work on iOS as well?

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Dec 5, 2023

Is this android specific or will it work on iOS as well?

It's not Android specific in theory, but it uses OpenGLES and Vulkan extensions which iOS doesn't (officially) support, so there would need to be investigation on whether this is available to iOS through the available compatibility APIs.

cc @BastiaanOlij @clayjohn

@BraveEvidence
Copy link

Any updates on when will this get merged?

@dsnopek
Copy link
Contributor

dsnopek commented Sep 19, 2024

Superseded by #96982

@dsnopek dsnopek closed this Sep 19, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Sep 19, 2024
@m4gr3d m4gr3d deleted the add_support_for_opengl_external_textures_main branch September 29, 2024 01:58
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.

7 participants