Merge Draw function and RenderCommand #7172
Conversation
alice-i-cecile
added
A-Rendering
kurtkuehnert
force-pushed
the
merge_draw_function_and_render_command
branch
from
372d415
to
570d23c
Compare
|
I have updated the documentation and rebased it onto main. This PR should now be ready for review. |
| let draw_function = draw_functions.get_mut(item.draw_function()).unwrap(); | ||
| draw_function.draw(world, render_pass, view, item); | ||
| } | ||
| let render_commands = world.resource::<RenderCommands<P>>(); |
There was a problem hiding this comment.
Curious if we could use &mut World and World::resource_scope here. It would allow us to remove the internal RwLock on RenderCommands.
This is probably at odds with parallelizing the render phase (i.e. with wgpu's RenderBundles), so feel free to ignore this.
There was a problem hiding this comment.
I am not too familiar with the ECS internals, but could we move the RenderCommandState::prepare logic out of the render graph? Maybe into the actual RenderStage::Prepare?
It seems like the only thing inside the RenderCommandState::render method that requires a mutable reference to self is the line: let param = self.state.get(world);.
This seems linked to #7084. But there the line: let param = self.state.get_manual(world);, still requires a mutable reference to self. Would it be possible to change that or is SystemState fundamentally different from QueryState in this regard?
Given that we could make the render method immutable and run the prepare method before the render graph, then we could eliminate the need for the RwLock, right?
kurtkuehnert
force-pushed
the
merge_draw_function_and_render_command
branch
2 times, most recently
from
db49394
to
08edef5
Compare
kurtkuehnert
force-pushed
the
merge_draw_function_and_render_command
branch
from
08edef5
to
bc66cac
Compare
kurtkuehnert
force-pushed
the
merge_draw_function_and_render_command
branch
from
bc66cac
to
8101e7c
Compare
kurtkuehnert
force-pushed
the
merge_draw_function_and_render_command
branch
from
8101e7c
to
5150e68
Compare
| /// When fetching resources, note that, due to lifetime limitations of the `Deref` trait, | ||
| /// [`SRes::into_inner`] must be called on each [`SRes`] reference in the | ||
| /// [`RenderCommand::render`] method, instead of being automatically dereferenced as is the | ||
| /// case in normal `systems`. | ||
| /// | ||
| /// All parameters have to be read only. | ||
| /// | ||
| /// [`SRes`]: bevy_ecs::system::lifetimeless::SRes | ||
| /// [`SRes::into_inner`]: bevy_ecs::system::lifetimeless::SRes::into_inner |
There was a problem hiding this comment.
This should not be removed.
Objective
Currently, you can choose between a
Drawfunction and aRenderCommandto render yourPhaseItems.Inspired by the recent merge of
PhaseItemandEntityPhaseItem(#6885), this PR does the same forDrawandRenderCommand.Are there use cases that are not covered by the RenderCommand abstraction?
This change aims to reduce the (irrelevant) decisions our users have to make and simplify our rendering terminology.
I believe that this is now much simpler: A
RenderPhaserenderers multipleRenderItemsvia the composableRenderCommands, by changing the state of theTrackedRenderPass.We can still decide how we name this
RenderCommand/DrawFunction, or maybe evenDrawCommand.Let me know which name you like best.
Solution
Changelog
Todo
Migration Guide
Todo