Skip to content

Conversation

@s-ol
Copy link
Contributor

@s-ol s-ol commented Nov 13, 2025

Description
Instead of having two std::Option<> types that are logically mutually exclusive, this unifies them in an enum.

As a result, vk::Device::texture_from_raw() becomes more generic and can be used more consistently in the wgpu-hal::vulkan implementation, which also fixes inconsistent usage of the "textures" counter.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@s-ol
Copy link
Contributor Author

s-ol commented Nov 13, 2025

from Matrix:
image

This improves the image counter situation, but I think it's still wrong - counters are incremented both on wgpu_hal::Texture creation, as well as when they are upleveled to wgpu::Texture; but they are decremented just once in wgpu::Device::destroy_texture(). However I took a brief look at other backends and they seem to follow the same inconsistent logic so I've left it alone for now in case I'm missing something about how the uplevelling works.

@s-ol s-ol force-pushed the vulkan-texture-unify-memory branch 2 times, most recently from 666d173 to 74c1319 Compare November 13, 2025 15:10
@Wumpf
Copy link
Member

Wumpf commented Nov 23, 2025

It seems to be a bit underdocumented how HalCounters is supposed to behave with raw textures. But looking through this it seems consistent if all textures that are injected into the system are created via texture_from_raw: wgpu-core always calls unsafe { self.raw().add_raw_texture(&*hal_texture) }; when creating a texture from hal which does the counter increment which texture_from_raw can't do because it doesn't have access to the counters.

Now I do agree that this is very wonky: if a user were to create a texture from the hal device via create_texture and then inject that texture again on the highlevel end on create_texture_from_hal we do double count that texture and never decrement. So to fix that we'd somehow need to know prior to calling add_raw_texture via which path the hal texture was created 🤔
It also really doesn't help that texture_from_d3d11_shared_handle is a lot more akin to create_texture in this regard rather than texture_from_raw. Given that this function isn't even exposed via any high level api there's definitely stuff going wrong here :/

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

change looks nice!
As you wrote yourself let's leave the counter kerfuffle for another iteration (with the exception of not bumping in texture_from_d3d11_shared_handle, agree that that this is better than before!).
Also CI needs fixing - a doc string has trouble

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

(oops, wrong button, slight changes are needed)

@s-ol s-ol force-pushed the vulkan-texture-unify-memory branch from 74c1319 to 9591f6e Compare November 29, 2025 14:37
@s-ol
Copy link
Contributor Author

s-ol commented Nov 29, 2025

thanks @Wumpf, I reverted the two requested hunks

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

thanks!
Some doc build things need attention (see ci). Otherwise good to go I think!

s-ol added 2 commits November 30, 2025 11:51
Instead of having two std::Option<> types that are logically mutually exclusive,
this unifies them in an enum.

As a result, vk::Device::texture_from_raw() becomes more generic and can be used
more consistently in the wgpu-hal::vulkan implementation, which also fixes
inconsistent usage of the "textures" counter.
@s-ol s-ol force-pushed the vulkan-texture-unify-memory branch from 9591f6e to 30e0083 Compare November 30, 2025 10:51
@s-ol
Copy link
Contributor Author

s-ol commented Nov 30, 2025

Thanks for pointing that out, I'm still getting the hang of the contribution / CI flows here and the rust linting toolchain 🙂

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants