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

Vulkan: Remove texture allocation check #96832

Closed
wants to merge 1 commit into from

Conversation

kroketio
Copy link
Contributor

During DEBUG_ENABLED (the default for Godot editor as distributed by Godot) a texture allocation check in texture_create_shared() causes foreign textures (those without tex_info->allocation) created by texture_create_from_extension() to fail, because such textures from external sources naturally lack the required allocation information.

#ifdef DEBUG_ENABLED
ERR_FAIL_COND_V(!owner_tex_info->allocation.handle, TextureID());
#endif

For example, a VkImage may be passed to texture_create_from_extension(), which is a function to handle incoming textures from external sources:

RID RenderingDevice::texture_create_from_extension(TextureType p_type, DataFormat p_format, TextureSamples p_samples, BitField<RenderingDevice::TextureUsageBits> p_usage, uint64_t p_image, uint64_t p_width, uint64_t p_height, uint64_t p_depth, uint64_t p_layers) {
_THREAD_SAFE_METHOD_
// This method creates a texture object using a VkImage created by an extension, module or other external source (OpenXR uses this).

But when the result is applied as albedo on a StandardMaterial3D, it will lead to the following error.

ERROR: Condition "!owner_tex_info->allocation.handle" is true. Returning: TextureID()
   at: texture_create_shared (drivers/vulkan/rendering_device_driver_vulkan.cpp:1600)
ERROR: Condition "!texture.driver_id" is true. Returning: RID()
   at: texture_create_shared (servers/rendering/rendering_device.cpp:906)
ERROR: Condition "!owner_tex_info->allocation.handle" is true. Returning: TextureID()
   at: texture_create_shared (drivers/vulkan/rendering_device_driver_vulkan.cpp:1600)
ERROR: Condition "!texture.driver_id" is true. Returning: RID()
   at: texture_create_shared (servers/rendering/rendering_device.cpp:906)

This PR removes the check, because for foreign textures (like VkImage) you only need a view, which texture_create_from_extension creates, and then just display the view, regardless of allocation information. In simple terms, this PR is necessary for external renderers to supply Godot with external textures.

During DEBUG_ENABLED, the default for the Godot editor, a texture allocation check in `texture_create_shared()` causes foreign textures (without `tex_info->allocation`) created by `texture_create_from_extension()` to fail, because those textures naturally lack the required allocation information.
@kroketio kroketio requested a review from a team as a code owner September 10, 2024 23:48
@clayjohn clayjohn added this to the 4.x milestone Sep 11, 2024
@clayjohn
Copy link
Member

I've requested a review from @RandomShaper as he added that check when doing a massive refactor of the RenderingDevice in #83452

@kroketio
Copy link
Contributor Author

For context, this was also discussed July 10 in #rendering @ dev chat:

me:
ERROR: Condition "!owner_tex_info->allocation.handle" is true. Returning: TextureID()

dariasamo: 
@RandomShaper It seems like the driver validates whether a texture is not a slice 
by checking the allocation, which means you can't create a shared version or a 
slice out of an externally created texture. Should it check instead by storing some 
other flag?

randomshaper: 
To be fair, I know very little about that method.
If I recall correctly, it was added by mux213 to support certain VR needs.
Oh, you already mostly said that.
I have no idea how flexible it is in practice beyond its original purpose.

mux213:
Indeed, that function was added for XR so we can render to a texture created 
outside of the core of Godot. There are definitely limitations here as Godot 
does not control this texture. Can’t say for sure whether this situation 
should work, will have to take a closer look tomorrow.

dariosamo:
It should be fine, it's just a texture view.

@akien-mga akien-mga changed the title vulkan: remove texture allocation check Vulkan: Remove texture allocation check Sep 11, 2024
@RandomShaper
Copy link
Member

The check has the purpose of validating if the id corresponds to an actual texture owning the data, which is what RenderingDevice is expected to provide and so keep the driver free of having to resolve that itself.

Rather than removing the check, I'd suggest enrichening it so it also accounts for the cases of externally backed textures (i.e., created from extension):

  • TextureInfo would get the following added below allocation:
    #ifdef DEBUG_ENABLED
    bool created_from_extension = false;
    #endif
    
  • texture_create_from_extension() would do this last thing before the return:
    #ifdef DEBUG_ENBLED
    tex_info->created_from_extension = true;
    #endif
    
  • The checks in both texture_create_shared() and texture_create_shared_from_slice() would change from
    ERR_FAIL_COND_V(!owner_tex_info->allocation.handle, TextureID());
    to
    ERR_FAIL_COND_V(!owner_tex_info->allocation.handle && !owner_tex_info->created_from_extension, TextureID());

As a final note, created_from_extension is the name that perfectly fits the current cases. If in the future another reason for a texture to be externally backed comes, we could just rename it to the more generic externally_backed.

kroketio added a commit to kroketio/godot that referenced this pull request Sep 11, 2024
…xture_create_from_extension()`), as such textures lack ownership information.

More info: godotengine#96832
@kroketio
Copy link
Contributor Author

that indeed sounds like a better idea @RandomShaper
I opened a new PR over at #96860

@kroketio kroketio closed this Sep 11, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Sep 11, 2024
kroketio added a commit to kroketio/godot that referenced this pull request Sep 12, 2024
…xture_create_from_extension()`), as such textures lack ownership information.

More info: godotengine#96832
laurentmackay pushed a commit to metapfhor/godot that referenced this pull request Oct 30, 2024
…xture_create_from_extension()`), as such textures lack ownership information.

More info: godotengine#96832
ChrisBase pushed a commit to ChrisBase/godot that referenced this pull request Nov 15, 2024
…xture_create_from_extension()`), as such textures lack ownership information.

More info: godotengine#96832
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