[Merged by Bors] - Replace `RemovedComponents<T>` backing with `Events<Entity>` #5680

Aceeri · 2022-08-14T00:02:32Z

Objective

Removal events are unwieldy and require some knowledge of when to put systems that need to catch events for them, it is very easy to end up missing one and end up with memory leak-ish issues where you don't clean up after yourself.

Solution

Consolidate removals with the benefits of Events<...> (such as double buffering and per system ticks for reading the events) and reduce the special casing of it, ideally I was hoping to move the removals to a Resource in the world, but that seems a bit more rough to implement/maintain because of double mutable borrowing issues.

This doesn't go the full length of change detection esque removal detection a la bevyengine/rfcs#44.
Just tries to make the current workflow a bit more user friendly so detecting removals isn't such a scheduling nightmare.

Changelog

RemovedComponents is now backed by an Events<Entity> for the benefits of double buffering.

Migration Guide

Add a mut for removed: RemovedComponents<T> since we are now modifying an event reader internally.
Iterating over removed components now requires &mut removed_components or removed_components.iter() instead of &removed_components.

Aceeri · 2022-08-15T23:15:46Z

I think re-thinking this I might just store the events in a RemovedComponents(SparseSet<ComponentId, Events<Entity>>);

Where RemovedComponent<T> would be a system param that would automatically fetch the RemovedComponents and component ids. This would make the api exactly the same as it is currently but with the benefits of unifying to the Events api and all that comes with that.

crates/bevy_ecs/src/world/entity_ref.rs

crates/bevy_ecs/src/removal_detection.rs

Aceeri · 2022-12-22T21:33:17Z

Reworked this a good bit to be less invasive of the ECS stuff, I would like #5735 to get merged before this because right now it isn't really possible for me to return empty iterators of components that haven't been removed yet.

crates/bevy_ecs/src/component.rs

JoJoJet · 2022-12-22T23:59:53Z

~~Replace RemovedComponent<T> with EventReader<RemovedComponent<T>> and iters will need to deref into the Entity since it will be a RemovedComponent<T> event instead of an Entity.~~

Why did you change course on this? This seems like a cleaner API, and I think I remember cart saying he preferred it.

Edit: it was Alice, not Cart.

Aceeri · 2022-12-23T03:07:12Z

Summary of why:
I can't name the Events<RemovedComponent<T>> type to send events on since I have to use a ComponentId to fetch it, which will only give me a type erased PtrMut. I could try registering closures or do something to make a proxy Events<Entity> in a resource, but those seem rather hacky.

JoJoJet

I think I'm on board with these changes. Having to add mut on the SystemParam is unfortunate, but IMO it's worth it for the consistency brought by double buffering.

crates/bevy_ecs/src/removal_detection.rs

crates/bevy_ecs/src/event.rs

crates/bevy_ecs/src/component.rs

crates/bevy_ecs/src/removal_detection.rs

alice-i-cecile

Thoughts:

ComponentIdFor is very interesting. @jakobhellermann may find it useful for working with dynamically typed APIs.
Oh right: Events used to live in bevy_ecs. That's a good explanation of why we didn't just use them originally.
3.I really like removing the "one frame" footgun, and reducing the tech debt of just using events.

Formatting Fix some docs Unnecessary imports impl From rather than Into for ToComponentId Try to fix CI errors Fix up test to expect double buffering Fix linnk in docs Simplify component id initializing/fetching Remove from system params Move component id for to component file Move ComponentIdFor to component.rs Deref and don't allow modifying the component id IntoIterator for RemovedComponents Formatting Example for ComponentIdFor Improve documentation MaybeIter inbetween Remove unnecessary type for iterator Improve safety comment, don't box unnecessarily formatting

…to removal detection file

alice-i-cecile · 2023-02-04T16:29:29Z

bors r+

bors · 2023-02-04T16:29:46Z

Merge conflict.

alice-i-cecile

bors r+

# Objective Removal events are unwieldy and require some knowledge of when to put systems that need to catch events for them, it is very easy to end up missing one and end up with memory leak-ish issues where you don't clean up after yourself. ## Solution Consolidate removals with the benefits of `Events<...>` (such as double buffering and per system ticks for reading the events) and reduce the special casing of it, ideally I was hoping to move the removals to a `Resource` in the world, but that seems a bit more rough to implement/maintain because of double mutable borrowing issues. This doesn't go the full length of change detection esque removal detection a la bevyengine/rfcs#44. Just tries to make the current workflow a bit more user friendly so detecting removals isn't such a scheduling nightmare. --- ## Changelog - RemovedComponents<T> is now backed by an `Events<Entity>` for the benefits of double buffering. ## Migration Guide - Add a `mut` for `removed: RemovedComponents<T>` since we are now modifying an event reader internally. - Iterating over removed components now requires `&mut removed_components` or `removed_components.iter()` instead of `&removed_components`.

bors · 2023-02-04T21:08:25Z

Pull request successfully merged into main.

Build succeeded:

Aceeri mentioned this pull request Aug 14, 2022

[Merged by Bors] - Windows as Entities #5589

Closed

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Aug 14, 2022

Aceeri marked this pull request as ready for review August 16, 2022 20:19

Aceeri commented Aug 16, 2022

View reviewed changes

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved

Aceeri changed the title ~~Replace RemovedComponents with an Event<RemovedComponent<T>>~~ Replace RemovedComponents<T> backing with Events<Entity> Aug 17, 2022

CGMossa reviewed Aug 17, 2022

View reviewed changes

crates/bevy_ecs/src/removal_detection.rs Outdated Show resolved Hide resolved

Aceeri force-pushed the removed-components-as-events branch from 82e2672 to 1561a1d Compare August 31, 2022 19:49

Aceeri force-pushed the removed-components-as-events branch 3 times, most recently from 235770c to 39d7af6 Compare December 22, 2022 21:29

JoJoJet reviewed Dec 22, 2022

View reviewed changes

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved

JoJoJet reviewed Dec 23, 2022

View reviewed changes

crates/bevy_ecs/src/removal_detection.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/component.rs Show resolved Hide resolved

JoJoJet reviewed Dec 23, 2022

View reviewed changes

crates/bevy_ecs/src/removal_detection.rs Outdated Show resolved Hide resolved

JoJoJet approved these changes Dec 24, 2022

View reviewed changes

crates/bevy_ecs/src/removal_detection.rs Show resolved Hide resolved

crates/bevy_ecs/src/removal_detection.rs Show resolved Hide resolved

Aceeri force-pushed the removed-components-as-events branch from 1ae0b76 to e71ada6 Compare December 25, 2022 02:46

Aceeri commented Dec 25, 2022

View reviewed changes

crates/bevy_ecs/src/removal_detection.rs Outdated Show resolved Hide resolved

Aceeri force-pushed the removed-components-as-events branch 3 times, most recently from 91cc714 to df404c2 Compare January 5, 2023 21:54

alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 30, 2023

alice-i-cecile added this to the 0.10 milestone Jan 30, 2023

Aceeri force-pushed the removed-components-as-events branch from 11a3ca7 to 95e5212 Compare January 30, 2023 20:52

alice-i-cecile approved these changes Jan 30, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 30, 2023

Aceeri force-pushed the removed-components-as-events branch from 95e5212 to 4d63994 Compare January 30, 2023 21:09

Aceeri added 3 commits January 30, 2023 13:12

Full path for phantomdata

8350f8b

Better docs for ComponentIdFor, another From impl for common usage of it

36469e1

Publicize the removed iter type with docs, move SystemParam impl for …

823e7c2

…to removal detection file

chrisjuchem mentioned this pull request Feb 4, 2023

Track Removed Comopnents chrisjuchem/bevy_mod_index#1

Merged

Aceeri added 2 commits February 4, 2023 12:26

Merge branch 'main' into removed-components-as-events

2389515

Add import for tests

3081a3a

alice-i-cecile approved these changes Feb 4, 2023

View reviewed changes

bors bot changed the title ~~Replace RemovedComponents<T> backing with Events<Entity>~~ [Merged by Bors] - Replace RemovedComponents<T> backing with Events<Entity> Feb 4, 2023

bors bot closed this Feb 4, 2023

Aceeri deleted the removed-components-as-events branch February 4, 2023 21:20

Shatur mentioned this pull request Feb 16, 2023

Add common component conditions #7711

Closed

chrisjuchem mentioned this pull request Jun 8, 2024

RemovedComponents can miss removals in 0.14.0-rc2 #13758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Replace `RemovedComponents<T>` backing with `Events<Entity>` #5680

[Merged by Bors] - Replace `RemovedComponents<T>` backing with `Events<Entity>` #5680

Aceeri commented Aug 14, 2022 •

edited

Loading

Aceeri commented Aug 15, 2022 •

edited

Loading

Aceeri commented Dec 22, 2022

JoJoJet commented Dec 22, 2022 •

edited

Loading

Aceeri commented Dec 23, 2022 •

edited

Loading

JoJoJet left a comment

alice-i-cecile left a comment

alice-i-cecile commented Feb 4, 2023

bors bot commented Feb 4, 2023

alice-i-cecile left a comment

bors bot commented Feb 4, 2023

[Merged by Bors] - Replace RemovedComponents<T> backing with Events<Entity> #5680

[Merged by Bors] - Replace RemovedComponents<T> backing with Events<Entity> #5680

Conversation

Aceeri commented Aug 14, 2022 • edited Loading

Objective

Solution

Changelog

Migration Guide

Aceeri commented Aug 15, 2022 • edited Loading

Aceeri commented Dec 22, 2022

JoJoJet commented Dec 22, 2022 • edited Loading

Aceeri commented Dec 23, 2022 • edited Loading

JoJoJet left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Feb 4, 2023

bors bot commented Feb 4, 2023

alice-i-cecile left a comment

Choose a reason for hiding this comment

bors bot commented Feb 4, 2023

[Merged by Bors] - Replace `RemovedComponents<T>` backing with `Events<Entity>` #5680

[Merged by Bors] - Replace `RemovedComponents<T>` backing with `Events<Entity>` #5680

Aceeri commented Aug 14, 2022 •

edited

Loading

Aceeri commented Aug 15, 2022 •

edited

Loading

JoJoJet commented Dec 22, 2022 •

edited

Loading

Aceeri commented Dec 23, 2022 •

edited

Loading