TrackedRenderPass internal tracking state reset
#14948
Conversation
| /// Resets tracking state | ||
| pub fn reset_tracking(&mut self) { | ||
| self.pipeline = None; | ||
| self.bind_groups.iter_mut().for_each(|val| { |
There was a problem hiding this comment.
I'm not sure I understand why you set the values to None instead of just clearing the entire vec so it's emtpy.
There was a problem hiding this comment.
See previous comment: https://github.com/bevyengine/bevy/pull/14948/files#r1733639227
| @@ -95,6 +95,19 @@ impl DrawState { | |||
| ) -> bool { | |||
| self.index_buffer == Some((buffer, offset, index_format)) | |||
| } | |||
|
|
|||
| /// Resets tracking state | |||
| pub fn reset_tracking(&mut self) { | |||
There was a problem hiding this comment.
Couldn't this just do self = Default::default()
There was a problem hiding this comment.
It is important to keep vector lengths to device limits. Additionally, we want to avoid reallocating Vec heap memory.
See TrackedRenderPass::new
state: DrawState {
bind_groups: vec![(None, Vec::new()); max_bind_groups],
vertex_buffers: vec![None; max_vertex_buffers],
..default()
},
alice-i-cecile
added
A-Rendering
|
@fintelia could I get your review here? |
|
This change looks plausible to me, but I'm not familiar enough with the surrounding code at this point to have much confidence. The one suggestion I have is that it might make sense to have |
|
Applied @fintelia suggestion. Fixed conflicts. |
PPakalns
force-pushed
the
tracked_render_pass_state_pr
branch
from
11a6650
to
6a9c663
Compare
| @@ -24,6 +24,9 @@ struct DrawState { | |||
| /// List of vertex buffers by [`BufferId`], offset, and size. See [`DrawState::buffer_slice_key`] | |||
| vertex_buffers: Vec<Option<(BufferId, u64, u64)>>, | |||
| index_buffer: Option<(BufferId, u64, IndexFormat)>, | |||
|
|
|||
| /// Stores whether this state is populated or empty for quick state invalidation | |||
| stores_state: bool, | |||
There was a problem hiding this comment.
Bikeshed: this name confuses me, can we try one of
dirtychangedmodified
instead?
There was a problem hiding this comment.
I thought of multiple variants when I implemented this, but they do not improve the situation and didn't feel correct for me.
DrawState structure stores state.
changed: It doesn't store if state has changed between different points in time, it stores state information of RenderPass.modified: The same as withchanged.dirty: Stored state doesn't make this structure dirty (It implies something incorrect, not cleaned up). In reality normally this structure should store state, be populated.
That is the reason why I decided to use explicit naming of store_state with documentation comment. Maybe populated could be used, but explicit name store_state made sense for me.
Can change variable name if other reviewers suggest to do so.
|
Ping @IceSentry |
IceSentry
added
S-Ready-For-Final-Review
Objective
Fixes #13225
Solution
Invalidate
TrackedRenderPassinternal state upon accessing internalwgpu::RenderPass.Testing
set_bind_grouponRenderPassreturned byTrackedRenderPass::wgpu_passand checking if in laterset_bind_groupcalls onTrackedRenderPasscorrect bind group is restored.