From 9f121a211dc521862eb5d9a5df17fef8e320410d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Wed, 20 May 2020 20:14:40 +0200 Subject: [PATCH 1/6] clutter/actor: Always reset absolute_origin_changed after relayout Since the introduction of the shallow relayout functionality it's possible to start an allocation cycle at any point in the tree, not only at the stage. Now when starting an allocation at an actor that's not the stage, we'd still look at the absolute_origin_changed property of this actors parent, which might still be set to TRUE from the parents last allocation. So avoid using the parents absolute_origin_changed property from the last allocation in case a shallow relayout is being done and always reset the absolute_origin_changed property to FALSE after the allocation cycle. This broke with the removal of the ABSOLUTE_ORIGIN_CHANGED ClutterAllocationFlag that was done in commit dc8e5c7f. https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1247 --- clutter/clutter/clutter-actor.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/clutter/clutter/clutter-actor.c b/clutter/clutter/clutter-actor.c index 05d90196283..6dbf3668d7e 100644 --- a/clutter/clutter/clutter-actor.c +++ b/clutter/clutter/clutter-actor.c @@ -10165,7 +10165,7 @@ clutter_actor_allocate (ClutterActor *self, if (!priv->needs_allocation && !stage_allocation_changed) { CLUTTER_NOTE (LAYOUT, "No allocation needed"); - return; + goto out; } if (CLUTTER_ACTOR_IS_MAPPED (self)) @@ -10179,12 +10179,15 @@ clutter_actor_allocate (ClutterActor *self, /* If the actor didn't move but needs_allocation is set, we just * need to allocate the children */ clutter_actor_allocate_internal (self, &real_allocation); - return; + goto out; } _clutter_actor_create_transition (self, obj_props[PROP_ALLOCATION], &priv->allocation, &real_allocation); + +out: + priv->absolute_origin_changed = FALSE; } /** -- GitLab From 0a37c32a727a8cb4aaa447dfc9574e220baf6bcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sat, 9 May 2020 23:39:26 +0200 Subject: [PATCH 2/6] clutter/actor: Update absolute_origin_changed inside set_allocation() When manipulating the allocation of a ClutterActor from an allocate() vfunc override, clutter_actor_set_allocation() is used to let Clutter know about the changes. If the actors allocation or its absolute origin did not change before that, this can also affect the actors absolute_origin_changed property used by the children to detect changes to their absolute position. So fix this bug (which luckily didn't seem to affect us so far) and set priv->absolute_origin_changed to TRUE in case the origin changes inside clutter_actor_set_allocation_internal(). Since this function is always called when our allocation changes, we no longer need to update absolute_origin_changed in clutter_actor_allocate() now. Since a change to the absolute origin always affects the resource scale, too, we also need to move that check from clutter_actor_allocate() here to make sure we update the resource scale. https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1247 --- clutter/clutter/clutter-actor.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/clutter/clutter/clutter-actor.c b/clutter/clutter/clutter-actor.c index 6dbf3668d7e..15f7e11e5d3 100644 --- a/clutter/clutter/clutter-actor.c +++ b/clutter/clutter/clutter-actor.c @@ -2603,6 +2603,11 @@ clutter_actor_set_allocation_internal (ClutterActor *self, priv->needs_height_request = FALSE; priv->needs_allocation = FALSE; + priv->absolute_origin_changed |= x1_changed || y1_changed; + + if (priv->absolute_origin_changed || x2_changed || y2_changed) + priv->needs_compute_resource_scale = TRUE; + if (x1_changed || y1_changed || x2_changed || @@ -10146,9 +10151,8 @@ clutter_actor_allocate (ClutterActor *self, ? priv->parent->priv->absolute_origin_changed : FALSE; - priv->absolute_origin_changed |= origin_changed; - - stage_allocation_changed = priv->absolute_origin_changed || size_changed; + stage_allocation_changed = + priv->absolute_origin_changed || origin_changed || size_changed; /* If we get an allocation "out of the blue" * (we did not queue relayout), then we want to @@ -10171,9 +10175,6 @@ clutter_actor_allocate (ClutterActor *self, if (CLUTTER_ACTOR_IS_MAPPED (self)) self->priv->needs_paint_volume_update = TRUE; - if (stage_allocation_changed) - priv->needs_compute_resource_scale = TRUE; - if (!stage_allocation_changed) { /* If the actor didn't move but needs_allocation is set, we just -- GitLab From 7abf0f1e2da0748efa2a412c2e8cf8fe18c09388 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sun, 10 May 2020 01:23:34 +0200 Subject: [PATCH 3/6] clutter/stage: Set viewport without getting the last allocation When getting the last allocation using clutter_actor_get_allocation_box(), Clutter will do an immediate relayout of the stage in case an actor has an invalid allocation. Since the allocation is always invalid when the allocate() vfunc is called, clutter_stage_allocate() always forces another allocation cycle. To fix that, stop comparing the old allocation to the new one to find out whether the viewport changed, but instead use the existing check in _clutter_stage_set_viewport() and implement the behavior of rounding the viewport to the nearest int using roundf() (which should behave just as CLUTTER_NEARBYINT()) since we're passing around floats anyway. https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1247 --- clutter/clutter/clutter-stage.c | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/clutter/clutter/clutter-stage.c b/clutter/clutter/clutter-stage.c index 540f9a1b78a..686152329c6 100644 --- a/clutter/clutter/clutter-stage.c +++ b/clutter/clutter/clutter-stage.c @@ -619,7 +619,6 @@ clutter_stage_allocate (ClutterActor *self, { ClutterStagePrivate *priv = CLUTTER_STAGE (self)->priv; ClutterActorBox alloc = CLUTTER_ACTOR_BOX_INIT_ZERO; - float old_width, old_height; float new_width, new_height; float width, height; cairo_rectangle_int_t window_size; @@ -628,10 +627,6 @@ clutter_stage_allocate (ClutterActor *self, if (priv->impl == NULL) return; - /* our old allocation */ - clutter_actor_get_allocation_box (self, &alloc); - clutter_actor_box_get_size (&alloc, &old_width, &old_height); - /* the current allocation */ clutter_actor_box_get_size (box, &width, &height); @@ -719,27 +714,14 @@ clutter_stage_allocate (ClutterActor *self, &override); } - /* reset the viewport if the allocation effectively changed */ + /* set the viewport to the new allocation */ clutter_actor_get_allocation_box (self, &alloc); clutter_actor_box_get_size (&alloc, &new_width, &new_height); - if (CLUTTER_NEARBYINT (old_width) != CLUTTER_NEARBYINT (new_width) || - CLUTTER_NEARBYINT (old_height) != CLUTTER_NEARBYINT (new_height)) - { - int real_width = CLUTTER_NEARBYINT (new_width); - int real_height = CLUTTER_NEARBYINT (new_height); - - _clutter_stage_set_viewport (CLUTTER_STAGE (self), - 0, 0, - real_width, - real_height); - - /* Note: we don't assume that set_viewport will queue a full redraw - * since it may bail-out early if something preemptively set the - * viewport before the stage was really allocated its new size. - */ - queue_full_redraw (CLUTTER_STAGE (self)); - } + _clutter_stage_set_viewport (CLUTTER_STAGE (self), + 0, 0, + new_width, + new_height); } typedef struct _Vector4 @@ -2494,6 +2476,8 @@ _clutter_stage_set_viewport (ClutterStage *stage, priv = stage->priv; + width = roundf (width); + height = roundf (height); if (x == priv->viewport[0] && y == priv->viewport[1] && -- GitLab From 38104755a232f6957575be99905b71c35a0b8787 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Tue, 2 Jun 2020 15:25:34 +0200 Subject: [PATCH 4/6] clutter/stage: Make set_viewport() a static method Since clutter_stage_set_viewport() is only used inside clutter-stage.c anyway, we can make it a static method. Also we can remove the x and y arguments from it since they're always set to 0 anyway. https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1247 --- clutter/clutter/clutter-stage-private.h | 5 ----- clutter/clutter/clutter-stage.c | 28 +++++++++++-------------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/clutter/clutter/clutter-stage-private.h b/clutter/clutter/clutter-stage-private.h index 2c5a2a50ee1..9f018ab84d8 100644 --- a/clutter/clutter/clutter-stage-private.h +++ b/clutter/clutter/clutter-stage-private.h @@ -50,11 +50,6 @@ ClutterStageWindow *_clutter_stage_get_window (ClutterStage void _clutter_stage_get_projection_matrix (ClutterStage *stage, CoglMatrix *projection); void _clutter_stage_dirty_projection (ClutterStage *stage); -void _clutter_stage_set_viewport (ClutterStage *stage, - float x, - float y, - float width, - float height); void _clutter_stage_get_viewport (ClutterStage *stage, float *x, float *y, diff --git a/clutter/clutter/clutter-stage.c b/clutter/clutter/clutter-stage.c index 686152329c6..4ed58094530 100644 --- a/clutter/clutter/clutter-stage.c +++ b/clutter/clutter/clutter-stage.c @@ -189,6 +189,9 @@ static void capture_view_into (ClutterStage *stage, uint8_t *data, int stride); static void clutter_stage_update_view_perspective (ClutterStage *stage); +static void clutter_stage_set_viewport (ClutterStage *stage, + float width, + float height); static void clutter_container_iface_init (ClutterContainerIface *iface); @@ -718,10 +721,7 @@ clutter_stage_allocate (ClutterActor *self, clutter_actor_get_allocation_box (self, &alloc); clutter_actor_box_get_size (&alloc, &new_width, &new_height); - _clutter_stage_set_viewport (CLUTTER_STAGE (self), - 0, 0, - new_width, - new_height); + clutter_stage_set_viewport (CLUTTER_STAGE (self), new_width, new_height); } typedef struct _Vector4 @@ -2239,10 +2239,7 @@ clutter_stage_init (ClutterStage *self) g_signal_connect (self, "notify::min-height", G_CALLBACK (clutter_stage_notify_min_size), NULL); - _clutter_stage_set_viewport (self, - 0, 0, - geom.width, - geom.height); + clutter_stage_set_viewport (self, geom.width, geom.height); priv->paint_volume_stack = g_array_new (FALSE, FALSE, sizeof (ClutterPaintVolume)); @@ -2429,8 +2426,6 @@ _clutter_stage_dirty_projection (ClutterStage *stage) /* * clutter_stage_set_viewport: * @stage: A #ClutterStage - * @x: The X postition to render the stage at, in window coordinates - * @y: The Y position to render the stage at, in window coordinates * @width: The width to render the stage at, in window coordinates * @height: The height to render the stage at, in window coordinates * @@ -2463,19 +2458,20 @@ _clutter_stage_dirty_projection (ClutterStage *stage) * * Since: 1.6 */ -void -_clutter_stage_set_viewport (ClutterStage *stage, - float x, - float y, - float width, - float height) +static void +clutter_stage_set_viewport (ClutterStage *stage, + float width, + float height) { ClutterStagePrivate *priv; + float x, y; g_return_if_fail (CLUTTER_IS_STAGE (stage)); priv = stage->priv; + x = 0.f; + y = 0.f; width = roundf (width); height = roundf (height); -- GitLab From 9b39e37fee28d5dd6fbcadc11a40d118423573cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sun, 10 May 2020 11:26:37 +0200 Subject: [PATCH 5/6] clutter/actor: Notify hidden actors about absolute allocation changes With commit 0eab73dc2e we introduced an optimization of not doing allocations for actors which are hidden. This broke the propagation of absolute origin changes to hidden actors, so if an actor is moved while its child is hidden, the child will not get priv->needs_compute_resource_scale set to TRUE, which means the resource scale won't be updated when the child gets mapped and shown again. Since we now have priv->absolute_origin_changed, we can simply check whether that is TRUE for our parent before bailing out of clutter_actor_allocate() and if it is, notify the whole hidden sub-tree about the absolute origin change. https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1247 --- clutter/clutter/clutter-actor.c | 43 +++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/clutter/clutter/clutter-actor.c b/clutter/clutter/clutter-actor.c index 15f7e11e5d3..c0eb4a134fb 100644 --- a/clutter/clutter/clutter-actor.c +++ b/clutter/clutter/clutter-actor.c @@ -2560,6 +2560,22 @@ clutter_actor_notify_if_geometry_changed (ClutterActor *self, g_object_thaw_notify (obj); } +static void +absolute_allocation_changed (ClutterActor *actor) +{ + actor->priv->needs_compute_resource_scale = TRUE; +} + +static ClutterActorTraverseVisitFlags +absolute_allocation_changed_cb (ClutterActor *actor, + int depth, + gpointer user_data) +{ + absolute_allocation_changed (actor); + + return CLUTTER_ACTOR_TRAVERSE_VISIT_CONTINUE; +} + /*< private > * clutter_actor_set_allocation_internal: * @self: a #ClutterActor @@ -2606,7 +2622,7 @@ clutter_actor_set_allocation_internal (ClutterActor *self, priv->absolute_origin_changed |= x1_changed || y1_changed; if (priv->absolute_origin_changed || x2_changed || y2_changed) - priv->needs_compute_resource_scale = TRUE; + absolute_allocation_changed (self); if (x1_changed || y1_changed || @@ -10111,11 +10127,26 @@ clutter_actor_allocate (ClutterActor *self, return; } - if (!clutter_actor_is_visible (self)) - return; - priv = self->priv; + priv->absolute_origin_changed = priv->parent + ? priv->parent->priv->absolute_origin_changed + : FALSE; + + if (!CLUTTER_ACTOR_IS_VISIBLE (self)) + { + if (priv->absolute_origin_changed) + { + _clutter_actor_traverse (self, + CLUTTER_ACTOR_TRAVERSE_DEPTH_FIRST, + absolute_allocation_changed_cb, + NULL, + NULL); + } + + goto out; + } + old_allocation = priv->allocation; real_allocation = *box; @@ -10147,10 +10178,6 @@ clutter_actor_allocate (ClutterActor *self, size_changed = (real_allocation.x2 != old_allocation.x2 || real_allocation.y2 != old_allocation.y2); - priv->absolute_origin_changed = priv->parent - ? priv->parent->priv->absolute_origin_changed - : FALSE; - stage_allocation_changed = priv->absolute_origin_changed || origin_changed || size_changed; -- GitLab From 720360b07ad06255dad39643397118568ec656fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Mon, 11 May 2020 15:11:07 +0200 Subject: [PATCH 6/6] clutter/actor: Don't allocate actors if only the absolute origin changed For actors which don't have needs_allocation set to TRUE and where the new allocation wouldn't be different from the old one, the allocate() vfunc doesn't have to be called. We still did this in case a parent actor was moved though (so the absolute origin changed), because we needed to propagate the ABSOLUTE_ORIGIN_CHANGED allocation flag down to all actors. Since that flag is now removed and got replaced with a private property, we can simply notify the children about the absolute allocation change using the existing infrastructure and safely stop allocating children at this point. https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1247 --- clutter/clutter/clutter-actor.c | 35 +++++++++++++++++---------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/clutter/clutter/clutter-actor.c b/clutter/clutter/clutter-actor.c index c0eb4a134fb..f7110380487 100644 --- a/clutter/clutter/clutter-actor.c +++ b/clutter/clutter/clutter-actor.c @@ -10115,7 +10115,6 @@ clutter_actor_allocate (ClutterActor *self, { ClutterActorBox old_allocation, real_allocation; gboolean origin_changed, size_changed; - gboolean stage_allocation_changed; ClutterActorPrivate *priv; g_return_if_fail (CLUTTER_IS_ACTOR (self)); @@ -10178,23 +10177,25 @@ clutter_actor_allocate (ClutterActor *self, size_changed = (real_allocation.x2 != old_allocation.x2 || real_allocation.y2 != old_allocation.y2); - stage_allocation_changed = - priv->absolute_origin_changed || origin_changed || size_changed; - - /* If we get an allocation "out of the blue" - * (we did not queue relayout), then we want to - * ignore it. But if we have needs_allocation set, - * we want to guarantee that allocate() virtual - * method is always called, i.e. that queue_relayout() - * always results in an allocate() invocation on - * an actor. + /* When needs_allocation is set but we didn't move nor resize, we still + * want to call the allocate() vfunc because a child probably called + * queue_relayout() and needs a new allocation. * - * The optimization here is to avoid re-allocating - * actors that did not queue relayout and were - * not moved. + * In case needs_allocation isn't set and we didn't move nor resize, we + * can safely stop allocating, but we need to notify the sub-tree in case + * our absolute origin changed. */ - if (!priv->needs_allocation && !stage_allocation_changed) + if (!priv->needs_allocation && !origin_changed && !size_changed) { + if (priv->absolute_origin_changed) + { + _clutter_actor_traverse (self, + CLUTTER_ACTOR_TRAVERSE_DEPTH_FIRST, + absolute_allocation_changed_cb, + NULL, + NULL); + } + CLUTTER_NOTE (LAYOUT, "No allocation needed"); goto out; } @@ -10202,10 +10203,10 @@ clutter_actor_allocate (ClutterActor *self, if (CLUTTER_ACTOR_IS_MAPPED (self)) self->priv->needs_paint_volume_update = TRUE; - if (!stage_allocation_changed) + if (!origin_changed && !size_changed) { /* If the actor didn't move but needs_allocation is set, we just - * need to allocate the children */ + * need to allocate the children (see comment above) */ clutter_actor_allocate_internal (self, &real_allocation); goto out; } -- GitLab