Add RenderComponent trait for transient render world marker components

[tychedelia]

Objective

Retained render world caused a number of things to break where previously we had relied on conditional extraction to toggle whether certain rendering features run or not. See the following issues and fixes:

• Conditional extraction to render world can cause desync #15871
• Remove components if not extracted #15948
• Attempt to remove component from render world if not extracted. #15904
• Fix deactivated camera still being used in render world #15946

In those fixes, we mostly attempted to restore the previous behavior by forcibly removing components from render world entities where conditional extraction previously would have caused them not to be inserted. While this fixes the superficial bugs, it's messy, error prone, and not how we want to model usage of retained render world to our users.

Solution

This pull request implements a new RenderComponent trait and corresponding derive/RenderComponentPlugin that is used to mark components as being transient markers, used for toggling render features on and off.

The idea here is that rather than relying on the presence of a component like Bloom or even ExtractedCamera to indicate that something should happen, we always try to use a marker component instead. These components are cleanup up at the end of rendering and must be inserted again the next frame during extract.

This makes some queries more complicated, since we have to remember to also add a query filter (see CameraActive, most notably), but should be less error prone overall and more clearly indicate when a certain feature is toggled on versus just having residual data.

This also allows us to mark more components as being generically extracted via ExtractComponent, which should make them easier to desync when removed from the main world, since ExtractComponentPlugin automatically inserts SyncComponentPlugin.

Testing

TODO:

• Retest all examples that broke in Conditional extraction to render world can cause desync #15871
• Perf test for regressions

Migration Guide

TODO

[alice-i-cecile]

Do you want to ship this in 0.15 or save it for 0.16?

[tychedelia]

@alice-i-cecile I've marked it contentious and put it in 0.15 for now. I discussed this general approach with @superdump last night and I think there's a general feeling the state of the conditional extraction stuff in 0.15 is pretty undesirable. Things seem to be mostly not-broken now, so we can cut this if needed but I think having a more robust fix would be good.

[alice-i-cecile]

Yeah, I'm generally in favor of a more robust solution here too. The existing design is really dodgy, and I'd prefer to ship something better.

[tychedelia]

@akimakinai would appreciate your opinion here since you've looked most closely at the previous defects.

[EmileGr]

Not for this PR obviously, but could #13120 (and the related follow-up work there) make this a little cleaner in the future?

[tychedelia]

Not for this PR obviously, but could #13120 (and the related follow-up work there) make this a little cleaner in the future?

Absolutely, that would be great.

[hymm]

I'm mostly concerned about the perf of this solution. Requiring 2+ archetype moves for every rendered entity every frame is probably pretty expensive.

[tychedelia]

I'm mostly concerned about the perf of this solution. Requiring 2+ archetype moves for every rendered entity every frame is probably pretty expensive.

Yup, that was our concern too, although just to be clear, it's not for rendered entities which are stored in resources already, but for things like views/lights. So typically this would only be a handful of entities.

[JMS55]

I'd rather removing the components from the render world to match than this solution, personally.

[tychedelia]

I'd rather removing the components from the render world to match than this solution, personally.

I'm fine with exploring a his design space in 0.16 but the ad hoc removal of components is messy, error prone, and conceptually confusing given our switch to retained entities. Minimally, if the marker components are too noisy, we should still mark per-frame components for removal somehow rather than manually removing them.

[JMS55]

I haven't really followed any of this, but iirc the current way it works is that when a component marked for syncing to the render world is removed from the main world, we have some system that also removes it from the render world when syncing right? That seems ideal to me, unless I'm missing something.

Again I haven't followed this work all that much, so maybe I'm wrong somewhere.

[alice-i-cecile]

I'm fine with exploring a his design space in 0.16 but the ad hoc removal of components is messy, error prone, and conceptually confusing given our switch to retained entities. Minimally, if the marker components are too noisy, we should still mark per-frame components for removal somehow rather than manually removing them.

Why not use TemporaryRenderEntity for this?

[tychedelia]

I'm fine with exploring a his design space in 0.16 but the ad hoc removal of components is messy, error prone, and conceptually confusing given our switch to retained entities. Minimally, if the marker components are too noisy, we should still mark per-frame components for removal somehow rather than manually removing them.

Why not use TemporaryRenderEntity for this?

Most of these are views and lights, which are entities we definitely are excited about being retained. The goal here is to mark certain components as temporary, not entire entities.

[tychedelia]

I don't think this is going to get review consensus so taking it out of 0.15.