Skip to content
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.

Sign up for GitHub

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

Move adding DynamicUniformIndex to Extract #5037

Closed
wants to merge 1 commit into from
Closed

Move adding DynamicUniformIndex to Extract #5037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/bevy_pbr/src/render/mesh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ pub fn extract_meshes(
entity,
(
handle.clone_weak(),
DynamicUniformIndex::<MeshUniform>::default(),
MeshUniform {
flags: if not_receiver.is_some() {
MeshFlags::empty().bits
Expand All @@ -174,6 +175,7 @@ pub fn extract_meshes(
entity,
(
mesh.clone_weak(),
DynamicUniformIndex::<MeshUniform>::default(),
MeshUniform {
flags: if not_receiver.is_some() {
MeshFlags::empty().bits
Expand Down
39 changes: 23 additions & 16 deletions crates/bevy_render/src/extract_component.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use bevy_ecs::{
use std::{marker::PhantomData, ops::Deref};

/// Stores the index of a uniform inside of [`ComponentUniforms`].
#[derive(Component)]
#[derive(Component, Copy)]
pub struct DynamicUniformIndex<C: Component> {
index: u32,
marker: PhantomData<C>,
Expand All @@ -28,6 +28,24 @@ impl<C: Component> DynamicUniformIndex<C> {
}
}

impl<C: Component> Default for DynamicUniformIndex<C> {
fn default() -> Self {
Self {
index: 0,
marker: PhantomData,
}
}
}
Comment on lines +31 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default impl also could be replaced by a derive.


impl<C: Component> Clone for DynamicUniformIndex<C> {
fn clone(&self) -> Self {
Self {
index: self.index,
marker: PhantomData,
}
}
}
Comment on lines +40 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the manual Clone necessary here?
It doesn't relax the C: Component bound and Copy is still derived.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PhantomData<T> only implements Clone iff T: Clone, which also transitively holds for the derived impl. This implements Clone and Default regardless of what T is.


/// Describes how a component gets extracted for rendering.
///
/// Therefore the component is transferred from the "app world" into the "render world"
Expand Down Expand Up @@ -100,28 +118,17 @@ impl<C: Component + ShaderType> Default for ComponentUniforms<C> {
/// This system prepares all components of the corresponding component type.
/// They are transformed into uniforms and stored in the [`ComponentUniforms`] resource.
fn prepare_uniform_components<C: Component>(
mut commands: Commands,
render_device: Res<RenderDevice>,
render_queue: Res<RenderQueue>,
mut component_uniforms: ResMut<ComponentUniforms<C>>,
components: Query<(Entity, &C)>,
mut components: Query<(&C, &mut DynamicUniformIndex<C>)>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this break the UniformComponentPlugin in the general case?
This now assumes that DynamicUniformIndex is added in the extract step, but that isn't the case for something using, say, ExtractComponentPlugin.

We aren't currently using this anywhere else, but given that this is intended to be a generalized (and user facing) abstraction, I think we should discuss ways to make this "fool proof".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've dropped the ball on this PR, but thinking on this a bit more. I think it makes a lot of sense to take an approach where we keep indices as components while directly writing extracted components to their target staging buffers.

Indices are small. 4-8 bytes typically. Compare this with the equivalent MeshUniform, which is 132 bytes currently. If we are going to heavily leverage commands for rendering, we should be minimizing the number of large copies that are being performed. I'd much rather us copy heavy components once and then just shuffle the indices around.

If we still need the intermediate data during Prepare or Queue, we can always refer back to the buffer in memory. It's less ergonomic, but alleviates the heaviest parts of running the Render World right now.

) where
C: ShaderType + WriteInto + Clone,
{
component_uniforms.uniforms.clear();
let entities = components
.iter()
.map(|(entity, component)| {
(
entity,
(DynamicUniformIndex::<C> {
index: component_uniforms.uniforms.push(component.clone()),
marker: PhantomData,
},),
)
})
.collect::<Vec<_>>();
commands.insert_or_spawn_batch(entities);
for (component, mut dynamic) in components.iter_mut() {
dynamic.index = component_uniforms.uniforms.push(component.clone());
}

component_uniforms
.uniforms
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_sprite/src/mesh2d/mesh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ pub fn extract_mesh2d(
entity,
(
Mesh2dHandle(handle.0.clone_weak()),
DynamicUniformIndex::<Mesh2dUniform>::default(),
Mesh2dUniform {
flags: MeshFlags::empty().bits,
transform,
Expand Down