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

Test for orphan parent-child relations #1278

Closed

Conversation

TheRawMeatball
Copy link
Member

@TheRawMeatball TheRawMeatball commented Jan 21, 2021

Currently, using Commands::despawn() leads to orphaned parent-child relations. These should be automatically cleaned up, so this PR adds a test case against such orphan relations.

@TheRawMeatball TheRawMeatball changed the title Clean up orphan parent-child relations Test for orphan parent-child relations Jan 21, 2021
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 21, 2021

(Apologies to @cart if this functionality already exists and we somehow missed it buried somewhere.)

At a high-level this shares a lot of the same challenges as indexes (#1205): you have more than one delocalized copy of the data.

However, because entities and components can only be removed at sync points (between stages), our freshness concerns are much lower. We can carefully control mutation via a limited API, so our only concern here is how to make sure orphaned components are also removed.

Linear search solution

The naive solution here would be to add a system that runs at the end of each stage. Scan through every copy of Parent and every copy of Child, and assert that the entity they point to exists. Remove all orphaned components.

We can speed this up a little bit by keeping a list of the entities that have had their component removed, but this is still linear time.

This works, but has bad complexity due to wasted work.

Cached solution

I think we could use Removed<Parent> (and its counterpart) to get a list of entities that had the given component removed (including via deletion of the entire entity?), and then use a cached copy of the parent-child relation from just before the component was removed to find the corresponding pair.

This requires us to update a cached copy of the component every time it's changed though, which seems like it may be frustrating and slow.

Index solution

If we had a working index solution, we could use it to cull the orphan relations by grabbing the list of entities with Removed, and then looking them up in our index.

This requires #1205 to be resolved though.

Notes

  1. I don't think we can use the Drop trait to ensure this work occurs, because it only gives access to &mut self.
  2. It would be nice to have a reusable solution for these types of components: defining relationships between entities is common and useful, but ensuring API hygiene and working cleanup is a pain. I've been chewing on an idea for "relational components" to generalize this pattern in the manner of compound keys, but that's a discussion for another place.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-ECS Entities, components, systems, and events A-UI Graphical user interfaces, styles, layouts, and widgets 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-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@TheRawMeatball
Copy link
Member Author

This problem will hopefully be solved eventually with the addition of relations (#1627), so this PR can be closed until then :)

@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label May 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants