Added visibility inheritance

[YohDeadfall]

Fixes #838.

I'm not very happy that there's a new type called VisibleEffective, but currently it's much better way than making a new public field in Visible to keep an effective value and doesn't cause unnecessary breaking changes. Ideally, I would like to move is_transparent to materials as one comment says and make Visible non default and with constructors, but it's another story.

[alice-i-cecile]

I'd very much like to see if we can get this sort of behavior working nicely with relations when we rework the parent-child hierarchy (see #1627).

Per discussions on inheriting parent translation / rotation / scale, I'd be curious to see if we could use a pattern where we have a single ChildOf relation and then 4 different InheritsVisibility, InheritsTranslation etc marker components.

[YohDeadfall]

Hello, @alice-i-cecile! Not sure that any help needed here. A test failure caused by mine inattention is fixed already (:

[YohDeadfall]

For some reason the build fails while these part hasn't changed.

[mockersf]

updated lint in rust nightly, you can rebase on main now that #2179 has been merged

[YohDeadfall]

@cart, gentle reminder.

[cart]

This will end up being extremely expensive when constructing hierarchies (or when many nodes change on a given update). When all nodes have changed, it will walk each entity in the hierarchy's "subtree" (which is exponential). We need to do this in linear time for it to be workable (aka visit each node exactly once).

[YohDeadfall]

It's not obvious, but the code already handles that. The following lines check that the current entity as no parent or has an effective visibility already. Therefore, for a newly created subtree only the topmost node satisfies the condition.

[YohDeadfall]

Though on it a bit and what can be done here is a better filter for changes_query, so an entity will be included if:

• its visibility changed;
• its parent changed and it already has effective visibility.

[cart]

Changes aren't guaranteed to iterate in hierarchy-order, which means that if we were to fix this impl to propagate "effective visibility" and only visit each node once, it might not be correct if we propagate in the wrong order.

[YohDeadfall]

Exactly for that reason I do not update child entities which parent has no effective visibility. An advantage of the current implementation is that it completely avoids allocations to collect roots first. All spawned subtrees will be traversed only once.

[cart]

Some more assorted thoughts:

1. No use case should involve exponential explosions. And we must always be "correct". I'm relatively certain that the current impl can (or will) still violate both of those requirements, based on my understanding of how "tree propagation" works:
   • Propagating changes from subtrees in a "random order" risks doing exponential amounts of work. You can avoid this by memoizing to avoid re-propagating a subtree that was already handled, but this will result in potential for incorrect values if you don't propagate each "changed subtree" in hierarchical order.
   • Using VisibleEffective component existence to avoid re-doing work isn't a full solution because it only applies to "newly created hierarchies", not "mutated hierarchies".
   • Even if we solve the "re-doing work" problem for "mutated hierarchies", we still have the "value propagation correctness problem" unless we sort the "changed subtree entity list".
   • Sorting a change list in hierarchy-order is expensive (both due to the sort and the allocations). Whether or not this expense outweighs the cost of walking the entire tree and skipping unchanged things is context-specific. Its worth running benchmarks for various scenarios to compare the two, but I personally prefer the simpler / easier-to-get-right "full hierarchy traversal" approach used by the "transform propagation system" until we have those numbers.
2. This probablem is basically identical to the "transform propagation problem". I think the "right solution" for visibility propagation is almost certainly the "right solution" for transform propagation (the only major difference is that transform propagation is more expensive).
3. I'm not a big fan of VisibleEffective / the "derived and dynamically inserted" component pattern in general. I think something like the following api is preferable. I think is_transparent shouldn't live on the Visible component, so I'm cool with breaking that out into its own thing (and it is also likely to be removed entirely in the renderer rework).

   #[derive(Debug, Clone, Reflect)]
   #[reflect(Component)]
   pub struct Visibility {
       is_visible: bool,
       pub(crate) is_parent_visible: bool,
   }
   
   impl Default for Visibility {
       fn default() -> Self {
           Visibility {
               is_visible: true,
               is_parent_visible: true,
           }
       }
   }
   
   impl Visibility {
       pub fn new(is_visible: bool) -> Self {
           Visibility {
               is_visible,
               is_parent_visible: true,
           }
       }
   
       pub fn is_visible(&self) -> bool {
           self.is_visible
       }
   
       // name take from Godot. we can rethink this if needed. 
       pub fn is_visible_in_tree(&self) -> bool {
           self.is_visible && self.is_parent_visible
       }
   
       pub fn show(&mut self) {
           self.is_visible = true;
       }
   
       pub fn hide(&mut self) {
           self.is_visible = false;
       }
   }

[james7132]

Closing in favor of #5146 as this is much farther out of date and has a few performance issues noted.