Skip to content

Add debug cycle detection for insert_recursive and remove_recursive#19293

Open
theotherphil wants to merge 2 commits intobevyengine:mainfrom
theotherphil:cyclic
Open

Add debug cycle detection for insert_recursive and remove_recursive#19293
theotherphil wants to merge 2 commits intobevyengine:mainfrom
theotherphil:cyclic

Conversation

@theotherphil
Copy link
Copy Markdown
Contributor

Objective

Partially fixes #17465.

Solution

Add a CyclicTraversalDetector struct and manually thread it through calls to insert_recursive(_impl) and remove_recursive(_impl).

All of the cfg(debug_assertion)s are a bit ugly, and this approach would require more wiring anywhere else we wanted to add cycle detection. We could instead use a scoped resource that gets inserted into world at the start of the traversal and removed at the end, but before adding complexity I wanted to check with @bushrat011899 and/or @alice-i-cecile where else they want cycle detection to be supported (and whether this "scoped resource" approach would even be sensible and/or has prior art).

Testing

Added should_panic tests for a cyclical hierarchy.

@janhohenheim janhohenheim added A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 19, 2025
@janhohenheim
Copy link
Copy Markdown
Member

Do you have benchmarks for these checks?

@theotherphil
Copy link
Copy Markdown
Contributor Author

Do you have benchmarks for these checks?

I don't - how are these typically added in Bevy?

@janhohenheim
Copy link
Copy Markdown
Member

janhohenheim commented May 19, 2025

People usually post a screenshot of Tracy for the function in question :)

I don't think it's too important for this PR, so feel free to skip it if it sounds like a hassle.

@theotherphil
Copy link
Copy Markdown
Contributor Author

Ah, thanks! I would be surprised if this was a material overhead in debug builds but I'm happy to write something synthetic to check. Before doing so I'll wait until I have an implementation that @bushrat011899 is happy with.

@alice-i-cecile
Copy link
Copy Markdown
Member

I feel like it would be more broadly useful to create a cycle detector for arbitrary relations, and then document that in the relations docs. This isn't specific to insert / remove recursive, and the user can then just stick it in wherever they want.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 19, 2025
@theotherphil
Copy link
Copy Markdown
Contributor Author

What would the API for that look like - is this something you want to check per traversal, or something to optionally check for within the Relationship component hooks? (Or something else?)

@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented May 19, 2025

What would the API for that look like - is this something you want to check per traversal, or something to optionally check for within the Relationship component hooks? (Or something else?)

Register a error-returning lifecycle observer with a generic type on the App, which users can then cfg(debug_assertions) themselves.

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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a detect_cyclic_traversal debug feature to bevy_ecs

3 participants