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

Directional light does not disappear after removing the DirectionalLight component #15560

Closed
tim-blackbird opened this issue Oct 1, 2024 · 2 comments · Fixed by #15582
Closed
Labels
A-ECS Entities, components, systems, and events A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Milestone

Comments

@tim-blackbird
Copy link
Contributor

Removing the DirectionalLight component does not remove the directional light from the render world

Caused by #15320

Noticed in the clearcoat example

@tim-blackbird tim-blackbird added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Oct 1, 2024
@tim-blackbird tim-blackbird added this to the 0.15 milestone Oct 1, 2024
@tim-blackbird tim-blackbird changed the title Directional light does not dissapear after removing the DirectionalLight component Directional light does not disappear after removing the DirectionalLight component Oct 1, 2024
@kristoff3r
Copy link
Contributor

This seems like a general issue we need to solve with the retained render world that the PR/docs doesn't answer, who is responsible for removing components from the render world? Should that be done by the extract step, a separate system/plugin, or is it the users responsibility? A workaround is to remove SyncToRenderWorld and RenderEntity together with the component, but that seems like a footgun.

In any case we really need to fix and/or document it before 0.15.

@alice-i-cecile
Copy link
Member

[4:24 PM]kristoff3r: The OIT example is broken with the retained render world in the same way as #15560. How do we plan to support removing components like this?
[4:29 PM]IceSentry: 😦 oh no
[4:30 PM]IceSentry: @bird! @alice 🌹 ^
[4:32 PM]Alice 🌹: Hmm. This is more common than I expected
[4:33 PM]Alice 🌹: My medium-term plan was to do a ref-counting style design, where you have a "number of components that are causing this to be retained" tracked
[4:33 PM]Alice 🌹: And then hooks to incremenet and decrement it
[4:33 PM]Alice 🌹: I think we probably need to prioritize getting that in

From Discord.

@alice-i-cecile alice-i-cecile added the A-ECS Entities, components, systems, and events label Oct 1, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 8, 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`

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants