Repository navigation
In copying to/from texture, use given aspect instead of hardcoded 'all' - #851
almarklein wants to merge 2 commits into
Conversation
| mipLevel=int(destination.get("mip_level", 0)), | ||
| origin=c_origin, | ||
| aspect=enums.TextureAspect.all, | ||
| aspect=destination.get("aspect", "all"), |
There was a problem hiding this comment.
I was about to leave a commit preferring the enum instead of the magic value for default. But then I checked what would be better for performance and maybe it's neither. Instead it should be the magic int value... so maybe doing something like
| aspect=destination.get("aspect", "all"), | |
| aspect=destination.get("aspect", enummap["TextureAspect.all"]), |
but that feels awkward too. (skips a lot of the string manipulation in _new_struct_p, but maybe we should have the enum types in the backend specific map... or add a _to_int method?)
way off topic, perhaps just something to remember should we do a performance pass
There was a problem hiding this comment.
I like the general idea though. I now defined ASPECT_ALL = enummap["TextureAspect.all"] at the top of the module and use that instead. What do you think?
There was a problem hiding this comment.
You would end up with something like this for every single entry in the current mapping. So I am not sure that's the way to go. Maybe for a few specific hot cases. Which I don't think this is.
I think Vulkan does this, and it's horrible.
I have thought a bit on how it could be improved... But I need to actually try and measure that. For this specific case, just keep it as it is which will be fine.
There was a problem hiding this comment.
This'd only be done for the default enums/flags, right? So the number of them is not that big? Anyway, I guess this is the first one for which we try some approach, we can can easily change that once we/you come up with a better one 😄
There was a problem hiding this comment.
there is a point to be made that defaults are also handled by the c layer and once more by the rust implementation. so maybe we should not do it ourselfs (function parameter default, and then .get etc).
There was a problem hiding this comment.
The fields in the C structs cannot be omitted; omitted means zero, which usually is the default, but not sure if I want to rely on that.
There was a problem hiding this comment.
yeah, I remember the point with semantic null in a few places. There is a list of exceptions, but I am not sure if that is exhaustive... especially about the wgpu native only stuff: https://webgpu-native.github.io/webgpu-headers/SentinelValues.html
I would have to lookup the semantics of unwrap_or in rust, since they also have a !map_enum_with_undefined macro somewhere. As in wgpu types there is no undefined values in the enums. https://github.com/gfx-rs/wgpu-native/blob/589e6caa4e9fa5596f601e82ca5d3937a1afc829/src/conv.rs#L808
All of this needs some benchmarking or profiling to reason about. For the user experience being able to use both strings and enums like currently is great. And having readable default arguments is great for the IDE hover experience too. To come back to my original point, I prefer the enum types over magic strings but that is preference. I don't think there is a performance difference - since both end up doing some string manipulation and then the mapping lookup.
Using "undefined" as the default would technically be more correct, but that is less readable and might confuse the user, plus we aren't mapping that.
There was a problem hiding this comment.
Ok, so what approach do you prefer for the current PR? Let's get it approved and merged 😉
- The current approach is a bit more performant (but not sure if its measurable).
- Using
get('aspect', 'all')is more readable and avoids the lookup on the enum.
There was a problem hiding this comment.
the 2nd one is my preference. basically the original commit
No description provided.