search

bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.36k stars 3.49k forks source link

Follow up on Retained Rendering #15459

Open Trashtalk217 opened 1 day ago

Trashtalk217 commented 1 day ago

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 currently blocked by #15458, but there might be some other places where we can use required components as they exist currently.

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.

hymm commented 1 day ago

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

Trashtalk217 commented 1 day ago

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 commented 1 day ago

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.