Guard against VisibilityClass duplicates

[it-me-joda]

Objective

• When you clone_and_spawn a Mesh3D, you get a duplicate in the VisibilityClass Component. This is especially noticable with transaparent materials.
• Fixes VisibilityClass gets duplicated when cloning an entity, causing issues with transparency #20884

Solution

• To avoid this duplicate case, I added a simple guard check to avoid adding a duplicate record. According to the documentation, there is rarely a case where there is more than one VisibilityClass. That said, there is probably a better option to fix it, but I didn't want to destroy performance or make a massive change to the underlying data structures.

Testing

• There is some unit testing to cover this case
• This was visually tested as well as verifying using bevy_inspector_egui

Before the fix:

After the fix:

Sorry for the janky screenshots

[dloukadakis]

We can reuse the TypeId, we should probably have some warning when the entity does not have VisibilityClass too.

    let Some(mut visibility_class) = world.get_mut::<VisibilityClass>(entity) else {
        return;
    };

    let type_id = TypeId::of::<C>();
    if !visibility_class.contains(&type_id) {
        visibility_class.push(type_id);
    }

[it-me-joda]

I thought about doing it this way but didn't for some reason. If there were to be a warning, what would you expect it to tell you? It seems like the most likely scenario is that it gets automatically handled when someone adds a different component. I'm not sure in what scenario it would warn in the real world and if that would just confuse people.

[it-me-joda]

I updated the code match what you have. I think it's cleaner to do it this way but I'm not sure about the conventions of warning users so I'd like to get some feedback on that before making that call.

[dloukadakis]

I'm not sure in what scenario it would warn in the real world and if that would just confuse people.

It could happen in case someone used this hook but forgot to #[require(VisibilityClass)]

[dloukadakis]

Maybe something similar to:

bevy/crates/bevy_ecs/src/hierarchy.rs

Lines 463 to 472 in 1dfcde6

	warn!(
	"warning[B0004]: {}{name} with the {ty_name} component has a parent ({parent}) without {ty_name}.\n\
	This will cause inconsistent behaviors! See: https://bevy.org/learn/errors/b0004",
	caller.map(|c| format!("{c}: ")).unwrap_or_default(),
	ty_name = debug_name.shortname(),
	name = name.map_or_else(
	|| format!("Entity {entity}"),
	|s| format!("The {s} entity")
	),
	);

[dloukadakis]

I wrote a test for this, we can include it to make sure it works as expected:

    #[derive(Component, Default, Clone, Reflect)]
    #[require(VisibilityClass)]
    #[reflect(Component, Default, Clone)]
    #[component(on_add = add_visibility_class::<Self>)]
    struct TestVisibilityClassHook;

    #[test]
    fn test_add_visibility_class_hook() {
        let mut world = World::new();
        let entity = world.spawn(TestVisibilityClassHook).id();
        let entity_clone = world.spawn_empty().id();
        world
            .entity_mut(entity)
            .clone_with_opt_out(entity_clone, |_| {});

        let entity_visibility_class = world.entity(entity).get::<VisibilityClass>().unwrap();
        assert_eq!(entity_visibility_class.len(), 1);

        let entity_clone_visibility_class =
            world.entity(entity_clone).get::<VisibilityClass>().unwrap();
        assert_eq!(entity_clone_visibility_class.len(), 1);
    }

[it-me-joda]

I wrote a test for this, we can include it to make sure it works as expected:

    #[derive(Component, Default, Clone, Reflect)]
    #[require(VisibilityClass)]
    #[reflect(Component, Default, Clone)]
    #[component(on_add = add_visibility_class::<Self>)]
    struct TestVisibilityClassHook;

    #[test]
    fn test_add_visibility_class_hook() {
        let mut world = World::new();
        let entity = world.spawn(TestVisibilityClassHook).id();
        let entity_clone = world.spawn_empty().id();
        world
            .entity_mut(entity)
            .clone_with_opt_out(entity_clone, |_| {});

        let entity_visibility_class = world.entity(entity).get::<VisibilityClass>().unwrap();
        assert_eq!(entity_visibility_class.len(), 1);

        let entity_clone_visibility_class =
            world.entity(entity_clone).get::<VisibilityClass>().unwrap();
        assert_eq!(entity_clone_visibility_class.len(), 1);
    }

I added this test and also added you as a co-author. Thanks for the assists. I've been trying to get into Bevy contributions so it's nice to have the help. I'm still considering the warning situation and I'll report back when I make a decision for that.

[dloukadakis]

No worries, I wonder if failing loudly with a panic is more appropriate, because if this hook runs on an entity without a VisibilityClass component, we would be technically skipping adding the visibility class for that entity.

We could panic like this to include some debug information:

pub fn add_visibility_class<C>(
    mut world: DeferredWorld<'_>,
    HookContext { entity, caller, .. }: HookContext,
) where
    C: 'static,
{
    let Some(mut visibility_class) = world.get_mut::<VisibilityClass>(entity) else {
        let component_debug_name = DebugName::type_name::<C>();
        let visibility_class_debug_name = DebugName::type_name::<VisibilityClass>();
        panic!(
            "{}Entity {entity} with the {} component must have the {} component",
            caller.map(|c| format!("{c}: ")).unwrap_or_default(),
            component_debug_name.shortname(),
            visibility_class_debug_name.shortname(),
        );
    };

    let type_id = TypeId::of::<C>();
    if !visibility_class.contains(&type_id) {
        visibility_class.push(type_id);
    }
}

Also, even though it's out of scope for this pull request, it's worth noting that nothing currently removes the visibility class once it has been added. Maybe we should track this as a separate issue?

[tychedelia]

Hmm, I understand why this happens now, but it's a bit weird. Or, maybe feels like there needs to be a way for certain components to be ignored when cloning @alice-i-cecile.

Still, if we're going to assert uniqueness, can we remove the Deref and DerefMut derives on VisibilityClass and add an impl block (plus iter)? It's a bit more work and I don't think it's likely that anyone is interacting with it manually atm but let's enforce invariants outside of just the hook, if they exist.

[alice-i-cecile]

Or, maybe feels like there needs to be a way for certain components to be ignored when cloning @alice-i-cecile.

@eugineerd may have better opinions than me here. I know that you can blacklist components, but maybe we should have a way to configure defaults at the component level too.

[eugineerd]

Cloning does trigger on_add hooks currently, so ignoring it during cloning is probably the right approach. This can be done by setting clone behavior to Ignore:

#[derive(Clone, Component, Default, Reflect, Deref, DerefMut)]
#[reflect(Component, Default, Clone)]
#[component(clone_behavior=Ignore)]
pub struct VisibilityClass(pub SmallVec<[TypeId; 1]>);

[alice-i-cecile]

I think we should open a new PR with that approach. Thanks!

[it-me-joda]

Thanks everyone for taking a look at the PR. Can I apply this new approach in this PR? Or do we want to make an entirely new one?

[it-me-joda]

Disregard my last comment. I'm going to close this one and create a new PR with the agreed upon approach.