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

Follow up on Retained Rendering #15459

Closed
Trashtalk217 opened this issue Sep 26, 2024 · 9 comments
Closed

Follow up on Retained Rendering #15459

Trashtalk217 opened this issue Sep 26, 2024 · 9 comments
Labels
A-ECS Entities, components, systems, and events A-Networking Sending data between clients, servers and machines A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes X-Contentious There are nontrivial implications that should be thought through
Milestone

Comments

@Trashtalk217
Copy link
Contributor

Trashtalk217 commented Sep 26, 2024

Merging #15320 leaves behind a couple of gaps that can be easily filled in a follow-up PR(s).

Implement WorldQuery for RenderEntity and MainEntity

When querying &RenderEntity currently from within the main world (or Extract systems in the render world), you constantly have to call .id() on every thing you query, because RenderEntity is a component that contains an entity. This indirection is annoying and makes the migration to 0.15 more cumbersome (more line changes), so we can implement a custom WorldQuery so that this happens automatically (think of querying Entity).

The same is true for MainEntity.

Using required components

There are a couple of places, mainly in bevy_render/src/extract_component.rs where SyncToRenderWorld is automatically added to certain components. This is currently done with observers, but it's much better to do this with required components. This is being tackled in #15582.

Deprecating get_or_spawn

get_or_spawn Is quite often used in extraction systems, given how things used to work. But with a retained rendering world, the entity should always already exist, so just get should suffice.

@Trashtalk217 Trashtalk217 added the S-Needs-Triage This issue needs to be labelled label Sep 26, 2024
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Blocked This cannot move forward until something else changes 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 and removed S-Needs-Triage This issue needs to be labelled labels Sep 26, 2024
@hymm
Copy link
Contributor

hymm commented Sep 26, 2024

We should probably also remove the in-engine uses of TemporaryRenderEntity if possible.

@Trashtalk217
Copy link
Contributor Author

Trashtalk217 commented Sep 26, 2024

We should probably also remove the in-engine uses of TemporaryRenderEntity if possible.

Yes, definitely a good idea. Although I expect the solutions for that to be more specific to the rendering contexts in which they are used.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Sep 26, 2024

Spawning a specific entity value is rarely the right choice. Most apps should favor Commands::spawn. This method should generally only be used for sharing entities across apps, and only when they have a scheme worked out to share an ID space (which doesn’t happen by default).

From get_or_spawn's docs. insert_or_spawn_batch has the same warning. Since we're no longer using this internally, we should deprecate this pattern completely as a footgun. Entity should be opaque, and we should not permit users to spawn entities with specific IDs in any way.

@alice-i-cecile alice-i-cecile added the A-Networking Sending data between clients, servers and machines label Sep 26, 2024
@JMS55 JMS55 added this to the 0.15 milestone Sep 29, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 7, 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.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@alice-i-cecile alice-i-cecile modified the milestones: 0.15, 0.16 Oct 8, 2024
@JMS55
Copy link
Contributor

JMS55 commented Oct 13, 2024

I think we've covered everything but #15704 now?

@chengts95
Copy link

Spawning a specific entity value is rarely the right choice. Most apps should favor Commands::spawn. This method should generally only be used for sharing entities across apps, and only when they have a scheme worked out to share an ID space (which doesn’t happen by default).

From get_or_spawn's docs. insert_or_spawn_batch has the same warning. Since we're no longer using this internally, we should deprecate this pattern completely as a footgun. Entity should be opaque, and we should not permit users to spawn entities with specific IDs in any way.

I have a strong motivation to use shared entity ids for multiple worlds. I have no idea why you think this is not needed and all other ECS can spawn entity with specific id. If entities' ids cannot be consistent when I save or load a world, it is a disaster. Please keep the feature available and do not deprecate it. Or you can tell me what is the alternatives.

@alice-i-cecile
Copy link
Member

I have a strong motivation to use shared entity ids for multiple worlds

Can you explain this use case? What are you ultimately trying to achieve, and why is this strategy important to do this? The EntityMapper trait is designed for the serialization / networking use cases, and has been how Bevy serializes and deserializes scenes since Bevy 0.1 as far as I'm aware.

@chengts95
Copy link

chengts95 commented Nov 29, 2024

I have a strong motivation to use shared entity ids for multiple worlds

Can you explain this use case? What are you ultimately trying to achieve, and why is this strategy important to do this? The EntityMapper trait is designed for the serialization / networking use cases, and has been how Bevy serializes and deserializes scenes since Bevy 0.1 as far as I'm aware.

  
    clone_entities(app.world(), app2.world_mut());
    reg.clone_world(app.world(), app2.world_mut());

I simply want to clone worlds, the worlds should have identical IDs for multiple algorithm designs, and I want native performance. The serialized and deserialized scenes are too heavy. Generally, bevy relies on reflection to serialize and deserialize, which is an overkill solution. Serialization is not an option for cloning a world for high-performance applications.

I have applications for power system simulation in Bevy. The ECS can become very successful in industrial applications that are not related to video games. Many algorithms such as Monte-Carlo simulation and N-1 stability verification require duplicating the data. In the design i have implemented, I only rely on the Clone trait to duplicate data. If the design permits, I will even choose to aggressively copy the archetype table as a whole and reconstruct the data in a new world.

@Trashtalk217
Copy link
Contributor Author

Trashtalk217 commented Nov 30, 2024

As a bit of extra context, because it's not explained in the warning. One of the issues with spawning arbitrary entities is the fact that under the hood, entity information is stored as a

Vec<EntityMeta>

see also https://docs.rs/bevy/0.14.2/bevy/ecs/entity/struct.Entities.html. This works well when your entity range is dense, but if you spawn in entity 1,000,000 out of the blue, you need to allocate a lot of memory for this one vector. This is a bit of a footgun.

Moreover, if you're using this for a save system where you just spawn in the entities by id, this is incredibly brittle when dealing with updating bevy / plugins. If a plugin decides to spawn in an extra entity, you can't load old saves anymore.

These are the primary reasons why we removed / are removing this feature.

EDIT: That's a fascinating usecase btw, I was busy writing while you commented.

@alice-i-cecile
Copy link
Member

I'm definitely interested in supporting that pattern! I've created #16559 to discuss how this can be best achieved; let's move this over there.

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-Networking Sending data between clients, servers and machines A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

No branches or pull requests

5 participants