Filter entities sent for deletion

[xiyuoh]

Currently when we select an entity to be deleted, it is sent as a Delete event to the handle_deletion_requests system and goes through a series of if conditions to filter out any entities that cannot/should not be deleted but processed in another manner. As we add in more plugins this system can become fairly bloated with various types of entities across the site editor.

This PR adds a series of BoxedSystems as a resource to the world that takes over some of the filtering work. The Delete events are read by the same handle_deletion_systems and goes through a vector of filter systems. If any entity should not be permanently deleted, e.g. temporarily removed or hidden, it can be filtered out in one of these systems. The set of entities left at the end of this filtering process will then be sent for deletion as usual.

An example of a BoxedSystem implementation is available here (updated) in relation to the scenarios PR #242. Here we demonstrate filtering out entities based on the current scenario using the BoxedSystem filter_instance_deletion.

[luca-della-vedova]

The struct fields are private by default and this resource has no public API so it won't be possible for users outside of this crate (or even file I believe?) to add their own deletion filters.

[luca-della-vedova]

It would be nice to get rid of the initialized field, a suggestion on the PR comment.

[luca-della-vedova]

Nice to see a small and simple PR 😅

The API needs some work or it won't be usable outside this crate, now given this struct:

#[derive(Default, Resource)]
pub struct DeletionFilter {
    boxed_systems: Vec<DeletionBox>,
    pending_delete: HashSet<Delete>,
}

I guess what you meant to do is allow users to access, or at least push to, the Vec<DeletionBox>, something like:

pub fn user_system(filters: ResMut<DeletionFilter>) {
    filters.boxed_systems.push(DeletionBox::new(custom_filter));
}

Now this would work (if we make the field pub) but I wonder if we can rework the API a bit to also simplify the struct and remove the initialized flag as well.

One way to do it could be to keep the fields private but add a public API to the DeletionBox system with at least a method to add a new filter (we could add a remove as well, since it seems BoxedSystem implements both Eq and Hash).

The struct could then contain a vector of DeletionBox objects that are pending initialization, we can iterate on those on every system run and after they are initialized add them to the actual boxed_systems vector. This means that whether they are initialized or not is encoded in where they are rather than a boolean flag that we need to keep track of. This would also allow simplifying the struct definition to have a single field, at which point we can make it even niftier by having it as a newtype and deriving Deref and DerefMut as such:

// Before: 
pub struct DeletionBox {
    system: BoxedSystem<HashSet<Entity>, HashSet<Entity>>,
    initialized: bool,
}
// After
#[derive(Deref, DerefMut)]
// Also will need to derive PartialEq / Eq if we want to allow removal
pub struct DeletionBox(BoxedSystem<HashSet<Entity>, HashSet<Entity>>);

For the DeletionFilter (probably bikeshedding but should be DeletionFilters since it contains a vector?) I was thinking something like:

pub struct DeletionFilters {
    boxed_systems: Vec<DeletionBox>,
    // `pending_delete` removed, doesn't seem to actually be needed as a resource field?
    pending_insertion: Vec<DeletionBox>,
    pending_removal: Vec<DeletionBox>,
}

impl DeletionFilters {
    pub fn insert(&mut self, filter: DeletionBox) // Add to pending_insertion Vec
    pub fn remove(&mut self, filter: DeletionBox) // Add to pending_removal Vec
}

Finally the system that actually does the work would:

• Iterate through all pending_insertion systems and initialize them.
• Iterate through all pending_removal systems and remove them.
• Itereate through all boxed_systems and carry the actual work.

[xiyuoh]

@luca-della-vedova Thanks for the suggestions! Restructuring the resource makes a lot of sense, I'll work on implementing that. With regards to pending_delete as a resource field, I found that I had to bring the read Delete events into the DeletionFilter resource, otherwise if I would be double borrowing world: once in the first line of handle_deletion_requests to access the resource, and a second time within the resource scope to access the event reader.

[luca-della-vedova]

@luca-della-vedova Thanks for the suggestions! Restructuring the resource makes a lot of sense, I'll work on implementing that. With regards to pending_delete as a resource field, I found that I had to bring the read Delete events into the DeletionFilter resource, otherwise if I would be double borrowing world: once in the first line of handle_deletion_requests to access the resource, and a second time within the resource scope to access the event reader.

Understood that you can't bring the Delete events in the scope to avoid double borrowing world, but maybe you can push them into a local variable instead of having a new field in a "global" struct, since it's only used in that function?

[xiyuoh]

Updated with the following changes:

• Restructured DeletionBox and DeletionFilters as suggested. This includes adding public API for inserting/running boxes, removing pending_delete from the resource, and simplifying the DeletionBox struct.
  I didn't add a removal API as Eq and Hash are implemented for Box but not System.
• DeletionBox systems have inputs/outputs of HashSet<Delete> instead of HashSet<Entity> - since we're using a local variable to hold the read events

[luca-della-vedova]

Can you give a quick look to the style (just run cargo fmt) and the web CI (I believe the command suggested by the error should work)?

[xiyuoh]

Thanks for reminding! Those should be fixed now

[luca-della-vedova]

Thanks for iterating!