-
Notifications
You must be signed in to change notification settings - Fork 330
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
[re_renderer] Interior mutable buffer/texture/bindgroup pools #1374
Conversation
… resource directly. Respective pools can now allocate with a & reference, no mut required
0, | ||
index_buffer_size.try_into().unwrap(), | ||
) | ||
.write_buffer_with(&index_buffer, 0, index_buffer_size.try_into().unwrap()) | ||
.unwrap(); // Fails only if mapping is bigger than buffer size. |
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.
unrelated, but there is a lot of scary unwrap
s in this code
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.
all of those write_buffer_with
will soon be replaced with the staging belt, making them at least look less scary 🙃
state.alive_resources.retain(|_, resource| { | ||
let resolved = resource | ||
.take() | ||
.expect("Alive resources should never be None"); |
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.
then why use an Option
in alive_resources
?? O.o
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.
ah ok, it needs to be taken here so we can unwrap it.
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.
yeah that whole thing is a bit of an annoying workaround
@@ -56,10 +76,16 @@ where | |||
Handle: Key, | |||
Desc: Clone + Eq + Hash + Debug + SizedResourceDesc, | |||
{ | |||
pub fn alloc<F: FnOnce(&Desc) -> Res>(&mut self, desc: &Desc, creation_func: F) -> Arc<Handle> { | |||
pub fn alloc<F: FnOnce(&Desc) -> Res>( | |||
&self, |
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.
so alloc
is now thread-safe? that's very nice!
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.
There's a lot of scary unwrap
calls, but otherwise LGTM
Allows allocating buffers/textures/bindgroups without a
mut
reference to the pool.Moved
wgpu::Buffer
ownership from slotmap inside the pool to the strong handle itself, simplifying a lot of callsites.Some ripple effects on
DrawData
lifetime requirements and that in turn onto howViewBuilder
are composited - needed to do a workaround for the egui integration case.Checklist
CHANGELOG.md
(if this is a big enough change to warrant it)