ByteAddressableBuffer: allow reading from read-only buffer

[Firestar99]

ByteAddressableBuffer currently has the major limitation of requiring a &mut [u32] even if it's only used for reading data. This PR should serve as a place for discussion of how to resolve this issue.

My currently proposed solution splits up ByteAddressableBuffer into the read-only variant ByteAddressableBuffer and the mutable variant MutByteAddressableBuffer. This is obviously a breaking change and the names are up for discussion.

I would also like to mention @eddyb's comment in https://github.com/EmbarkStudios/rust-gpu/pull/1014 about adding TypedBuffer:

TODO(@eddyb): add changelog, tests, discuss UntypedBuffer (TypedBuffer<[u32]> wrapper), maybe deprecate ByteAddressibleBuffer (to rename it to ByteAddressibleView or something? unsure what's best here)

[schell]

Does this have any effect on the shader side of things? It seems like a CPU-only change, as from my reading ByteAddressableBuffer is a type used to help collect and marshal data over to the GPU. My guess is that a shader wouldn't use this type directly?

[Firestar99]

Not at all. ByteAddressableBuffer is a shader-only thing and allows you to read and write arbitrary types at arbirary offsets from &[u32] slices, usually backed by storage buffers. Now you can't directly declare a ByteAddressableBuffer as a storage buffer (afaik), so you instead declare a &[u32] as a storage buffer and use the ByteAddressableBuffer to view it and do your reads/writes. Note however that you can only read and write structs with a minimum alignment of 4 bytes (both as struct alignment and offset), so technically it's a WordAddressableBuffer, but ByteAddressableBuffer is what HLSL calls them.

[schell]

Got it. So then I am interested in this for crabslab, though I'm not sure if crabslab could benefit from switching to using ByteAddressableBuffer. It currently uses a series of index_unchecked into the &[u32], driven by a derivable trait.

[schell]

Just spit-balling here, but if we're going to break backwards compatibility, we could define the type as:

pub struct ByteAddressableBuffer<T> {
    inner: T
}

impl<'a> ByteAddressableBuffer<&'a [u32]> {
    pub fn from_slice(s: &'a [u32]) -> Self {
        Self{inner: s}
    }

    pub fn load<T>(&self, byte_index: u32) -> T {
        todo!()    
    }
}

impl<'a> ByteAddressableBuffer<&'a mut [u32]> {
    pub fn from_mut_slice(s: &'a mut [u32]) -> Self {
        Self{inner: s}
    }
    pub fn store<T>(&mut self, byte_index: u32, t: T) {
        todo!()
    }
}

That would cut out the new type and make fixing the callsites a bit easier, but it also makes discovering what ByteAddressableBuffer does if you're uninitiated (like me)...

[Firestar99]

I've actually thought about such a design already, but didn't knew how to express it properly in rust. Got stuck on trying to express it as ByteAddressableBuffer<[u32]> and ByteAddressableBuffer<mut [u32]>, but using the reference and lifetime like that is really nice! The only disadvantage is duplicating the load and load_unchecked methods, but my suggestion is doing that too. Makes it unfortunately difficult to pass a ByteAddressableBuffer<&mut [u32]> into a method only needing to read, thus accepting only ByteAddressableBuffer<&[u32]>.

[schell]

@Firestar99 Yes, those functions would have to be defined in both impls. But it might be nice then to add an as_ref function to ByteAddressableBuffer<&mut [u32]> to make proxying to the non-mutable buffer easier:

pub struct ByteAddressableBuffer<T> {
    inner: T,
}

impl<'a> ByteAddressableBuffer<&'a [u32]> {
    pub fn from_slice(s: &'a [u32]) -> Self {
        Self { inner: s }
    }

    pub fn load<T>(&self, byte_index: u32) -> T {
        todo!()
    }
}

impl<'a> ByteAddressableBuffer<&'a mut [u32]> {
    pub fn from_mut_slice(s: &'a mut [u32]) -> Self {
        Self { inner: s }
    }

    pub fn store<T>(&mut self, byte_index: u32, t: T) {
        todo!()
    }

    /// Create a non-mutable `ByteAddressableBuffer` from this mutable one.
    pub fn as_ref(&self) -> ByteAddressableBuffer<&[u32]> {
        let buffer: ByteAddressableBuffer<&[u32]> = ByteAddressableBuffer { inner: self.inner };
        buffer
    }

    pub fn load<T>(&self, byte_index: u32) -> T {
        self.as_ref().load(byte_index)
    }
}

#[cfg(test)]
mod test_buffer {
    use super::*;

    #[test]
    fn buffer() {
        let mut slab = [0u32; 36];
        let mut buffer_mut = ByteAddressableBuffer::from_mut_slice(&mut slab);
        buffer_mut.store(0, Some(123u64));
        let _maybe_u64: Option<u64> = buffer_mut.load(0);
    }
}

It may be acceptable to add as_ref and then not define load and load_unchecked on the mutable buffer, since it's so easy to go from mutable to non-mutable?

[Firestar99]

Added :heart: This makes it pretty much feature complete, and ready for review. Changelog is already added as well.