Remove Deref and DerefMut impls on Buffer#313
Merged
Conversation
madsmtm
force-pushed
the
madsmtm/remove-deref
branch
3 times, most recently
from
7723663 to
2944dc3
Compare
MarijnS95
reviewed
Jan 20, 2026
src/lib.rs
Outdated
| } | ||
|
|
||
| /// Pixel helper accessors. | ||
| /// Helper accessors for viewing the buffer's data as RGBA pixel data. |
Member
There was a problem hiding this comment.
"viewing" -> not sure if we have a better word to describe "accessing/writing" as you just discouraged "reading" in the comment above.
Member
Author
There was a problem hiding this comment.
You're right. I went with:
Helper methods for writing to the buffer as RGBA pixel data.
Which is not technically correct either, as the methods only provide &mut u32, they don't actually write, but eh, close enough.
Member
There was a problem hiding this comment.
Yeah that's the intended use for the mutable slice, close enough.
MarijnS95
approved these changes
Jan 20, 2026
To make it explicit when you're accessing the buffer data as pixels.
We want to migrate towards exposing either less structured buffers (`[u8]`, useful for different formats) or more structured buffers (`[Pixel]`, useful for ergonomics). `[u32]` is kind of in a weird state in between these and has confusing interactions with endianess. Since the current impls are not zero-cost on all backends, making them be a potential invisible performance cliff for users, we'd prefer for users to instead explicitly access the buffer's data in the format they desire.
Serves as documentation for why we don't expose different `pixels` and `pixels_mut` methods.
No longer really a necessary optimization, as accessing pixel data is an explicit operation, not invisible inside `Deref[Mut]`.
It is no longer used, and likely won't be necessary in the future either.
madsmtm
force-pushed
the
madsmtm/remove-deref
branch
from
2944dc3 to
5cba2e5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
&mut [u32]as a buffer format is kinda weird and has confusing interactions with endianess, see #109. I'd like to move towards exposing buffer data in different formats instead:Buffer::data(&mut self) -> &mut [u8], useful when working with different pixel formats, see Pixel format API design #98.Buffer::data(&mut self) -> &mut [Pixel]see AddPixelstruct andPixelFormatenum #289.Removing the
Deref/DerefMutas done in this PR should make it easier to reduce our "dependency" onu32as the pixel type in the future. Users would instead explicitly use the newBuffer::pixelsor the accessors added in #312 (which this PR builds upon).Finally, the current
Deref[Mut]impls are not zero-cost on all backends (and is kinda hard to make it so on Linux because ofBufferDispatch), making them be a potential invisible performance cliff.(The
BorrowStackhack could maybe be used to fix some of this, as was done on Wayland, but it requires the stored state to be&mut Tinstead ofT, and it adds 2*pointer size toBuffer<'_>which is kinda annoying. I've removed it in this PR because I believe it will no longer be necessary).