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.

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

Reuse VisibleEntities in check_light_mesh_visibilty #13894

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

re0312
Copy link
Contributor

@re0312 re0312 commented Jun 17, 2024

Objective

Testing

cargo run --release --example many_cubes --features bevy/trace_tracy -- --shadows
~10% win in check_light_mesh_visibilty
image

@janhohenheim janhohenheim added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 17, 2024
Copy link
Contributor

@kristoff3r kristoff3r left a comment

Choose a reason for hiding this comment

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

Nice find, this wasn't caught in the original PR because both TypeIdMap and Vec have the clear / shrink_to functions and the code still compiled.

@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 Jun 17, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 17, 2024
Merged via the queue into bevyengine:main with commit 41ad4e9 Jun 17, 2024
33 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jul 14, 2024
…14073)

# Objective
- After #13894, I noticed the performance of `many_lights `dropped from
120+ to 60+. I reviewed the PR but couldn't identify any mistakes. After
profiling, I discovered that `Hashmap::Clone `was very slow when its not
empty, causing `extract_light` to increase from 3ms to 8ms.
- Lighting only checks visibility for 3D Meshes. We don't need to
maintain a TypeIdMap for this, as it not only impacts performance
negatively but also reduces ergonomics.

## Solution

- use VisibleMeshEntities for lighint visibility checking.


## Performance
cargo run --release --example many_lights  --features bevy/trace_tracy 
name="bevy_pbr::light::check_point_light_mesh_visibility"}

![image](https://github.com/bevyengine/bevy/assets/45868716/8bad061a-f936-45a0-9bb9-4fbdaceec08b)

system{name="bevy_pbr::render::light::extract_lights"}

![image](https://github.com/bevyengine/bevy/assets/45868716/ca75b46c-b4ad-45d3-8c8d-66442447b753)


## Migration Guide

> now `SpotLightBundle` , `CascadesVisibleEntities `and
`CubemapVisibleEntities `use VisibleMeshEntities instead of
`VisibleEntities`

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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-Performance A change motivated by improving speed, memory usage or compile times 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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants