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

Add a doc note about despawn footgun #10889

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Conversation

benfrankel
Copy link
Contributor

Objective

The Despawn command breaks the hierarchy whenever you use it if the despawned entity has a parent or any children. This is a serious footgun because the Despawn command has the shortest name, the behavior is unexpected and not likely to be what you want, and the crash that it causes can be very difficult to track down.

Solution

Until this can be fixed by relations, add a note mentioning the footgun in the documentation.

@ItsDoot ItsDoot added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Dec 6, 2023
@Nilirad Nilirad added the A-Hierarchy Parent-child entity hierarchies label Dec 6, 2023
@Nilirad
Copy link
Contributor

Nilirad commented Dec 6, 2023

I'm not in favor of adding notes about other crates that are not dependencies. I would avoid being too specific and instead just note that despawning an entity won't have side effects on other entities (until we have relations).

If a user knows how to use add_child/push_children/etc, they should also know how to use despawn_recursive since they are both contained in the same plugin. Therefore I suggest to add specific references to despawn_recursive in those methods instead and keep it very general on bevy_ecs.

EDIT: Further thoughts: IMO despawn could be renamed to despawn_single, and despawn_recursive to despawn (maybe not both in the same release to avoid migration hell)

@benfrankel
Copy link
Contributor Author

benfrankel commented Dec 6, 2023

I'm not in favor of adding notes about other crates that are not dependencies. I would avoid being too specific and instead just note that despawning an entity won't have side effects on other entities (until we have relations).

If a user knows how to use add_child/push_children/etc, they should also know how to use despawn_recursive since they are both contained in the same plugin. Therefore I suggest to add specific references to despawn_recursive in those methods instead and keep it very general on bevy_ecs.

EDIT: Further thoughts: IMO despawn could be renamed to despawn_single, and despawn_recursive to despawn (maybe not both in the same release to avoid migration hell)

Makes sense. Renaming things can be in a different PR. I updated the notes to remove specific references to items in bevy_hierarchy, but still mentioning bevy_hierarchy itself as an example of what can go wrong.

Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Do you want to document despawn_recursive in children addition methods in bevy_hierarchy on this PR, or do you want to leave it as future work?

@benfrankel
Copy link
Contributor Author

LGTM.

Do you want to document despawn_recursive in children addition methods in bevy_hierarchy on this PR, or do you want to leave it as future work?

I'll leave it as future work.

@Nilirad Nilirad 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 Dec 7, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 12, 2023
Merged via the queue into bevyengine:main with commit 9c85769 Dec 12, 2023
23 checks passed
@Nilirad
Copy link
Contributor

Nilirad commented Dec 12, 2023

I furtherly addressed the despawn footgun in the bevy_hierarchy crate: #10951

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-Hierarchy Parent-child entity hierarchies C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants