gpu: fix getMappedRange size alignment #223

Merged

d3m1gd merged 2 commits from fix-mapped-range into main

2022-04-16 17:16:20 +00:00

d3m1gd commented

2022-04-16 07:18:44 +00:00

(Migrated from github.com)

same for getConstMappedRange
mirror align fix for getConstMappedRange

rationale:

user needs to align buffer on creation, as in

gpu.Buffer.Descriptor{ .size = size, ... }

so size == sizeOf(T) * len might not always hold true.

on getMappedRange request, there is no way to specify
that size was manually aligned, and using the function directly
with realigned size gives extremely unhelpful error:

thread 254201 panic: attempt to use null value
.../mach/gpu/src/NativeInstance.zig:1721:42: 0x480747
in .gpu.NativeInstance.buffer_vtable.getMappedRange (game)
        return @ptrCast([*c]u8, range.?)[0..size];
                                      ^

it seems, requested size must match size specified on creation exactly.

so this commit does exactly that. aligns buffer size to 4 - something
that user must have done already if this point is reached.

aligned buffer sizes stay unchanged.

the whole point is to reduce mental load for the user and prevent hacks
and workarounds for corner cases. original issue originated with
having u16 index buffer array with odd number of elements, which is a
perfectly valid thing to have.

it would be best to also auto align size on buffer creation to not force
user to know such obscure implementation details and learn magic numbers.

By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

+ same for getConstMappedRange + mirror align fix for getConstMappedRange rationale: user needs to align buffer on creation, as in gpu.Buffer.Descriptor{ .size = size, ... } so `size == sizeOf(T) * len` might not always hold true. on `getMappedRange` request, there is no way to specify that size was manually aligned, and using the function directly with realigned size gives extremely unhelpful error: thread 254201 panic: attempt to use null value .../mach/gpu/src/NativeInstance.zig:1721:42: 0x480747 in .gpu.NativeInstance.buffer_vtable.getMappedRange (game) return @ptrCast([*c]u8, range.?)[0..size]; ^ it seems, requested size must match size specified on creation exactly. so this commit does exactly that. aligns buffer size to 4 - something that user must have done already if this point is reached. aligned buffer sizes stay unchanged. the whole point is to reduce mental load for the user and prevent hacks and workarounds for corner cases. original issue originated with having u16 index buffer array with odd number of elements, which is a perfectly valid thing to have. it would be best to also auto align size on buffer creation to not force user to know such obscure implementation details and learn magic numbers. - [x] By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

d3m1gd commented

2022-04-16 09:32:05 +00:00

(Migrated from github.com)

technically speaking, buffer size must be at least 4, so might want to size.max(4) or something similar, but i'm not sure how practical this is - creating buffer with size less than 4.

technically speaking, buffer size must be at least 4, so might want to `size.max(4)` or something similar, but i'm not sure how practical this is - creating buffer with size less than 4.

d3m1gd commented

2022-04-16 09:34:23 +00:00

(Migrated from github.com)

also, 4 is a magic number, so might define it more officially. wgpu.rs defines it as a constant COPY_BUFFER_ALIGNMENT for the whole crate

also, 4 is a magic number, so might define it more officially. wgpu.rs defines it as a constant `COPY_BUFFER_ALIGNMENT` for the whole crate

emidoots commented

2022-04-16 17:21:13 +00:00

(Migrated from github.com)

Thank you @d3m1gd! This looks like a good improvement over the status quo today, and so I've immediately merged this.

I think that we may need to do more here to be completely correct, though:

COPY_BUFFER_ALIGNMENT is not exposed through https://github.com/webgpu-native/webgpu-headers/blob/main/webgpu.h and so this makes me wonder if 4 is always the right alignment or not.
There exist minUniformBufferOffsetAlignment and minStorageBufferOffsetAlignment, I'm not sure how they relate yet.
We should indeed handle all buffer sizes in some reasonable way.

I will dig into these further / reach out to the Dawn authors for clarification, unless you beat me to it :)

Thank you @d3m1gd! This looks like a good improvement over the status quo today, and so I've immediately merged this. I think that we may need to do more here to be completely correct, though: * `COPY_BUFFER_ALIGNMENT` is not exposed through https://github.com/webgpu-native/webgpu-headers/blob/main/webgpu.h and so this makes me wonder if `4` is always the right alignment or not. * There exist [`minUniformBufferOffsetAlignment` and `minStorageBufferOffsetAlignment`](https://github.com/webgpu-native/webgpu-headers/blob/b46ca3c39249c537b2b421a37cf06b0d20bbf2c1/webgpu.h#L753-L754), I'm not sure how they relate yet. * We should indeed handle all buffer sizes in some reasonable way. I will dig into these further / reach out to the Dawn authors for clarification, unless you beat me to it :)