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

Deprecate get_or_spawn #15652

Merged
merged 8 commits into from
Oct 7, 2024

Conversation

Trashtalk217
Copy link
Contributor

@Trashtalk217 Trashtalk217 commented Oct 4, 2024

Objective

After merging retained rendering world #15320, we now have a good way of creating a link between worlds (HIYAA intensifies). This means that get_or_spawn is no longer necessary for that function. Entity should be opaque as the warning above get_or_spawn says. This is also part of #15459.

I'm deprecating get_or_spawn_batch in a different PR in order to keep the PR small in size.

Solution

Deprecate get_or_spawn and replace it with get_entity in most contexts. If it's possible to query &RenderEntity, then the entity is synced and render_entity.id() is initialized in the render world.

Migration Guide

If you are given an Entity and you want to do something with it, use Commands.entity(...) or World.entity(...). If instead you want to spawn something use Commands.spawn(...) or World.spawn(...). If you are not sure if an entity exists, you can always use get_entity and match on the Option<...> that is returned.

@Trashtalk217 Trashtalk217 changed the title Deprecate get_or_spawn Deprecate get_or_spawn Oct 4, 2024
@Trashtalk217 Trashtalk217 added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Oct 4, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 4, 2024
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through labels Oct 4, 2024
@MrGVSV
Copy link
Member

MrGVSV commented Oct 4, 2024

I have no clue how networking code works, so ignore this comment if it's completely irrelevant.

Does removing this affect networking crates at all? My assumption would be that they might desire to pass entities over the wire and sync them one-to-one using something like get_or_spawn. Or do they typically just use a map to correlate entities (e.g. EntityMapper)?

@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Oct 5, 2024
@alice-i-cecile
Copy link
Member

Or do they typically just use a map to correlate entities (e.g. EntityMapper)?

This is the recommended pattern. Per @maniwani, since we don't have entity ID range reservation, relying on specific entity IDs is very fragile and ill-advised, because things like UI spawning will generating new entity IDs and clash with the networked entities.

Copy link
Contributor

@NiseVoid NiseVoid left a comment

Choose a reason for hiding this comment

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

Changes look fine to me, a bit odd that the transform hierarchy and prepare_cluster ones don't have expect and all the other ones do, but not really blocking imo.

@Trashtalk217
Copy link
Contributor Author

Changes look fine to me, a bit odd that the transform hierarchy and prepare_cluster ones don't have expect and all the other ones do, but not really blocking imo.

Those, I believe, don't use any of the sync machinery so I don't know what I would write in the expect clause.

@Shatur
Copy link
Contributor

Shatur commented Oct 6, 2024

Or do they typically just use a map to correlate entities (e.g. EntityMapper)

That's what we do in bevy_replicon, so we won't be affected.

@bushrat011899
Copy link
Contributor

Or do they typically just use a map to correlate entities (e.g. EntityMapper)

That's what we do in bevy_replicon, so we won't be affected.

Can also confirm that's how bevy_ggrs works!

@bushrat011899 bushrat011899 added 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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 6, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 7, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 7, 2024
Merged via the queue into bevyengine:main with commit d1bd46d Oct 7, 2024
26 checks passed
richchurcher added a commit to richchurcher/bevy_ecs_tilemap that referenced this pull request Oct 25, 2024
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants