From e2b8cc836b8f072858044b9fa5a007b0ba64c5b3 Mon Sep 17 00:00:00 2001
From: Asier Illarramendi <asier@illarra.com>
Date: Tue, 28 Mar 2023 22:18:02 +0200
Subject: [PATCH] Improve UI `update_clipping_system` comments (#8147)

# Objective

- Improve `update_clipping_system` comments

## Solution

- Add extra comments
- Reorder `CalculatedClip` updating `match` so it's reads `(old, new)`
vs previous `(new, old)`
- Remove `clone` on children iteration

---------

Co-authored-by: ickshonpe <27962798+ickshonpe@users.noreply.github.com>
---
 crates/bevy_ui/src/update.rs | 46 +++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/crates/bevy_ui/src/update.rs b/crates/bevy_ui/src/update.rs
index 623add4a6a..d7ed301e42 100644
--- a/crates/bevy_ui/src/update.rs
+++ b/crates/bevy_ui/src/update.rs
@@ -35,37 +35,51 @@ fn update_clipping(
     children_query: &Query<&Children>,
     node_query: &mut Query<(&Node, &GlobalTransform, &Style, Option<&mut CalculatedClip>)>,
     entity: Entity,
-    clip: Option<Rect>,
+    maybe_inherited_clip: Option<Rect>,
 ) {
-    let (node, global_transform, style, calculated_clip) = node_query.get_mut(entity).unwrap();
-    // Update this node's CalculatedClip component
-    match (clip, calculated_clip) {
+    let (node, global_transform, style, maybe_calculated_clip) =
+        node_query.get_mut(entity).unwrap();
+
+    // Update current node's CalculatedClip component
+    match (maybe_calculated_clip, maybe_inherited_clip) {
         (None, None) => {}
-        (None, Some(_)) => {
+        (Some(_), None) => {
             commands.entity(entity).remove::<CalculatedClip>();
         }
-        (Some(clip), None) => {
-            commands.entity(entity).insert(CalculatedClip { clip });
+        (None, Some(inherited_clip)) => {
+            commands.entity(entity).insert(CalculatedClip {
+                clip: inherited_clip,
+            });
         }
-        (Some(clip), Some(mut old_clip)) => {
-            if old_clip.clip != clip {
-                *old_clip = CalculatedClip { clip };
+        (Some(mut calculated_clip), Some(inherited_clip)) => {
+            if calculated_clip.clip != inherited_clip {
+                *calculated_clip = CalculatedClip {
+                    clip: inherited_clip,
+                };
             }
         }
     }
 
-    // Calculate new clip for its children
+    // Calculate new clip rectangle for children nodes
     let children_clip = match style.overflow {
-        Overflow::Visible => clip,
+        // When `Visible`, children might be visible even when they are outside
+        // the current node's boundaries. In this case they inherit the current
+        // node's parent clip. If an ancestor is set as `Hidden`, that clip will
+        // be used; otherwise this will be `None`.
+        Overflow::Visible => maybe_inherited_clip,
         Overflow::Hidden => {
-            let node_center = global_transform.translation().truncate();
-            let node_rect = Rect::from_center_size(node_center, node.calculated_size);
-            Some(clip.map_or(node_rect, |c| c.intersect(node_rect)))
+            let node_clip = node.logical_rect(global_transform);
+
+            // If `maybe_inherited_clip` is `Some`, use the intersection between
+            // current node's clip and the inherited clip. This handles the case
+            // of nested `Overflow::Hidden` nodes. If parent `clip` is not
+            // defined, use the current node's clip.
+            Some(maybe_inherited_clip.map_or(node_clip, |c| c.intersect(node_clip)))
         }
     };
 
     if let Ok(children) = children_query.get(entity) {
-        for child in children.iter().cloned() {
+        for &child in children {
             update_clipping(commands, children_query, node_query, child, children_clip);
         }
     }