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

QueueWriteBufferView is strange. #3134

Closed
botahamec opened this issue Oct 22, 2022 · 8 comments · Fixed by #3336
Closed

QueueWriteBufferView is strange. #3134

botahamec opened this issue Oct 22, 2022 · 8 comments · Fixed by #3336
Labels
type: question Further information is requested

Comments

@botahamec
Copy link
Contributor

According to the documentation for Queue::write_buffer_with:

The returned value can be dereferenced to a &mut [u8]; dereferencing it to a &[u8] panics!

According to the documentation for Deref:

this trait should never fail. Failure during dereferencing can be extremely confusing when Deref is invoked implicitly.

@cwfitzgerald cwfitzgerald added the type: question Further information is requested label Oct 23, 2022
@teoxoy
Copy link
Member

teoxoy commented Dec 5, 2022

Sadly there is no nice way to express this in rust since it's always valid to reborrow a &mut T as a & T.

@botahamec
Copy link
Contributor Author

@teoxoy There's no reason why it needs to implement DerefMut. Preferably there would be a safer interface that replicates only the features of an array which make sense to use.

@kpreid
Copy link
Contributor

kpreid commented Dec 5, 2022

Whatever interface is used, it would have to return an &mut [u8] eventually, because the point of this function is to allow ordinary Rust bulk-data operations to write into the GPU buffer rather than requiring a copy from elsewhere.

@botahamec
Copy link
Contributor Author

@kpreid Is the idea that there might be a method that modifies a &mut [u8] but doesn't read from it that someone might want to use for writing to the buffer? The better interface there would probably just to use a method that returns a &mut [u8]. But I must say that this seems like a weird thing to need.

@teoxoy
Copy link
Member

teoxoy commented Dec 5, 2022

Is the idea that there might be a method that modifies a &mut [u8] but doesn't read from it that someone might want to use for writing to the buffer?

Yes.

The better interface there would probably just to use a method that returns a &mut [u8].

It might be but there was precedent to go this route.

It was done to discourage reading since it will be slow to do so.

Thread on the PR where this was discussed previously: #2777 (comment)

@botahamec
Copy link
Contributor Author

What was the precedent?

I get that it's slow, but is it really worth it to crash on an implicit dereference? Especially when the docs for Deref say that it "must not fail", in bold. It's still possible to read from it anyway. If you really want to prevent reading, we could get something similar by just implementing AsMut<[u8]> but not AsRef<[u8]>.

@teoxoy
Copy link
Member

teoxoy commented Dec 5, 2022

What was the precedent?

BufferViewMut

I get that it's slow, but is it really worth it to crash on an implicit dereference? Especially when the docs for Deref say that it "must not fail", in bold. It's still possible to read from it anyway. If you really want to prevent reading, we could get something similar by just implementing AsMut<[u8]> but not AsRef<[u8]>.

That's fair, I just think we should also change BufferViewMut and maybe BufferView as well if we go this route.

@botahamec
Copy link
Contributor Author

I could probably implement that, at least for the QueueWriteBufferView.

BufferViewMut might need some more brainstorming, as it is sometimes readable, but it shouldn't be too bad. The docs for AsRef have a pretty clear recommendation for what to do if it can fail, which is to not implement it, and instead just make your own as_ref which returns an Option or a Result.

BufferView already just implements Deref and AsRef, so as far as I can tell, it should already be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants