Make the pick id pool per Stage

The id pool used for the actor's id should be a per-stage field. At some
point, we might have a Stage mapped to multiple framebuffers, or each
Stage mapped to a different framebuffer; also, on backend with low
color precision we don't want to exhaust the size of the available ids
with a global pool. Finally, it's yet another thing we can remove from
the global Clutter context.

Having the id pool allocated per-stage, and the pick id assigned on
Actor:mapped will make the whole pick-id more reliable and future proof.

http://bugzilla.clutter-project.org/show_bug.cgi?id=2633

https://bugzilla.gnome.org/show_bug.cgi?id=647876

clutter/clutter-actor.c

@@ -449,6 +449,8 @@ struct _ClutterActorPrivate
   gchar          *name;
   guint32         id; /* Unique ID */
 
+  gint32 pick_id;
+
   gdouble         scale_x;
   gdouble         scale_y;
   AnchorCoord     scale_center;
@@ -1032,12 +1034,23 @@ clutter_actor_update_map_state (ClutterActor  *self,
 static void
 clutter_actor_real_map (ClutterActor *self)
 {
+  ClutterActorPrivate *priv = self->priv;
+  ClutterActor *stage;
   GList *c;
 
   g_assert (!CLUTTER_ACTOR_IS_MAPPED (self));
 
+  CLUTTER_NOTE (ACTOR, "Mapping actor '%s'",
+                _clutter_actor_get_debug_name (self));
+
   CLUTTER_ACTOR_SET_FLAGS (self, CLUTTER_ACTOR_MAPPED);
 
+  stage = _clutter_actor_get_stage_internal (self);
+  priv->pick_id = _clutter_stage_acquire_pick_id (CLUTTER_STAGE (stage), self);
+  CLUTTER_NOTE (ACTOR, "Pick id '%d' for actor '%s'",
+                priv->pick_id,
+                _clutter_actor_get_debug_name (self));
+
   /* notify on parent mapped before potentially mapping
    * children, so apps see a top-down notification.
    */
@@ -1086,11 +1099,15 @@ clutter_actor_map (ClutterActor *self)
 static void
 clutter_actor_real_unmap (ClutterActor *self)
 {
+  ClutterActorPrivate *priv = self->priv;
   GList *c;
 
   g_assert (CLUTTER_ACTOR_IS_MAPPED (self));
 
-  for (c = self->priv->children; c; c = c->next)
+  CLUTTER_NOTE (ACTOR, "Unmapping actor '%s'",
+                _clutter_actor_get_debug_name (self));
+
+  for (c = priv->children; c; c = c->next)
     {
       ClutterActor *child = c->data;
       clutter_actor_unmap (child);
@@ -1101,8 +1118,8 @@ clutter_actor_real_unmap (ClutterActor *self)
   /* clear the contents of the last paint volume, so that hiding + moving +
    * showing will not result in the wrong area being repainted
    */
-  _clutter_paint_volume_init_static (&self->priv->last_paint_volume, NULL);
+  _clutter_paint_volume_init_static (&priv->last_paint_volume, NULL);
-  self->priv->last_paint_volume_valid = TRUE;
+  priv->last_paint_volume_valid = TRUE;
 
   /* notify on parent mapped after potentially unmapping
    * children, so apps see a bottom-up notification.
@@ -1112,14 +1129,19 @@ clutter_actor_real_unmap (ClutterActor *self)
   /* relinquish keyboard focus if we were unmapped while owning it */
   if (!CLUTTER_ACTOR_IS_TOPLEVEL (self))
     {
-      ClutterActor *stage;
+      ClutterStage *stage;
 
-      stage = _clutter_actor_get_stage_internal (self);
+      stage = CLUTTER_STAGE (_clutter_actor_get_stage_internal (self));
+
+      if (stage != NULL)
+        _clutter_stage_release_pick_id (stage, priv->pick_id);
+
+      priv->pick_id = -1;
 
       if (stage != NULL &&
-          clutter_stage_get_key_focus (CLUTTER_STAGE (stage)) == self)
+          clutter_stage_get_key_focus (stage) == self)
         {
-          clutter_stage_set_key_focus (CLUTTER_STAGE (stage), NULL);
+          clutter_stage_set_key_focus (stage, NULL);
         }
     }
 }
@@ -2653,7 +2675,10 @@ actor_has_shader_data (ClutterActor *self)
 guint32
 _clutter_actor_get_pick_id (ClutterActor *self)
 {
-  return self->priv->id;
+  if (self->priv->pick_id < 0)
+    return 0;
+
+  return self->priv->pick_id;
 }
 
 /**
@@ -5003,6 +5028,7 @@ clutter_actor_init (ClutterActor *self)
   priv->has_clip = FALSE;
   priv->opacity = 0xff;
   priv->id = _clutter_context_acquire_id (self);
+  priv->pick_id = -1;
   priv->scale_x = 1.0;
   priv->scale_y = 1.0;
   priv->show_on_set_parent = TRUE;

clutter/clutter-main.c

@@ -304,13 +304,17 @@ clutter_get_motion_events_enabled (void)
 }
 
 ClutterActor *
-_clutter_get_actor_by_id (guint32 actor_id)
+_clutter_get_actor_by_id (ClutterStage *stage,
+                          guint32       actor_id)
 {
-  ClutterMainContext *context = _clutter_context_get_default ();
+  if (stage == NULL)
+    {
+      ClutterMainContext *context = _clutter_context_get_default ();
 
-  g_assert (context->id_pool != NULL);
+      return _clutter_id_pool_lookup (context->id_pool, actor_id);
+    }
 
-  return _clutter_id_pool_lookup (context->id_pool, actor_id);
+  return _clutter_stage_get_actor_by_pick_id (stage, actor_id);
 }
 
 void
@@ -2302,7 +2306,7 @@ _clutter_process_event (ClutterEvent *event)
 ClutterActor *
 clutter_get_actor_by_gid (guint32 id_)
 {
-  return _clutter_get_actor_by_id (id_);
+  return _clutter_get_actor_by_id (NULL, id_);
 }
 
 void

clutter/clutter-private.h

@@ -194,10 +194,11 @@ G_CONST_RETURN gchar *_clutter_gettext (const gchar *str);
 gboolean      _clutter_feature_init (GError **error);
 
 /* Picking code */
-guint         _clutter_pixel_to_id (guchar pixel[4]);
+guint           _clutter_pixel_to_id            (guchar        pixel[4]);
-void          _clutter_id_to_color (guint id,
+void            _clutter_id_to_color            (guint         id,
-                                    ClutterColor *col);
+                                                 ClutterColor *col);
-ClutterActor *_clutter_get_actor_by_id (guint32 actor_id);
+ClutterActor *  _clutter_get_actor_by_id        (ClutterStage *stage,
+                                                 guint32       actor_id);
 
 /* use this function as the accumulator if you have a signal with
  * a G_TYPE_BOOLEAN return value; this will stop the emission as

clutter/clutter-stage-private.h

@@ -96,6 +96,13 @@ gboolean _clutter_stage_get_motion_events_enabled (ClutterStage *stage);
 
 CoglFramebuffer *_clutter_stage_get_active_framebuffer (ClutterStage *stage);
 
+gint32          _clutter_stage_acquire_pick_id          (ClutterStage *stage,
+                                                         ClutterActor *actor);
+void            _clutter_stage_release_pick_id          (ClutterStage *stage,
+                                                         gint32        pick_id);
+ClutterActor *  _clutter_stage_get_actor_by_pick_id     (ClutterStage *stage,
+                                                         gint32        pick_id);
+
 G_END_DECLS
 
 #endif /* __CLUTTER_STAGE_PRIVATE_H__ */

clutter/clutter-stage.c

@@ -66,6 +66,7 @@
 #include "clutter-device-manager-private.h"
 #include "clutter-enum-types.h"
 #include "clutter-event-private.h"
+#include "clutter-id-pool.h"
 #include "clutter-main.h"
 #include "clutter-marshal.h"
 #include "clutter-master-clock.h"
@@ -155,6 +156,8 @@ struct _ClutterStagePrivate
   GTimer *fps_timer;
   gint32 timer_n_frames;
 
+  ClutterIDPool *pick_id_pool;
+
 #ifdef CLUTTER_ENABLE_DEBUG
   gulong redraw_count;
 #endif /* CLUTTER_ENABLE_DEBUG */
@@ -1284,7 +1287,7 @@ _clutter_stage_do_pick (ClutterStage   *stage,
         }
 
       id_ = _clutter_pixel_to_id (pixel);
-      actor = _clutter_get_actor_by_id (id_);
+      actor = _clutter_get_actor_by_id (stage, id_);
       goto result;
     }
 
@@ -1376,7 +1379,7 @@ _clutter_stage_do_pick (ClutterStage   *stage,
     }
 
   id_ = _clutter_pixel_to_id (pixel);
-  actor = _clutter_get_actor_by_id (id_);
+  actor = _clutter_get_actor_by_id (stage, id_);
 
 result:
 
@@ -1598,6 +1601,8 @@ clutter_stage_finalize (GObject *object)
 
   g_hash_table_destroy (priv->devices);
 
+  _clutter_id_pool_free (priv->pick_id_pool);
+
   if (priv->fps_timer != NULL)
     g_timer_destroy (priv->fps_timer);
 
@@ -2034,6 +2039,8 @@ clutter_stage_init (ClutterStage *self)
     g_array_new (FALSE, FALSE, sizeof (ClutterPaintVolume));
 
   priv->devices = g_hash_table_new (NULL, NULL);
+
+  priv->pick_id_pool = _clutter_id_pool_new (256);
 }
 
 /**
@@ -3827,3 +3834,36 @@ _clutter_stage_get_active_framebuffer (ClutterStage *stage)
 {
   return stage->priv->active_framebuffer;
 }
+
+gint32
+_clutter_stage_acquire_pick_id (ClutterStage *stage,
+                                ClutterActor *actor)
+{
+  ClutterStagePrivate *priv = stage->priv;
+
+  g_assert (priv->pick_id_pool != NULL);
+
+  return _clutter_id_pool_add (priv->pick_id_pool, actor);
+}
+
+void
+_clutter_stage_release_pick_id (ClutterStage *stage,
+                                gint32        pick_id)
+{
+  ClutterStagePrivate *priv = stage->priv;
+
+  g_assert (priv->pick_id_pool != NULL);
+
+  _clutter_id_pool_remove (priv->pick_id_pool, pick_id);
+}
+
+ClutterActor *
+_clutter_stage_get_actor_by_pick_id (ClutterStage *stage,
+                                     gint32        pick_id)
+{
+  ClutterStagePrivate *priv = stage->priv;
+
+  g_assert (priv->pick_id_pool != NULL);
+
+  return _clutter_id_pool_lookup (priv->pick_id_pool, pick_id);
+}

tests/conform/test-pick.c

@@ -13,7 +13,7 @@ struct _State
 {
   ClutterActor *stage;
   int y, x;
-  guint32 gids[ACTORS_X * ACTORS_Y];
+  ClutterActor *actors[ACTORS_X * ACTORS_Y];
   guint actor_width, actor_height;
   gboolean pass;
 };
@@ -74,7 +74,6 @@ on_timeout (State *state)
         for (x = 0; x < ACTORS_X; x++)
           {
             gboolean pass = FALSE;
-            guint32 gid;
             ClutterActor *actor
               = clutter_stage_get_actor_at_pos (CLUTTER_STAGE (state->stage),
                                                 CLUTTER_PICK_ALL,
@@ -84,8 +83,8 @@ on_timeout (State *state)
                                                 + state->actor_height / 2);
 
             if (g_test_verbose ())
-              g_print ("% 3i,% 3i / % 4i -> ",
+              g_print ("% 3i,% 3i / %p -> ",
-                       x, y, (int) state->gids[y * ACTORS_X + x]);
+                       x, y, state->actors[y * ACTORS_X + x]);
 
             if (actor == NULL)
               {
@@ -104,15 +103,14 @@ on_timeout (State *state)
               }
             else
               {
-                gid = clutter_actor_get_gid (actor);
+                if (actor == state->actors[y * ACTORS_X + x]
-                if (gid == state->gids[y * ACTORS_X + x]
                     && (test_num != 2
                         || x < 2 || x >= ACTORS_X - 2
                         || y < 2 || y >= ACTORS_Y - 2))
                   pass = TRUE;
 
                 if (g_test_verbose ())
-                  g_print ("% 10i: %s\n", gid, pass ? "pass" : "FAIL");
+                  g_print ("%p: %s\n", actor, pass ? "pass" : "FAIL");
               }
 
             if (!pass)
@@ -152,7 +150,7 @@ actor_picking (void)
 
 	clutter_container_add (CLUTTER_CONTAINER (state.stage), rect, NULL);
 
-	state.gids[y * ACTORS_X + x] = clutter_actor_get_gid (rect);
+	state.actors[y * ACTORS_X + x] = rect;
       }
 
   clutter_actor_show (state.stage);