Don't render children of entities with is_visible set to false

[ambeeeeee]

Created in response to #838, would likely need to be manually merged with #861 due to changing the structure of the file that PR modifies.

[ambeeeeee]

Currently looking into leveraging Changed<T> in the visible checking system.

[CleanCut]

Since I was doing this manually here and here I thought I'd give this a try (you need to connect at least one gamepad if you want to try it out). Unfortunately, it doesn't seem to work for me!

I did a quick read-through of the PR, and nothing jumps out as obviously wrong to me, but I haven't looked closely yet.

[CleanCut]

LOL. Nevermind. It was me. I changed the bevy source just before I stashed my local changes, so my bevy wasn't pointing to this PR during my testing. Once I point to the right place, it works great.

I read through the PR, and it all seems correct to me. It seems a bit verbose, especially in the number of arguments to the recursive functions...but I couldn't think of a decent way to simplify it. Perhaps someone else will see a way to simplify it.

[ambeeeeee]

Since I was doing this manually here and here I thought I'd give this a try (you need to connect at least one gamepad if you want to try it out). Unfortunately, it doesn't seem to work for me!

I did a quick read-through of the PR, and nothing jumps out as obviously wrong to me, but I haven't looked closely yet.

A big goal for me with this PR is not to modify the is_visible of the children, which you obviously had to do, due to the feature not existing in bevy itself. I'm happy with how it came out and I hope it's satisfactory for cart :)

[AlexCMueller]

I don't have a good understanding of the engine's internals so I don't know how feasible my first thought of approaching this would be, but I notice that this solution requires some recursion while mine wouldn't if it works: would it be possible to, for each entity, determine if it should be drawn by searching up through the parents until it either reaches the root entity or finds an entity with is_visible set to false?

something along the lines of

let mut is_visible = true;

while let Some(parent) = entity.get_component::<Parent>() {
    if let Some(draw) = parent.get_component::<Draw>() {
        if !draw.is_visible {
            is_visible = false;
            break;
        }
    }
}

if is_visible {
    // drawing code here
}

does this have performance issues relative to amber's solution that I'm not seeing? unexpected behaviors regarding entities that should be drawn but aren't? etc...

EDIT: very big performance issue, as written this is an infinite loop...

[ambeeeeee]

Searching up through parents would most likely be less performant in general.

I based my solution off of the transform_propagate_system, so barring any horrific performance issues I didn't notice, I believe it would be acceptable due to previous use of recursion within bevy. Additionally, when I hit an entity with is_visible set to false I stop iterating on its children since it no longer matters what their values are.

[AlexCMueller]

I'm starting to see the advantages of a recursive parent-to-children approach, especially in my specific use case where I have a single root node holding potentially hundreds of children sprites. Thanks for taking on this issue!

[cart]

Just pushed a slight refactor that reduces the overall size/complexity of the implementation. In general this looks good, but we do have a problem that will block merging: we now only draw hierarchies like this:

e1 (has Draw) -> e2 (has Draw)  -> e3 (has Draw) 

We no longer draw cases like these:

// e3 is not drawn
e1 (has Draw) -> e2 (no Draw)  -> e3 (has Draw) 

// e2 and e3 are not drawn
e1 (no Draw) -> e2 (has Draw)  -> e3 (has Draw)

This breaks the assumptions we make when loading GLTF files as scenes (note that the load_gltf.rs example no longer draws anything)

// e1 is generated from a GLTF node that isn't intended to be "drawn", only to transform its children
e1 (Transform, GlobalTransform) -> e2 (PbrComponents { Draw, Transform, GlobalTransform, ...})

We have options here:

1. I think my ideal behavior would be that we dont require a "Draw" chain and just implicitly treat entities without Draw as "visible" / still attempt to draw their children. Unfortunately if we use the approach in this pr, I think it would require removing the With<Draw> filter and walking the entire entity tree / checking component existence at each entity. The alternative is querying every drawn entity and then "walking up" the tree to determine if that entity should be drawn. A naive implementation would be extremely expensive and almost certainly worse than just visiting every entity in most cases. With memoization we could cut down on the compute cost, but with O(n) memory usage (where "n" is the number of entities in World).
2. In cases like the GLTF example, we could require that Draw components are inserted.

[ambeeeeee]

Through discussion on discord we have analyzed the issue and come to the conclusion it needs to be re-implemented. I'm closing this PR in favor of a future one coming in the next day or two from me, as this one's git history isn't entirely relevant anymore.

Relevant discord link.