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

Synchronize removed components with the render world #15582

Merged
merged 16 commits into from
Oct 8, 2024

Conversation

kristoff3r
Copy link
Contributor

@kristoff3r kristoff3r commented Oct 1, 2024

Objective

Fixes #15560
Fixes (most of) #15570

Currently a lot of examples (and presumably some user code) depend on toggling certain render features by adding/removing a single component to an entity, e.g. SpotLight to toggle a light. Because of the retained render world this no longer works: Extract will add any new components, but when it is removed the entity persists unchanged in the render world.

Solution

Add SyncComponentPlugin<C: Component> that registers SyncToRenderWorld as a required component for C, and adds a component hook that will clear all components from the render world entity when C is removed. We add this plugin to ExtractComponentPlugin which fixes most instances of the problem. For custom extraction logic we can manually add SyncComponentPlugin for that component.

We also rename WorldSyncPlugin to SyncWorldPlugin so we start a naming convention like all the Extract plugins.

In this PR I also fixed a bunch of breakage related to the retained render world, stemming from old code that assumed that Entity would be the same in both worlds.

I found that using the RenderEntity wrapper instead of Entity in data structures when referring to render world entities makes intent much clearer, so I propose we make this an official pattern.

Testing

Run examples like

cargo run --features pbr_multi_layer_material_textures --example clearcoat
cargo run --example volumetric_fog

and see that they work, and that toggles work correctly. But really we should test every single example, as we might not even have caught all the breakage yet.


Migration Guide

The retained render world notes should be updated to explain this edge case and SyncComponentPlugin

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 1, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 1, 2024
@alice-i-cecile alice-i-cecile self-requested a review October 1, 2024 22:44
@kristoff3r
Copy link
Contributor Author

I'll add and update documentation if we decide this is the solution we want

@kristoff3r kristoff3r marked this pull request as ready for review October 2, 2024 20:49
@IceSentry IceSentry added the A-ECS Entities, components, systems, and events label Oct 3, 2024
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 3, 2024
Copy link
Contributor

@Trashtalk217 Trashtalk217 left a comment

Choose a reason for hiding this comment

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

Broadly in favour of this, but there's one big exception that would break this system. This is exceedingly rare, but it does need to be documented.

Say that we have two synced and extracted components A and B. That are located on the same entity.

| Main World                   | Render World               |
| -----------------------------|----------------------------|
| e1: (A, B, RenderEntity(e2)) | e2: (A, B, MainEntity(e1)) |

If you now remove A from e1, this will remove both A and B in the render world.

| Main World                   | Render World              |
| -----------------------------|---------------------------|
| e1: (B, RenderEntity(e2))    | e2: (MainEntity(e1))      |

This may not be what the user expects. And while this scenario may be very rare, it should be documented. You could solve this by counting the number of synced components a entity has (and storing it with SyncToRenderWorld), but that might be a step to far.

Also some documentation is still required for the sync_component plugin.

crates/bevy_pbr/src/render/mesh.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/render/mesh.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/extract_component.rs Show resolved Hide resolved
crates/bevy_render/src/gpu_readback.rs Show resolved Hide resolved
crates/bevy_render/src/world_sync.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Trashtalk217 Trashtalk217 left a comment

Choose a reason for hiding this comment

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

Last couple nits, after this it's ready to go.

crates/bevy_render/src/world_sync.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/world_sync.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/sync_component.rs Outdated Show resolved Hide resolved
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
@kristoff3r
Copy link
Contributor Author

There are still a few open questions:

  • Do we do the WorldSync to SyncWorld rename?
  • Do we want the SyncComponentPlugin to add the required component registration for SyncToRenderWorld component? If yes we should remove all the ones that have been added in the required components migrations. If no we should re-add the ones removed here. In any case we should find some place to document it.

Also it looks like some of the examples broke when I merged main, I'll have to debug that later.

@alice-i-cecile
Copy link
Member

Do we do the WorldSync to SyncWorld rename?

Yes.

Do we want the SyncComponentPlugin to add the required component registration for SyncToRenderWorld component? If yes we should remove all the ones that have been added in the required components migrations. If no we should re-add the ones removed here. In any case we should find some place to document it.

Yes. Document this on SyncComponentPlugin, and also on the SyncToRenderWorld component please :)

@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 Oct 8, 2024
@kristoff3r
Copy link
Contributor Author

@alice-i-cecile seems to work now, tested with a bunch of different examples. It has a tendency to break because of the required component migrations, so I hope we can get it in soon.

@kristoff3r kristoff3r added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 8, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 8, 2024
@alice-i-cecile alice-i-cecile 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 8, 2024
Merged via the queue into bevyengine:main with commit 2d1b493 Oct 8, 2024
28 checks passed
@kristoff3r kristoff3r deleted the render-world-sync branch October 13, 2024 18:13
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-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Directional light does not disappear after removing the DirectionalLight component
4 participants