Skip to content

Conversation

@andriyDev
Copy link
Contributor

Objective

Solution

  • Convert FromWorld impls to systems in RenderStartup.
  • Use a closure system for initializing ResourceManager to capture the cluster_buffer_slots.

Testing

  • Ran the meshlet example and it behaves the same as main.

@andriyDev andriyDev requested review from IceSentry, JMS55 and atlv24 July 20, 2025 01:28
@andriyDev andriyDev added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 20, 2025
Comment on lines 213 to 226
(
check_meshlet_features,
(
(
// This captures cluster_buffer_slots to create the resource manager.
move |mut commands: Commands, render_device: Res<RenderDevice>| {
commands.insert_resource(ResourceManager::new(
cluster_buffer_slots,
&render_device,
));
},
init_meshlet_pipelines,
)
.chain(),
init_meshlet_mesh_manager,
),
)
.chain(),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a pretty dreadful amount of nesting, can any of it be split into a let binding or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized I could split off the closure into a variable and I think this makes it much more tolerable. There's still the same amount of nesting, but I think it's easier to see what's happening.

@andriyDev andriyDev requested a review from atlv24 July 20, 2025 16:07
Comment on lines +217 to +227
.add_systems(
RenderStartup,
(
check_meshlet_features,
(
(init_resource_manager_system, init_meshlet_pipelines).chain(),
init_meshlet_mesh_manager,
),
)
.chain(),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

take this with an extremely large grain of salt, just wondering if something like this can help maybe? deliberately not a "suggestion"

            .add_systems(
                RenderStartup,
                (
                    (check_meshlet_features, init_meshlet_mesh_manager).chain(),
                    (init_resource_manager_system, init_meshlet_pipelines).chain()
                        .after(check_meshlet_features),
                )
            )

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 20, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 20, 2025
Merged via the queue into bevyengine:main with commit 8878083 Jul 20, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants