Wgpu-rs: Buffer ranges

[kvark]

Every place where a buffer is used, we need to provide a range. Currently, crate_bind_group requires the actual size to be passed, which is often inconvenient. The upstream spec allows to assume the whole size available, if nothing is provided. Also set_index_buffer and set_vertex_buffer now need the size (also optional).

There are many options here:

1. require both the offset and the size be meaningful
2. treat size=0 as the remaining part of the buffer. This may be a bit confusing if 0 is computed automatically and unintentionally.
3. turn size into an Option<BufferAddress>. This is what gfx-rs does. It has slightly weak semantics, but hands down convenient.
   3a. introduce enum Size { Exact(u32), Remainder } - this is stronger but less convenient
4. be generic over RangeBounds. This doesn't work well with create_bind_group since it has many buffers in it.

In addition, we should have a struct that encapsulates buffer + range to be used in all these entry points, e.g.

pub struct BufferRange<'a> {
  buffer: &'a Buffer,
  offset: BufferAddress,
  size: Option<BufferAddress>,
}

Somewhat relevant, Option<BufferAddress> takes 16 bytes, which is sad.

[aloucks]

Somewhat relevant, Option<BufferAddress> takes 16 bytes, which is sad.

Could use a NonZero variant to optimize it:

pub struct BufferRange<'a> {
  buffer: &'a Buffer,
  offset: BufferAddress,
  size: Option<NonZeroU64<BufferAddress>>,
}

pub const WHOLE_SIZE: Option<NonZeroU64<BufferAddress>> = None;

Or alternatively (which happens to match vulkan):

pub struct BufferRange<'a> {
  buffer: &'a Buffer,
  offset: BufferAddress,
  size: BufferAddress,
}

pub const WHOLE_SIZE: BufferAddress = !0;

The last example still uses a magic number but at least (1) it's named and (2) it's much more obvious to be "special" when the value is u64::MAX.

[aloucks]

One other observation - I've noticed that I end up having a lot of length as _ or length.try_into()?. I'm wondering if just using usize would be better for size and offset values and doing the check internally as a way to be more ergonomic. Are values > u32::MAX valid on 32bit platforms? DynamicOffset may be tricky but I _think_ most other places would be OK.

[kvark]

Both are good points!

WHOLE_SIZE seems to be weaker than having our own enum: you still need need to refer something in wgpu::, unless you just type !0, which is controversial...

Upstream spec defines offsets as u64, but we can totally do the conversion in wgpu-rs and expect usize in all these places. That would be a good ergonomic improvement.

On Mar 15, 2020, at 13:48, aloucks [e-mail] wrote:


One other observation - I've noticed that I end up having a lot of length as _ or length.try_into()?. I'm wondering if just using usize would be better for size and offset values and doing the check internally as a way to be more ergonomic. Are values > u32::MAX valid on 32bit platforms? DynamicOffset may be tricky but I think most other places would be OK.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[aloucks]

unless you just type !0, which is controversial

I guess I'm not sure how is !0 or u64::MAX is controversial when 0 wouldn't be?

you still need need to refer something in wgpu::,

I don't think this is a bad thing at all. A named constant describes the intent much better:

use wgpu::WHOLE_SIZE;

pass.set_vertex_buffer(index, &buffer, 0, WHOLE_SIZE);

[kvark]

I guess I'm not sure how is !0 or u64::MAX is controversial when 0 wouldn't be?

Yes, "0" is also controversial.

I don't think this is a bad thing at all. A named constant describes the intent much better:

What I'm trying to say is: if you are going to require the user to use a type from wgpu, you might as well go for a rich enum instead of a magical constant.

[lachlansneff]

Perhaps you can have the internal representation of a range be different from the user-exposed representation. The internal datastructures can store it in the most compact way, but the exposed api can use an enum or whatever is most convenient.

[paulkernfeld]

I'm going to take a swing at this. If I don't respond within a week, assume I got too confused and gave up! 😂

[kvark]

@paulkernfeld thank you, that would be wonderful! Are you going to do just an Option<> ? or something else?

[paulkernfeld]

My plan for starters is to begin with just this:

pub struct BufferRange<'a> {
  buffer: &'a Buffer,
  offset: BufferAddress,
  size: Option<BufferAddress>,
}

I was also thinking about something like:

impl<'a> BufferRange<'a> {
    /// If size is None, the range is taken to extend to the end of the buffer.
    pub fn new(buffer: &'a Buffer, offset: BufferAddress, size: Option<NonZeroBufferAddress>) -> Self {
        ...
    }
}

This would disallow the user from passing a size of 0.

[kvark]

@paulkernfeld I don't think we should try hard enough to prevent value of 0 specifically. There are many values that are invalid, e.g. everything above the remaining buffer space is invalid as well. There is only so much we can do by a more complicated type system.
There is a benefit of making it simple. Just have Option<BufferAddress> for the sizes, no need to add extra constructors, etc.

[paulkernfeld]

I just opened a WIP PR. A few design questions:

• How far down should this change be pushed? For example, is it worth using BufferRange inside of RenderPassInner?
• Is it worth to try to do map_read? The challenge there is that you can't just do .unwrap_or(0) (I think?) so we'd either need to make the wgpu-rs object aware of the size or we'd need to make upstream changes to treat 0 specially.
• I felt awkward about copy_buffer_to_buffer because it takes two buffers and only one size. Should we pass in two BufferRange objects and error if the sizes don't match?

[kvark]

How far down should this change be pushed? For example, is it worth using BufferRange inside of RenderPassInner?

I think we can keep offset and size as separate arguments in the RenderPassInner methods, for ergonomic reasons only.

Is it worth to try to do map_read?

Don't bother changing map_read for now, it's in flux.

I felt awkward about copy_buffer_to_buffer because it takes two buffers and only one size.

That shouldn't be affected either. We basically need to follow the upstream API. Size is not optional in there.

[paulkernfeld]

Great, that makes it easier! Then I think #302 is ready to be reviewed.

[paulkernfeld]

I think this can now be closed from PR #302

[aloucks]

Related https://github.com/gfx-rs/wgpu/issues/654