clutter: Move assembling the redraw clip out of "queue-redraw" signal

Putting together the redraw clip of the stage never really fitted nicely
with the "queue-redraw" signal emission, it forces us to emit the
signals in a batch and we also use a weird trick to get the old paint
volume that's already on-screen into the final redraw clip (we call
_clutter_actor_propagate_queue_redraw() on the stage).

So start breaking up this association by making the stage explicitely
request the redraw clip from the actor and removing the
ClutterPaintVolume argument from _clutter_actor_finish_queue_redraw().
This is done by adding a private function
clutter_actor_get_redraw_clip() which returns our old (currently
visible) paint volume and the new paint volume.

This also allows removing the check whether a full stage redraw has been
queued in clutter_actor_real_queue_redraw() and we can now just stop the
signal emission if a propagation happened at least once.

Part-of: <https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1511>

clutter/clutter/clutter-actor-private.h

@@ -232,8 +232,7 @@ void                            _clutter_actor_queue_redraw_full
                                                                                          const ClutterPaintVolume *volume,
                                                                                          ClutterEffect            *effect);
 
-void                            _clutter_actor_finish_queue_redraw                      (ClutterActor       *self,
+void                            _clutter_actor_finish_queue_redraw                      (ClutterActor *self);
-                                                                                         ClutterPaintVolume *clip);
 
 gboolean                        _clutter_actor_set_default_paint_volume                 (ClutterActor       *self,
                                                                                          GType               check_gtype,
@@ -268,6 +267,10 @@ void clutter_actor_queue_immediate_relayout (ClutterActor *self);
 
 gboolean clutter_actor_is_painting_unmapped (ClutterActor *self);
 
+gboolean clutter_actor_get_redraw_clip (ClutterActor       *self,
+                                        ClutterPaintVolume *dst_old_pv,
+                                        ClutterPaintVolume *dst_new_pv);
+
 G_END_DECLS
 
 #endif /* __CLUTTER_ACTOR_PRIVATE_H__ */

clutter/clutter/clutter-actor.c

@@ -2602,9 +2602,8 @@ _clutter_actor_queue_redraw_on_clones (ClutterActor *self)
 }
 
 static void
-_clutter_actor_propagate_queue_redraw (ClutterActor       *self,
+_clutter_actor_propagate_queue_redraw (ClutterActor *self,
-                                       ClutterActor       *origin,
+                                       ClutterActor *origin)
-                                       ClutterPaintVolume *pv)
 {
   gboolean stop = FALSE;
 
@@ -2624,11 +2623,11 @@ _clutter_actor_propagate_queue_redraw (ClutterActor       *self,
       if (g_signal_has_handler_pending (self, actor_signals[QUEUE_REDRAW],
                                     0, TRUE))
         {
-          g_signal_emit (self, actor_signals[QUEUE_REDRAW], 0, origin, pv, &stop);
+          g_signal_emit (self, actor_signals[QUEUE_REDRAW], 0, origin, &stop);
         }
       else
         {
-          stop = CLUTTER_ACTOR_GET_CLASS (self)->queue_redraw (self, origin, pv);
+          stop = CLUTTER_ACTOR_GET_CLASS (self)->queue_redraw (self, origin);
         }
 
       if (stop)
@@ -2639,9 +2638,8 @@ _clutter_actor_propagate_queue_redraw (ClutterActor       *self,
 }
 
 static gboolean
-clutter_actor_real_queue_redraw (ClutterActor       *self,
+clutter_actor_real_queue_redraw (ClutterActor *self,
-                                 ClutterActor       *origin,
+                                 ClutterActor *origin)
-                                 ClutterPaintVolume *paint_volume)
 {
   CLUTTER_NOTE (PAINT, "Redraw queued on '%s' (from: '%s')",
                 _clutter_actor_get_debug_name (self),
@@ -2667,20 +2665,13 @@ clutter_actor_real_queue_redraw (ClutterActor       *self,
   if (!CLUTTER_ACTOR_IS_VISIBLE (self))
     return TRUE;
 
-  /* Although we could determine here that a full stage redraw
+  /* We guarantee that we will propagate a queue-redraw signal up
-   * has already been queued and immediately bail out, we actually
+   * the tree at least once so that it's possible to implement a
-   * guarantee that we will propagate a queue-redraw signal to our
-   * parent at least once so that it's possible to implement a
    * container that tracks which of its children have queued a
    * redraw.
    */
   if (self->priv->propagated_one_redraw)
-    {
+    return TRUE;
-      ClutterActor *stage = _clutter_actor_get_stage_internal (self);
-      if (stage != NULL &&
-          _clutter_stage_has_full_redraw_queued (CLUTTER_STAGE (stage)))
-        return TRUE;
-    }
 
   self->priv->propagated_one_redraw = TRUE;
 
@@ -7414,13 +7405,12 @@ clutter_actor_class_init (ClutterActorClass *klass)
 		  G_STRUCT_OFFSET (ClutterActorClass, queue_redraw),
                   g_signal_accumulator_true_handled,
 		  NULL,
-		  _clutter_marshal_BOOLEAN__OBJECT_BOXED,
+		  _clutter_marshal_BOOLEAN__OBJECT,
-		  G_TYPE_BOOLEAN, 2,
+		  G_TYPE_BOOLEAN, 1,
-                  CLUTTER_TYPE_ACTOR,
+                  CLUTTER_TYPE_ACTOR);
-                  CLUTTER_TYPE_PAINT_VOLUME);
   g_signal_set_va_marshaller (actor_signals[QUEUE_REDRAW],
                               G_TYPE_FROM_CLASS (object_class),
-                              _clutter_marshal_BOOLEAN__OBJECT_BOXEDv);
+                              _clutter_marshal_BOOLEAN__OBJECTv);
 
   /**
    * ClutterActor::queue-relayout:
@@ -8022,11 +8012,9 @@ clutter_actor_destroy (ClutterActor *self)
 }
 
 void
-_clutter_actor_finish_queue_redraw (ClutterActor *self,
+_clutter_actor_finish_queue_redraw (ClutterActor *self)
-                                    ClutterPaintVolume *clip)
 {
   ClutterActorPrivate *priv = self->priv;
-  ClutterPaintVolume *pv = NULL;
 
   /* Remove queue entry early in the process, otherwise a new
      queue_redraw() during signal handling could put back this
@@ -8036,39 +8024,7 @@ _clutter_actor_finish_queue_redraw (ClutterActor *self,
   */
   priv->queue_redraw_entry = NULL;
 
-  /* If we've been explicitly passed a clip volume then there's
+  _clutter_actor_propagate_queue_redraw (self, self);
-   * nothing more to calculate, but otherwise the only thing we know
-   * is that the change is constrained to the given actor.
-   *
-   * The idea is that if we know the paint volume for where the actor
-   * was last drawn (in eye coordinates) and we also have the paint
-   * volume for where it will be drawn next (in actor coordinates)
-   * then if we queue a redraw for both these volumes that will cover
-   * everything that needs to be redrawn to clear the old view and
-   * show the latest view of the actor.
-   *
-   * Don't clip this redraw if we don't know what position we had for
-   * the previous redraw since we don't know where to set the clip so
-   * it will clear the actor as it is currently.
-   */
-  if (clip)
-    {
-      pv = clip;
-    }
-  else if (G_LIKELY (priv->last_paint_volume_valid))
-    {
-      pv = _clutter_actor_get_paint_volume_mutable (self);
-      if (pv)
-        {
-          ClutterActor *stage = _clutter_actor_get_stage_internal (self);
-
-          /* make sure we redraw the actors old position... */
-          _clutter_actor_propagate_queue_redraw (stage, stage,
-                                                 &priv->last_paint_volume);
-        }
-    }
-
-  _clutter_actor_propagate_queue_redraw (self, self, pv);
 }
 
 void
@@ -19667,3 +19623,21 @@ clutter_actor_invalidate_transform (ClutterActor *self)
 
   transform_changed (self);
 }
+
+gboolean
+clutter_actor_get_redraw_clip (ClutterActor       *self,
+                               ClutterPaintVolume *dst_old_pv,
+                               ClutterPaintVolume *dst_new_pv)
+{
+  ClutterActorPrivate *priv = self->priv;
+  ClutterPaintVolume *paint_volume;
+
+  paint_volume = _clutter_actor_get_paint_volume_mutable (self);
+  if (!paint_volume || !priv->last_paint_volume_valid)
+    return FALSE;
+
+  _clutter_paint_volume_set_from_volume (dst_old_pv, &priv->last_paint_volume);
+  _clutter_paint_volume_set_from_volume (dst_new_pv, paint_volume);
+
+  return TRUE;
+}

clutter/clutter/clutter-actor.h

@@ -240,8 +240,7 @@ struct _ClutterActorClass
                                  ClutterPickContext    *pick_context);
 
   gboolean (* queue_redraw)     (ClutterActor          *actor,
-                                 ClutterActor          *leaf_that_queued,
+                                 ClutterActor          *leaf_that_queued);
-                                 ClutterPaintVolume    *paint_volume);
 
   /* size negotiation */
   void (* get_preferred_width)  (ClutterActor           *self,

clutter/clutter/clutter-stage.c

@@ -1054,68 +1054,6 @@ is_full_stage_redraw_queued (ClutterStage *stage)
   return TRUE;
 }
 
-static gboolean
-clutter_stage_real_queue_redraw (ClutterActor       *actor,
-                                 ClutterActor       *leaf,
-                                 ClutterPaintVolume *redraw_clip)
-{
-  ClutterStage *stage = CLUTTER_STAGE (actor);
-  ClutterStageWindow *stage_window;
-  ClutterActorBox bounding_box;
-  ClutterActorBox intersection_box;
-  cairo_rectangle_int_t geom, stage_clip;
-
-  if (CLUTTER_ACTOR_IN_DESTRUCTION (actor))
-    return TRUE;
-
-  /* If the backend can't do anything with redraw clips (e.g. it already knows
-   * it needs to redraw everything anyway) then don't spend time transforming
-   * any clip volume into stage coordinates... */
-  stage_window = _clutter_stage_get_window (stage);
-  if (stage_window == NULL)
-    return TRUE;
-
-  if (is_full_stage_redraw_queued (stage))
-    return FALSE;
-
-  if (redraw_clip == NULL)
-    {
-      clutter_stage_add_redraw_clip (stage, NULL);
-      return FALSE;
-    }
-
-  if (redraw_clip->is_empty)
-    return TRUE;
-
-  /* Convert the clip volume into stage coordinates and then into an
-   * axis aligned stage coordinates bounding box... */
-  _clutter_paint_volume_get_stage_paint_box (redraw_clip,
-                                             stage,
-                                             &bounding_box);
-
-  _clutter_stage_window_get_geometry (stage_window, &geom);
-
-  intersection_box.x1 = MAX (bounding_box.x1, 0);
-  intersection_box.y1 = MAX (bounding_box.y1, 0);
-  intersection_box.x2 = MIN (bounding_box.x2, geom.width);
-  intersection_box.y2 = MIN (bounding_box.y2, geom.height);
-
-  /* There is no need to track degenerate/empty redraw clips */
-  if (intersection_box.x2 <= intersection_box.x1 ||
-      intersection_box.y2 <= intersection_box.y1)
-    return TRUE;
-
-  /* when converting to integer coordinates make sure we round the edges of the
-   * clip rectangle outwards... */
-  stage_clip.x = intersection_box.x1;
-  stage_clip.y = intersection_box.y1;
-  stage_clip.width = intersection_box.x2 - stage_clip.x;
-  stage_clip.height = intersection_box.y2 - stage_clip.y;
-
-  clutter_stage_add_redraw_clip (stage, &stage_clip);
-  return FALSE;
-}
-
 gboolean
 _clutter_stage_has_full_redraw_queued (ClutterStage *stage)
 {
@@ -1437,7 +1375,6 @@ clutter_stage_class_init (ClutterStageClass *klass)
   actor_class->hide = clutter_stage_hide;
   actor_class->hide_all = clutter_stage_hide_all;
   actor_class->queue_relayout = clutter_stage_real_queue_relayout;
-  actor_class->queue_redraw = clutter_stage_real_queue_redraw;
   actor_class->apply_transform = clutter_stage_real_apply_transform;
 
   klass->paint_view = clutter_stage_real_paint_view;
@@ -2882,6 +2819,65 @@ _clutter_stage_queue_redraw_entry_invalidate (ClutterStageQueueRedrawEntry *entr
     }
 }
 
