-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove lifetime from BufferSlice, BufferView, BufferViewMut #8046
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using BufferSlice instead of &BufferSlice would be less diff, but less flexible (user would need to clone BufferSlice in some cases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing I really like about this change is that BufferView[Mut] (the one that gives access to mapped buffers, not just designating a part of a buffer) now has no lifetime, so it can be used in more ways. (But that could also be done on its own without modifying BufferSlice.)
The thing I don't like is that every render pass user has to add more &s, and their .slice() calls now perform a completely unnecessary reference count increment and decrement. Is there any chance it would be possible to arrange so that the set_vertex_buffer() etc. will actually take ownership of the slice's reference count and take advantage of that internally for their own resource tracking purposes? Or are there too many layers in the way? If this were possible, it would keep the syntax for users simpler and be a net performance improvement.
Signed-off-by: sagudev <[email protected]>
It might be possible, but that would be more involved as inner method takes &DispathcBuffer anyway.
I just check and I only need BufferView without lifetimes, so I went with this approach. |
Signed-off-by: sagudev <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay, finally!
This will fix some awful unsafe code I maintain in a different project! :)
Connections
Continuation of #7764
Related issue: #6974
Description
Currently, BufferView* is generic over a lifetime, but the lifetime is only used for a reference to a buffer. Since buffers are now Cloneable, it make sense to abandon this and instead have BufferView* "own" a Buffer.
This makes it easier to pass around BufferView*.
Testing
Tested via existing tests.
Squash or Rebase?
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.