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 texture property accessors that are missing to comply with webgpu.h #3740

Closed
wants to merge 4 commits into from

Conversation

eliemichel
Copy link
Contributor

I am looking at the way to implement missing bits of wgpu-native like wgpuTextureGetWidth as a way to get started with wgpu code base.

This PR is not ready yet, but I would love to get some feedback about the first steps so that I know whether I am going in the right direction!

@Wumpf
Copy link
Member

Wumpf commented Apr 30, 2023

Since wgpu-native only depends on wgpu-core, you shouldn't need to touch the wgpu crate itself. There, one can already access the previously passed in texture descriptor which has all the information. I.e. context.rs doesn't need extending :)

Having to do per wgpu-hal backend implementations seems unnecessarily complicated & repetetive, after all it seems that there's many many more properites that need querying: https://github.com/gfx-rs/wgpu-native/blob/trunk/src/unimplemented.rs#L245-L284
I haven't checked yet if there's a way to expose this from wgpu-core directly, but if there's not, maybe it's a better idea to store the descriptor like wgpu does in wgpu-native? Or alternatively move wgpu's descriptor storing to wgpu-core so that both wgpu and wgpu-native can access it directly

@eliemichel
Copy link
Contributor Author

Okey thanks for the feedback! This makes me understand better the overall organization of the code... and realize that the descriptor is already stored within the texture object, so the change becomes very simple :) Will add other simple getters.

@eliemichel
Copy link
Contributor Author

Here we are with all missing accessors! Some questions:

  • Was it worth defining a TextureAccessError?
  • Should I move texture_get_desc_then some place else? And is it correctly named?

Besides these, should be ready to merge.

@eliemichel eliemichel changed the title [WIP] Get texture size Add texture property accessors that are missing to comply with webgpu.h Apr 30, 2023
@cwfitzgerald
Copy link
Member

The reason we lifted the descriptor storing into wgpu is so that we didn't need to take any locks to get the descriptor. Once arcinization lands, we can discus unifying these, but for now, wgpu-native should deal with this entirely on its own by storing a copy of the texture descriptor.

@eliemichel
Copy link
Contributor Author

Okey, not sure I understand what the action should be for me in the end (sorry still new to this code base). Do you mean that I should not use texture.desc as I do currently because I should not use hub.textures.read just to read from the descriptor? If so, where is the correct data structure where to store the duplicate of the descriptor? The data/logic split between wgpu-native vs wgpu-core vs wgpu vs wgpu-hal is not so clear in my mind.

@rajveermalviya
Copy link
Contributor

This was implemented completely in wgpu-native side in gfx-rs/wgpu-native#265.

@Wumpf
Copy link
Member

Wumpf commented Jun 9, 2023

Nice, in that case we can close this off on this side

@Wumpf Wumpf closed this Jun 9, 2023
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.

4 participants