Re-add Out to ExtractComponent#23334
Conversation
kristoff3r
left a comment
There was a problem hiding this comment.
This looks like a good approach, and roughly in line with what I was thinking of for future work when I did the SyncComponent trait.
A couple of things to consider:
- The type
Outwas named because it was the output type of the extract method, but we might be able to come up with a better name. I think of extraction as taking a component from the main world, based on that we directly produce some components in the render world, and then systems in the render world might additionally produce more. Maybe those can be called derived components? It would be nice to have some terminology for this and use it in the traits. - If we no longer use
OutinSyncComponent, we can either split the traits (i.e. haveExtractComponent<F = ()>: Componentinstead ofExtractComponent<F = ()>: SyncComponent<F>), or we can double down and haveSyncComponentuse bothOutandTargetfor removal. I kinda like the second option but I haven't thought too much about it yet.
Those aren't necessarily blockers for this as this is a decent incremental improvement, but we might as well try to improve things.
Also this will massively collide with #22852. We should make a decision about which one we want merged first.
|
Also fixed #23142 by removing both But there is still an issue with |
Definitely this one should merge first |
examples/3d/transmission.rs
Outdated
| if input.just_pressed(KeyCode::KeyH) { | ||
| if hdr { | ||
| commands.entity(camera_entity).remove::<Hdr>(); | ||
| commands.entity(camera_entity).remove::<(Bloom, Hdr)>(); |
There was a problem hiding this comment.
If I understand correctly, this does mean that the Out change doesn't really fix this issue. I'm in favor of this PR as a whole, but this is kinda papering over this specific problem imo. This is caused by required components, right?
There was a problem hiding this comment.
I think this issue is actually related to QueryFilter, not required component.
Bloom will be filtered out and not extracted if Hdr is removed, but the extracted BloomUniforms will still exist. The same applies to other ExtractComponents that use QueryFilter.
atlv24
left a comment
There was a problem hiding this comment.
id merge this without the bloom fix hack, and fix the bloom hdr problem with #23543 instead. But both is also good, I just want to make sure we look into the extract filter query problem later, as that is an actual issue that needs solving imo
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
Can you also update the migration guide that removed Out this cycle so that we're not claiming it was removed when we actually added it back please? |
Objective
Fixes #22863. Fixes #22999. Fixes #23142.
Solution
Outtype toExtractComponent. RenameSyncComponent::OuttoSyncComponent::Target, which is used to clean up components when removing, so that the output of extraction can be different fromSyncComponent::Target.#[extract_component_sync_target]attribute for#[derive(ExtractComponent)]to specifySyncComponent::Target.Testing
Tested
ssrandorder_independent_transparencyexamples.There may be other places where derived components need to be cleaned up, but finding them is somewhat challenging.