+static void
+add_to_stage_clip (ClutterStage       *stage,
+                   ClutterPaintVolume *redraw_clip)
+{
+  ClutterStageWindow *stage_window;
+  ClutterActorBox bounding_box;
+  ClutterActorBox intersection_box;
+  cairo_rectangle_int_t geom, stage_clip;
+
+  if (CLUTTER_ACTOR_IN_DESTRUCTION (CLUTTER_ACTOR (stage)))
+    return;
+
+  /* If the backend can't do anything with redraw clips (e.g. it already knows
+   * it needs to redraw everything anyway) then don't spend time transforming
+   * any clip volume into stage coordinates... */
+  stage_window = _clutter_stage_get_window (stage);
+  if (stage_window == NULL)
+    return;
+
+  if (is_full_stage_redraw_queued (stage))
+    return;
+
+  if (redraw_clip == NULL)
+    {
+      clutter_stage_add_redraw_clip (stage, NULL);
+      return;
+    }
+
+  if (redraw_clip->is_empty)
+    return;
+
+  /* Convert the clip volume into stage coordinates and then into an
+   * axis aligned stage coordinates bounding box... */
+  _clutter_paint_volume_get_stage_paint_box (redraw_clip,
+                                             stage,
+                                             &bounding_box);
+
+  _clutter_stage_window_get_geometry (stage_window, &geom);
+
+  intersection_box.x1 = MAX (bounding_box.x1, 0);
+  intersection_box.y1 = MAX (bounding_box.y1, 0);
+  intersection_box.x2 = MIN (bounding_box.x2, geom.width);
+  intersection_box.y2 = MIN (bounding_box.y2, geom.height);
+
+  /* There is no need to track degenerate/empty redraw clips */
+  if (intersection_box.x2 <= intersection_box.x1 ||
+      intersection_box.y2 <= intersection_box.y1)
+    return;
+
+  /* when converting to integer coordinates make sure we round the edges of the
+   * clip rectangle outwards... */
+  stage_clip.x = intersection_box.x1;
+  stage_clip.y = intersection_box.y1;
+  stage_clip.width = intersection_box.x2 - stage_clip.x;
+  stage_clip.height = intersection_box.y2 - stage_clip.y;
+
+  clutter_stage_add_redraw_clip (stage, &stage_clip);
+}
+
 void
 clutter_stage_maybe_finish_queue_redraws (ClutterStage *stage)
 {
@@ -2913,15 +2909,44 @@ clutter_stage_maybe_finish_queue_redraws (ClutterStage *stage)
       for (l = stolen_list; l; l = l->next)
         {
           ClutterStageQueueRedrawEntry *entry = l->data;
-          ClutterPaintVolume *clip;
 
           /* NB: Entries may be invalidated if the actor gets destroyed */
           if (G_LIKELY (entry->actor != NULL))
-	    {
+            {
-	      clip = entry->has_clip ? &entry->clip : NULL;
+              ClutterPaintVolume old_actor_pv, new_actor_pv;
 
-	      _clutter_actor_finish_queue_redraw (entry->actor, clip);
+              _clutter_actor_finish_queue_redraw (entry->actor);
-	    }
+
+              _clutter_paint_volume_init_static (&old_actor_pv, NULL);
+              _clutter_paint_volume_init_static (&new_actor_pv, NULL);
+
+              if (entry->has_clip)
+                {
+                  add_to_stage_clip (stage, &entry->clip);
+                }
+              else if (clutter_actor_get_redraw_clip (entry->actor,
+                                                      &old_actor_pv,
+                                                      &new_actor_pv))
+                {
+                  /* Add both the old paint volume of the actor (which is
+                   * currently visible on the screen) and the new paint volume
+                   * (which will be visible on the screen after this redraw)
+                   * to the redraw clip.
+                   * The former we do to ensure the old texture on the screen
+                   * will be fully painted over in case the actor was moved.
+                   */
+                  add_to_stage_clip (stage, &old_actor_pv);
+                  add_to_stage_clip (stage, &new_actor_pv);
+                }
+              else
+                {
+                  /* If there's no clip we can use, we have to trigger an
+                   * unclipped full stage redraw.
+                   */
+                  add_to_stage_clip (stage, NULL);
+                }
+
+            }
 
           free_queue_redraw_entry (entry);
         }