`ClutterOffscreenEffect` had been getting the wrong bounding box in the
case of clones and descendents of clones, causing visibly incorrect
clipping. This was due to `clutter_actor_get_paint_box` only ever being
given the source actor during a paint (which is correct) and not the clone.
Even if we weren't painting a clone but an offscreened descendent of a
clone (like in gnome-shell's desktop zoom), we would get the wrong result.
Fortunately we don't need to know the actual clone/actor being painted so
don't need to call the problematic `clutter_actor_get_paint_box` at all.
The solution is to only keep untransformed rendering in the FBO and leave
the correct transformation for later. The correct clone/actor's
transformation is already set for us as the current cogl modelview matrix
by `clutter_actor_paint`.
Bonus optimization: This all means we don't need to keep `last_matrix_drawn`
or force a full repaint every time some part of the transformation changes.
Because the FBO contents are no longer affected by transformations. As it
should be. In other words, offscreen-effected actors can now move around
on screen without themselves being repainted.
Special thanks to Mai Lavelle for identifying the cause of the problem.
Fixes:
https://bugzilla.gnome.org/show_bug.cgi?id=789050,
https://bugzilla.gnome.org/show_bug.cgi?id=659523#c9,
https://gitlab.gnome.org/GNOME/mutter/issues/196,
https://gitlab.gnome.org/GNOME/mutter/issues/282,
https://gitlab.gnome.org/GNOME/gnome-shell/issues/387,
https://launchpad.net/bugs/
1767648,
https://launchpad.net/bugs/
1779615
Origin: https://gitlab.gnome.org/vanvugt/mutter/commit/
9a466f289318050bde065bc8878947cb2691bc98
Bug-Ubuntu: https://launchpad.net/bugs/
1767648, https://launchpad.net/bugs/
1779615
Applied-Upstream: 3.31.90
Gbp-Pq: Name clutter-Fix-offscreen-effect-painting-of-clones.patch
--- /dev/null
+#ifndef __CLUTTER_ACTOR_BOX_PRIVATE_H__
+#define __CLUTTER_ACTOR_BOX_PRIVATE_H__
+
+#include <clutter/clutter-types.h>
+
+G_BEGIN_DECLS
+
+void _clutter_actor_box_enlarge_for_effects (ClutterActorBox *box);
+
+G_END_DECLS
+
+#endif /* __CLUTTER_ACTOR_BOX_PRIVATE_H__ */
#include "clutter-types.h"
#include "clutter-interval.h"
#include "clutter-private.h"
+#include "clutter-actor-box-private.h"
/**
* clutter_actor_box_new:
box->y2 = box->y1 + height;
}
+void
+_clutter_actor_box_enlarge_for_effects (ClutterActorBox *box)
+{
+ float width, height;
+
+ /* The aim here is that for a given rectangle defined with floating point
+ * coordinates we want to determine a stable quantized size in pixels
+ * that doesn't vary due to the original box's sub-pixel position.
+ *
+ * The reason this is important is because effects will use this
+ * API to determine the size of offscreen framebuffers and so for
+ * a fixed-size object that may be animated accross the screen we
+ * want to make sure that the stage paint-box has an equally stable
+ * size so that effects aren't made to continuously re-allocate
+ * a corresponding fbo.
+ *
+ * The other thing we consider is that the calculation of this box is
+ * subject to floating point precision issues that might be slightly
+ * different to the precision issues involved with actually painting the
+ * actor, which might result in painting slightly leaking outside the
+ * user's calculated paint-volume. For this we simply aim to pad out the
+ * paint-volume by at least half a pixel all the way around.
+ */
+ width = box->x2 - box->x1;
+ height = box->y2 - box->y1;
+ width = CLUTTER_NEARBYINT (width);
+ height = CLUTTER_NEARBYINT (height);
+ /* XXX: NB the width/height may now be up to 0.5px too small so we
+ * must also pad by 0.25px all around to account for this. In total we
+ * must padd by at least 0.75px around all sides. */
+
+ /* XXX: The furthest that we can overshoot the bottom right corner by
+ * here is 1.75px in total if you consider that the 0.75 padding could
+ * just cross an integer boundary and so ceil will effectively add 1.
+ */
+ box->x2 = ceilf (box->x2 + 0.75);
+ box->y2 = ceilf (box->y2 + 0.75);
+
+ /* Now we redefine the top-left relative to the bottom right based on the
+ * rounded width/height determined above + a constant so that the overall
+ * size of the box will be stable and not dependant on the box's
+ * position.
+ *
+ * Adding 3px to the width/height will ensure we cover the maximum of
+ * 1.75px padding on the bottom/right and still ensure we have > 0.75px
+ * padding on the top/left.
+ */
+ box->x1 = box->x2 - width - 3;
+ box->y1 = box->y2 - height - 3;
+}
+
G_DEFINE_BOXED_TYPE_WITH_CODE (ClutterActorBox, clutter_actor_box,
clutter_actor_box_copy,
clutter_actor_box_free,
#include "clutter-debug.h"
#include "clutter-private.h"
#include "clutter-stage-private.h"
+#include "clutter-paint-volume-private.h"
+#include "clutter-actor-box-private.h"
struct _ClutterOffscreenEffectPrivate
{
ClutterActor *actor;
ClutterActor *stage;
- gfloat x_offset;
- gfloat y_offset;
+ ClutterVertex position;
+
+ int fbo_offset_x;
+ int fbo_offset_y;
/* This is the calculated size of the fbo before being passed
through create_texture(). This needs to be tracked separately so
int fbo_height;
gint old_opacity_override;
-
- /* The matrix that was current the last time the fbo was updated. We
- need to keep track of this to detect when we can reuse the
- contents of the fbo without redrawing the actor. We need the
- actual matrix rather than just detecting queued redraws on the
- actor because any change in the parent hierarchy (even just a
- translation) could cause the actor to look completely different
- and it won't cause a redraw to be queued on the parent's
- children. */
- CoglMatrix last_matrix_drawn;
};
G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE (ClutterOffscreenEffect,
{
ClutterOffscreenEffect *self = CLUTTER_OFFSCREEN_EFFECT (effect);
ClutterOffscreenEffectPrivate *priv = self->priv;
- ClutterActorBox box;
+ ClutterActorBox raw_box, box;
ClutterActor *stage;
- CoglMatrix projection;
+ CoglMatrix projection, old_modelview, modelview;
+ const ClutterPaintVolume *volume;
CoglColor transparent;
gfloat stage_width, stage_height;
gfloat fbo_width = -1, fbo_height = -1;
- gfloat width, height;
- gfloat xexpand, yexpand;
- int texture_width, texture_height;
+ ClutterVertex local_offset = { 0.f, 0.f, 0.f };
+ gfloat old_viewport[4];
if (!clutter_actor_meta_get_enabled (CLUTTER_ACTOR_META (effect)))
return FALSE;
stage = _clutter_actor_get_stage_internal (priv->actor);
clutter_actor_get_size (stage, &stage_width, &stage_height);
- /* The paint box is the bounding box of the actor's paint volume in
- * stage coordinates. This will give us the size for the framebuffer
- * we need to redirect its rendering offscreen and its position will
- * be used to setup an offset viewport */
- if (clutter_actor_get_paint_box (priv->actor, &box))
+ /* Get the minimal bounding box for what we want to paint, relative to the
+ * parent of priv->actor. Note that we may actually be painting a clone of
+ * priv->actor so we need to be careful to avoid querying the transformation
+ * of priv->actor (like clutter_actor_get_paint_box would). Just stay in
+ * local coordinates for now...
+ */
+ volume = clutter_actor_get_paint_volume (priv->actor);
+ if (volume)
{
- clutter_actor_box_get_size (&box, &fbo_width, &fbo_height);
- clutter_actor_box_get_origin (&box, &priv->x_offset, &priv->y_offset);
+ ClutterPaintVolume mutable_volume;
- fbo_width = MIN (fbo_width, stage_width);
- fbo_height = MIN (fbo_height, stage_height);
+ _clutter_paint_volume_copy_static (volume, &mutable_volume);
+ _clutter_paint_volume_get_bounding_box (&mutable_volume, &raw_box);
+ clutter_paint_volume_free (&mutable_volume);
}
else
{
- fbo_width = stage_width;
- fbo_height = stage_height;
+ clutter_actor_get_allocation_box (priv->actor, &raw_box);
}
- if (fbo_width == stage_width)
- priv->x_offset = 0.0f;
- if (fbo_height == stage_height)
- priv->y_offset = 0.0f;
+ box = raw_box;
+ _clutter_actor_box_enlarge_for_effects (&box);
+
+ priv->fbo_offset_x = box.x1 - raw_box.x1;
+ priv->fbo_offset_y = box.y1 - raw_box.y1;
+
+ clutter_actor_box_get_size (&box, &fbo_width, &fbo_height);
/* First assert that the framebuffer is the right size... */
if (!update_fbo (effect, fbo_width, fbo_height))
return FALSE;
- texture_width = cogl_texture_get_width (priv->texture);
- texture_height = cogl_texture_get_height (priv->texture);
-
- /* get the current modelview matrix so that we can copy it to the
- * framebuffer. We also store the matrix that was last used when we
- * updated the FBO so that we can detect when we don't need to
- * update the FBO to paint a second time */
- cogl_get_modelview_matrix (&priv->last_matrix_drawn);
+ cogl_get_modelview_matrix (&old_modelview);
/* let's draw offscreen */
cogl_push_framebuffer (priv->offscreen);
- /* Copy the modelview that would have been used if rendering onscreen */
- cogl_set_modelview_matrix (&priv->last_matrix_drawn);
+ /* We don't want the FBO contents to be transformed. That could waste memory
+ * (e.g. during zoom), or result in something that's not rectangular (clipped
+ * incorrectly). So drop the modelview matrix of the current paint chain.
+ * This is fine since paint_texture runs with the same modelview matrix,
+ * so it will come out correctly whenever that is used to put the FBO
+ * contents on screen...
+ */
+ clutter_actor_get_transform (priv->stage, &modelview);
+ cogl_set_modelview_matrix (&modelview);
- /* Set up the viewport so that it has the same size as the stage,
- * but offset it so that the actor of interest lands on our
- * framebuffer. */
- clutter_actor_get_size (priv->stage, &width, &height);
+ /* Save the original viewport for calculating priv->position */
+ _clutter_stage_get_viewport (CLUTTER_STAGE (priv->stage),
+ &old_viewport[0],
+ &old_viewport[1],
+ &old_viewport[2],
+ &old_viewport[3]);
- /* Expand the viewport if the actor is partially off-stage,
- * otherwise the actor will end up clipped to the stage viewport
+ /* Set up the viewport so that it has the same size as the stage (avoid
+ * distortion), but translated to account for the FBO offset...
*/
- xexpand = 0.f;
- if (priv->x_offset < 0.f)
- xexpand = -priv->x_offset;
- if (priv->x_offset + texture_width > width)
- xexpand = MAX (xexpand, (priv->x_offset + texture_width) - width);
-
- yexpand = 0.f;
- if (priv->y_offset < 0.f)
- yexpand = -priv->y_offset;
- if (priv->y_offset + texture_height > height)
- yexpand = MAX (yexpand, (priv->y_offset + texture_height) - height);
-
- /* Set the viewport */
- cogl_set_viewport (-(priv->x_offset + xexpand), -(priv->y_offset + yexpand),
- width + (2 * xexpand), height + (2 * yexpand));
+ cogl_set_viewport (-priv->fbo_offset_x,
+ -priv->fbo_offset_y,
+ stage_width,
+ stage_height);
/* Copy the stage's projection matrix across to the framebuffer */
_clutter_stage_get_projection_matrix (CLUTTER_STAGE (priv->stage),
&projection);
- /* If we've expanded the viewport, make sure to scale the projection
- * matrix accordingly (as it's been initialised to work with the
- * original viewport and not our expanded one).
+ /* Now save the global position of the effect (not just of the actor).
+ * It doesn't appear anyone actually uses this yet, but get_target_rect is
+ * documented as returning it. So we should...
*/
- if (xexpand > 0.f || yexpand > 0.f)
- {
- gfloat new_width, new_height;
-
- new_width = width + (2 * xexpand);
- new_height = height + (2 * yexpand);
-
- cogl_matrix_scale (&projection,
- width / new_width,
- height / new_height,
- 1);
- }
+ _clutter_util_fully_transform_vertices (&old_modelview,
+ &projection,
+ old_viewport,
+ &local_offset,
+ &priv->position,
+ 1);
cogl_set_projection_matrix (&projection);
cogl_push_matrix ();
- /* Now reset the modelview to put us in stage coordinates so
- * we can drawn the result of our offscreen render as a textured
- * quad... */
-
- cogl_matrix_init_identity (&modelview);
- _clutter_actor_apply_modelview_transform (priv->stage, &modelview);
- cogl_matrix_translate (&modelview, priv->x_offset, priv->y_offset, 0.0f);
+ /* The current modelview matrix is *almost* perfect already. It's only
+ * missing a correction for the expanded FBO and offset rendering within...
+ */
+ cogl_get_modelview_matrix (&modelview);
+ cogl_matrix_translate (&modelview,
+ priv->fbo_offset_x,
+ priv->fbo_offset_y,
+ 0.0f);
cogl_set_modelview_matrix (&modelview);
/* paint the target material; this is virtualized for
{
ClutterOffscreenEffect *self = CLUTTER_OFFSCREEN_EFFECT (effect);
ClutterOffscreenEffectPrivate *priv = self->priv;
- CoglMatrix matrix;
-
- cogl_get_modelview_matrix (&matrix);
- /* If we've already got a cached image for the same matrix and the
- actor hasn't been redrawn then we can just use the cached image
- in the fbo */
- if (priv->offscreen == NULL ||
- (flags & CLUTTER_EFFECT_PAINT_ACTOR_DIRTY) ||
- !cogl_matrix_equal (&matrix, &priv->last_matrix_drawn))
+ /* If we've already got a cached image and the actor hasn't been redrawn
+ * then we can just use the cached image in the FBO.
+ */
+ if (priv->offscreen == NULL || (flags & CLUTTER_EFFECT_PAINT_ACTOR_DIRTY))
{
/* Chain up to the parent paint method which will call the pre and
post paint functions to update the image */
return FALSE;
clutter_rect_init (rect,
- priv->x_offset,
- priv->y_offset,
+ priv->position.x,
+ priv->position.y,
cogl_texture_get_width (priv->texture),
cogl_texture_get_height (priv->texture));
#include "clutter-paint-volume-private.h"
#include "clutter-private.h"
#include "clutter-stage-private.h"
+#include "clutter-actor-box-private.h"
G_DEFINE_BOXED_TYPE (ClutterPaintVolume, clutter_paint_volume,
clutter_paint_volume_copy,
CoglMatrix modelview;
CoglMatrix projection;
float viewport[4];
- float width;
- float height;
_clutter_paint_volume_copy_static (pv, &projected_pv);
return;
}
- /* The aim here is that for a given rectangle defined with floating point
- * coordinates we want to determine a stable quantized size in pixels
- * that doesn't vary due to the original box's sub-pixel position.
- *
- * The reason this is important is because effects will use this
- * API to determine the size of offscreen framebuffers and so for
- * a fixed-size object that may be animated accross the screen we
- * want to make sure that the stage paint-box has an equally stable
- * size so that effects aren't made to continuously re-allocate
- * a corresponding fbo.
- *
- * The other thing we consider is that the calculation of this box is
- * subject to floating point precision issues that might be slightly
- * different to the precision issues involved with actually painting the
- * actor, which might result in painting slightly leaking outside the
- * user's calculated paint-volume. For this we simply aim to pad out the
- * paint-volume by at least half a pixel all the way around.
- */
- width = box->x2 - box->x1;
- height = box->y2 - box->y1;
- width = CLUTTER_NEARBYINT (width);
- height = CLUTTER_NEARBYINT (height);
- /* XXX: NB the width/height may now be up to 0.5px too small so we
- * must also pad by 0.25px all around to account for this. In total we
- * must padd by at least 0.75px around all sides. */
-
- /* XXX: The furthest that we can overshoot the bottom right corner by
- * here is 1.75px in total if you consider that the 0.75 padding could
- * just cross an integer boundary and so ceil will effectively add 1.
- */
- box->x2 = ceilf (box->x2 + 0.75);
- box->y2 = ceilf (box->y2 + 0.75);
-
- /* Now we redefine the top-left relative to the bottom right based on the
- * rounded width/height determined above + a constant so that the overall
- * size of the box will be stable and not dependant on the box's
- * position.
- *
- * Adding 3px to the width/height will ensure we cover the maximum of
- * 1.75px padding on the bottom/right and still ensure we have > 0.75px
- * padding on the top/left.
- */
- box->x1 = box->x2 - width - 3;
- box->y1 = box->y2 - height - 3;
+ _clutter_actor_box_enlarge_for_effects (box);
clutter_paint_volume_free (&projected_pv);
}
actor-iter \
actor-layout \
actor-meta \
- actor-offscreen-limit-max-size \
actor-offscreen-redirect \
actor-paint-opacity \
actor-pick \
+++ /dev/null
-#define CLUTTER_ENABLE_EXPERIMENTAL_API
-#include <clutter/clutter.h>
-
-#define STAGE_WIDTH (300)
-#define STAGE_HEIGHT (300)
-
-typedef struct
-{
- ClutterActor *stage;
-
- ClutterActor *actor_group1;
- ClutterEffect *blur_effect1;
-
- ClutterActor *actor_group2;
- ClutterEffect *blur_effect2;
-} Data;
-
-static void
-check_results (ClutterStage *stage, gpointer user_data)
-{
- Data *data = user_data;
- gfloat width, height;
- ClutterRect rect;
-
- clutter_offscreen_effect_get_target_rect (CLUTTER_OFFSCREEN_EFFECT (data->blur_effect1),
- &rect);
-
- width = clutter_rect_get_width (&rect);
- height = clutter_rect_get_height (&rect);
-
- if (g_test_verbose ())
- g_print ("Checking effect1 size: %.2f x %.2f\n",
- clutter_rect_get_width (&rect),
- clutter_rect_get_height (&rect));
-
- g_assert_cmpint (width, <, STAGE_WIDTH);
- g_assert_cmpint (height, <, STAGE_HEIGHT);
-
- clutter_offscreen_effect_get_target_rect (CLUTTER_OFFSCREEN_EFFECT (data->blur_effect2),
- &rect);
-
- width = clutter_rect_get_width (&rect);
- height = clutter_rect_get_height (&rect);
-
- if (g_test_verbose ())
- g_print ("Checking effect2 size: %.2f x %.2f\n", width, height);
-
- g_assert_cmpint (width, ==, STAGE_WIDTH);
- g_assert_cmpint (height, ==, STAGE_HEIGHT);
-
-
- clutter_main_quit ();
-}
-
-static ClutterActor *
-create_actor (gfloat x, gfloat y,
- gfloat width, gfloat height,
- const ClutterColor *color)
-{
- return g_object_new (CLUTTER_TYPE_ACTOR,
- "x", x,
- "y", y,
- "width", width,
- "height", height,
- "background-color", color,
- NULL);
-}
-
-static void
-actor_offscreen_limit_max_size (void)
-{
- Data data;
-
- if (!cogl_features_available (COGL_FEATURE_OFFSCREEN))
- return;
-
- data.stage = clutter_test_get_stage ();
- g_signal_connect (data.stage, "after-paint",
- G_CALLBACK (check_results), &data);
- clutter_actor_set_size (data.stage, STAGE_WIDTH, STAGE_HEIGHT);
-
- data.actor_group1 = clutter_actor_new ();
- clutter_actor_add_child (data.stage, data.actor_group1);
- data.blur_effect1 = clutter_blur_effect_new ();
- clutter_actor_add_effect (data.actor_group1, data.blur_effect1);
- clutter_actor_add_child (data.actor_group1,
- create_actor (10, 10,
- 100, 100,
- CLUTTER_COLOR_Blue));
- clutter_actor_add_child (data.actor_group1,
- create_actor (100, 100,
- 100, 100,
- CLUTTER_COLOR_Gray));
-
- data.actor_group2 = clutter_actor_new ();
- clutter_actor_add_child (data.stage, data.actor_group2);
- data.blur_effect2 = clutter_blur_effect_new ();
- clutter_actor_add_effect (data.actor_group2, data.blur_effect2);
- clutter_actor_add_child (data.actor_group2,
- create_actor (-10, -10,
- 100, 100,
- CLUTTER_COLOR_Yellow));
- clutter_actor_add_child (data.actor_group2,
- create_actor (250, 10,
- 100, 100,
- CLUTTER_COLOR_ScarletRed));
- clutter_actor_add_child (data.actor_group2,
- create_actor (10, 250,
- 100, 100,
- CLUTTER_COLOR_Yellow));
-
- clutter_actor_show (data.stage);
-
- clutter_main ();
-}
-
-CLUTTER_TEST_SUITE (
- CLUTTER_TEST_UNIT ("/actor/offscreen/limit-max-size", actor_offscreen_limit_max_size)
-)
clutter_actor_queue_redraw (data->child);
verify_redraw (data, 1);
- /* Modifying the transformation on the parent should cause a
- redraw */
+ /* Modifying the transformation on the parent should not cause a redraw,
+ since the FBO stores pre-transformed rendering that can be reused with
+ any transformation. */
clutter_actor_set_anchor_point (data->parent_container, 0, 1);
- verify_redraw (data, 1);
+ verify_redraw (data, 0);
/* Redrawing an unrelated actor shouldn't cause a redraw */
clutter_actor_set_position (data->unrelated_actor, 0, 1);
projects / mutter.git / commitdiff