Type safe retained render world

[tychedelia]

Objective

In the Render World, there are a number of collections that are derived from Main World entities and are used to drive rendering. The most notable are:

• VisibleEntities, which is generated in the check_visibility system and contains visible entities for a view.
• ExtractedInstances, which maps entity ids to asset ids.

In the old model, these collections were trivially kept in sync -- any extracted phase item could look itself up because the render entity id was guaranteed to always match the corresponding main world id.

After #15320, this became much more complicated, and was leading to a number of subtle bugs in the Render World. The main rendering systems, i.e. queue_material_meshes and queue_material2d_meshes, follow a similar pattern:

for visible_entity in visible_entities.iter::<With<Mesh2d>>() {
    let Some(mesh_instance) = render_mesh_instances.get_mut(visible_entity) else {
        continue;
    };
            
    // Look some more stuff up and specialize the pipeline...
            
    let bin_key = Opaque2dBinKey {
        pipeline: pipeline_id,
        draw_function: draw_opaque_2d,
        asset_id: mesh_instance.mesh_asset_id.into(),
        material_bind_group_id: material_2d.get_bind_group_id().0,
    };
    opaque_phase.add(
        bin_key,
        *visible_entity,
        BinnedRenderPhaseType::mesh(mesh_instance.automatic_batching),
    );
}

In this case, visible_entities and render_mesh_instances are both collections that are created and keyed by Main World entity ids, and so this lookup happens to work by coincidence. However, there is a major unintentional bug here: namely, because visible_entities is a collection of Main World ids, the phase item being queued is created with a Main World id rather than its correct Render World id.

This happens to not break mesh rendering because the render commands used for drawing meshes do not access the ItemQuery parameter, but demonstrates the confusion that is now possible: our UI phase items are correctly being queued with Render World ids while our meshes aren't.

Additionally, this makes it very easy and error prone to use the wrong entity id to look up things like assets. For example, if instead we ignored visibility checks and queued our meshes via a query, we'd have to be extra careful to use &MainEntity instead of the natural Entity.

Solution

Make all collections that are derived from Main World data use MainEntity as their key, to ensure type safety and avoid accidentally looking up data with the wrong entity id:

pub type MainEntityHashMap<V> = hashbrown::HashMap<MainEntity, V, EntityHash>;

Additionally, we make all PhaseItem be able to provide both their Main and Render World ids, to allow render phase implementors maximum flexibility as to what id should be used to look up data.

You can think of this like tracking at the type level whether something in the Render World should use it's "primary key", i.e. entity id, or needs to use a foreign key, i.e. MainEntity.

Testing

TODO:

This will require extensive testing to make sure things didn't break! Additionally, some extraction logic has become more complicated and needs to be checked for regressions.

Migration Guide

With the advent of the retained render world, collections that contain references to Entity that are extracted into the render world have been changed to contain MainEntity in order to prevent errors where a render world entity id is used to look up an item by accident. Custom rendering code may need to be changed to query for &MainEntity in order to look up the correct item from such a collection. Additionally, users who implement their own extraction logic for collections of main world entity should strongly consider extracting into a different collection that uses MainEntity as a key.

Additionally, render phases now require specifying both the Entity and MainEntity for a given PhaseItem. Custom render phases should ensure MainEntity is available when queuing a phase item.

[IceSentry]

The approach used in the PR make sense to me. It's a bit unfortuante that we need to do all that but I think it's the easiest fix for now.

LGTM once CI is green

[tychedelia]

Blocked on #15755

[BenjaminBrienen]

The blocker is fixed so after a merge and a migration guide this should be ready

[NiseVoid]

Would maybe be a bit nicer if it was harder to turn entities into MainEntity/RenderEntity incorrectly (trough .into()s) and getting typed a typed RenderEntity back in more places, but that's probably not worth blocking on since we probably want a nicer fix in the long term anyway.

[alice-i-cecile]

I like that this is substantially more error resistant. Thank you.