[Merged by Bors] - Fix sparse insert #1748
Conversation
cart
added
C-Bug
|
Yup these (in combination) are the bug I mentioned in #1492. This resolves the issue. I actually opted to not remove the checks because it turns out that adding the extra commands when unneeded significantly cuts perf. I'm curious to see if thats the allocations, the dynamic dispatch, or the ops themselves. I expect its actually the allocations and not the ops (which is yet another incentive to optimize Command allocations). This does not appear to be the issue in #1743. Seems like they are attempting to insert a component into a non-existent entity. This should be an error as it has real implications for app state (unlike removing components from non-existent entities or despawning non-existent entities, which we have previously agreed should not be errors due to Command synchronization complexity). |
|
bors r+ |
Removing the checks on this line https://github.com/bevyengine/bevy/blob/main/crates/bevy_sprite/src/frustum_culling.rs#L64 and running the "many_sprites" example revealed two corner case bugs in bevy_ecs. The first, a simple and honest missed line introduced in #1471. The other, an insidious monster that has been there since the ECS v2 rewrite, just waiting for the time to strike: 1. #1471 accidentally removed the "insert" line for sparse set components with the "mutated" bundle state. Re-adding it fixes the problem. I did a slight refactor here to make the implementation simpler and remove a branch. 2. The other issue is nastier. ECS v2 added an "archetype graph". When determining what components were added/mutated during an archetype change, we read the FromBundle edge (which encodes this state) on the "new" archetype. The problem is that unlike "add edges" which are guaranteed to be unique for a given ("graph node", "bundle id") pair, FromBundle edges are not necessarily unique: ```rust // OLD_ARCHETYPE -> NEW_ARCHETYPE // [] -> [usize] e.insert(2usize); // [usize] -> [usize, i32] e.insert(1i32); // [usize, i32] -> [usize, i32] e.insert(1i32); // [usize, i32] -> [usize] e.remove::<i32>(); // [usize] -> [usize, i32] e.insert(1i32); ``` Note that the second `e.insert(1i32)` command has a different "archetype graph edge" than the first, but they both lead to the same "new archetype". The fix here is simple: just remove FromBundle edges because they are broken and store the information in the "add edges", which are guaranteed to be unique. FromBundle edges were added to cut down on the number of archetype accesses / make the archetype access patterns nicer. But benching this change resulted in no significant perf changes and the addition of get_2_mut() for archetypes resolves the access pattern issue.
|
Pull request successfully merged into main. Build succeeded: |
Removing the checks on this line https://github.com/bevyengine/bevy/blob/main/crates/bevy_sprite/src/frustum_culling.rs#L64 and running the "many_sprites" example revealed two corner case bugs in bevy_ecs. The first, a simple and honest missed line introduced in #1471. The other, an insidious monster that has been there since the ECS v2 rewrite, just waiting for the time to strike:
Note that the second
e.insert(1i32)command has a different "archetype graph edge" than the first, but they both lead to the same "new archetype".The fix here is simple: just remove FromBundle edges because they are broken and store the information in the "add edges", which are guaranteed to be unique.
FromBundle edges were added to cut down on the number of archetype accesses / make the archetype access patterns nicer. But benching this change resulted in no significant perf changes and the addition of get_2_mut() for archetypes resolves the access pattern issue.