Skip to content

Refactor extraction to bypass the orphan rules#22766

Merged
alice-i-cecile merged 8 commits intobevyengine:mainfrom
kristoff3r:ks/extract_refactor
Feb 6, 2026
Merged

Refactor extraction to bypass the orphan rules#22766
alice-i-cecile merged 8 commits intobevyengine:mainfrom
kristoff3r:ks/extract_refactor

Conversation

@kristoff3r
Copy link
Contributor

@kristoff3r kristoff3r commented Feb 1, 2026

Objective

Fixes #18722, and allows ExtractComponent to be used for foreign types.

Solution

  • Split the Out type from ExtractComponent to a SyncComponent trait. This allows types to use the synchronization logic without the extraction logic, and allows SyncComponentPlugin to correctly identify which components should be removed.
  • Don't delete the entire entity but only the Out components in SyncComponentPlugin/SyncWorldPlugin, fixing Use of SyncComponentPlugin on ExtractComponentPlugin causes RenderEntity to be incorrectly despawned #18722.
  • Add marker types to ExtractComponent and SyncComponent, allowing them to be implemented for foreign types outside bevy_render. (Example: DirectionalLight is defined in bevy_light which doesn't depend on bevy_render, and used by bevy_pbr. Without the marker no crate is allowed to implement the trait.)

During some earlier render crate refactors by @atlv24, some uses of ExtractComponent was converted to manual implementations. I have not ported these back, that can be done in follow up PRs.

As a follow up it might be interesting to make a derive macro for SyncComponent, and/or update the ExtractComponent macro to be able to customize the behavior around syncing.

Testing

Ran a bunch of the examples. It would be good to test others, especially ones that toggle components.

A test case is in #22758. If that one gets merged first this PR should be updated to uncomment the relevant assert. edit: the assert has been added.

@kristoff3r kristoff3r added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 1, 2026
@github-project-automation github-project-automation bot moved this to Needs SME Triage in Rendering (2026 Proposal) Feb 1, 2026
@atlv24 atlv24 self-requested a review February 1, 2026 16:27
Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

this is pretty clever, needs a migration guide though

@kristoff3r kristoff3r requested a review from tychedelia February 1, 2026 16:46
Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

glad theres a way to clean this up and fix a bug too, excited to see this tucked away into a derive macro

@alice-i-cecile alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 6, 2026
@alice-i-cecile
Copy link
Member

#22758 is merging; can you update this and ping me?

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use 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 Feb 6, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 6, 2026
Merged via the queue into bevyengine:main with commit bbcc1e6 Feb 6, 2026
42 checks passed
@github-project-automation github-project-automation bot moved this to Done in Rendering Feb 6, 2026
@github-project-automation github-project-automation bot moved this from Needs SME Triage to Done in Rendering (2026 Proposal) Feb 6, 2026
@mockersf
Copy link
Member

mockersf commented Feb 7, 2026

Since this PR, some examples like light_probe_blending log a spam of

ERROR bevy_pbr::cluster: Clustered light or decal 4v0 had no assigned index!

which shouldn't happen according to the comment above the log

// This should never happen. The appropriate systems
// should have populated
// `global_clusterable_object_meta` by now.
error!(
"Clustered light or decal {:?} had no assigned index!",
entity
);

github-merge-queue bot pushed a commit that referenced this pull request Feb 7, 2026
# Objective

Fixes the following log spam (and wrong sync behavior) introduced by
#22766:

```
ERROR bevy_pbr::cluster: Clustered light or decal 5v0 had no assigned index!
```

## Solution

The components here are named in an interesting way. We have 3
components:
* `EnvironmentMapLight`
* `GeneratedEnvironmentMapLight`
* `RenderEnvironmentMap`

When I added the `SyncComponent` impl I pattern matched on the upper two
names, and assumed that `EnvironmentMapLight` was the main world
component and `GeneratedEnvironmentMapLight` was in the render world.

Actually `GeneratedEnvironmentMapLight` is a specific way to generate an
`EnvironmentMapLight`, and the render world entity is called
`RenderEnvironmentMap`. Fixing the `SyncComponent` impl to reflect that
fixes the issue.

I still feel like there are other issues after this fix:
* `GeneratedEnvironmentMapLight` could really use a better name
(`GenerateEnvironmentMapLight`? `RuntimeEnvironmentMapLight`?), and
better docs.
* I think it's still missing some syncing logic, as far as I can tell
nothing handles the generated `EnvironmentMapLight` if a
`GeneratedEnvironmentMapLight` is updated or deleted? I didn't do a deep
dive so I might have missed something.

## Testing

Tested with the `light_probe_blending` and `reflection_probes` examples.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use M-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

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Use of SyncComponentPlugin on ExtractComponentPlugin causes RenderEntity to be incorrectly despawned

4 participants