[clutter] actor: review use of _apply_modelview_transform_recursive

[Robert Bragg <rbragg src gnome org>]

commit 1720b77d29b474aeff156633138baf7244db1567
Author: Robert Bragg <robert linux intel com>
Date:   Fri Jun 17 17:44:16 2011 +0100

    actor: review use of _apply_modelview_transform_recursive
    
    On reviewing the clutter-actor.c code using
    _apply_modelview_transform_recursive I noticed various comments stating
    that it will never call the stage's ->apply_transform vfunc to transform
    into eye coordinates, but actually looking at the implementation that's
    not true. The comments probably got out of sync with an earlier
    implementation that had that constraint. This removes the miss-leading
    comments and also updates various uses of the api where we were manually
    applying the stage->apply_transform.

 clutter/clutter-actor.c        |   30 +++++-------------------------
 clutter/clutter-paint-volume.c |   32 ++------------------------------
 clutter/clutter-texture.c      |    8 --------
 3 files changed, 7 insertions(+), 63 deletions(-)
---
diff --git a/clutter/clutter-actor.c b/clutter/clutter-actor.c
index 1d22739..3dea016 100644
--- a/clutter/clutter-actor.c
+++ b/clutter/clutter-actor.c
@@ -2039,10 +2039,6 @@ _clutter_actor_fully_transform_vertices (ClutterActor *self,
 
   g_return_val_if_fail (CLUTTER_IS_ACTOR (self), FALSE);
 
-  /* NB: _clutter_actor_apply_modelview_transform_recursive will never
-   * include the transformation between stage coordinates and OpenGL
-   * eye coordinates, we have to explicitly use the
-   * stage->apply_transform to get that... */
   stage = _clutter_actor_get_stage_internal (self);
 
   /* We really can't do anything meaningful in this case so don't try
@@ -2050,10 +2046,10 @@ _clutter_actor_fully_transform_vertices (ClutterActor *self,
   if (stage == NULL)
     return FALSE;
 
-  /* Setup the modelview */
-  cogl_matrix_init_identity (&modelview);
-  _clutter_actor_apply_modelview_transform (stage, &modelview);
-  _clutter_actor_apply_modelview_transform_recursive (self, stage, &modelview);
+  /* Note: we pass NULL as the ancestor because we don't just want the modelview
+   * that gets us to stage coordinates, we want to go all the way to eye
+   * coordinates */
+  _clutter_actor_apply_modelview_transform_recursive (self, NULL, &modelview);
 
   /* Fetch the projection and viewport */
   _clutter_stage_get_projection_matrix (CLUTTER_STAGE (stage), &projection);
@@ -2098,21 +2094,7 @@ clutter_actor_apply_transform_to_point (ClutterActor        *self,
 /* _clutter_actor_get_relative_modelview:
  *
  * Retrieves the modelview transformation relative to some ancestor
- * actor, or the stage if NULL is given for the ancestor.
- *
- * Note: This will never include the transformations from
- * stage::apply_transform since that would give you a modelview
- * transform relative to the OpenGL window coordinate space that the
- * stage lies within.
- *
- * If you need to do a full modelview + projective transform and get
- * to window coordinates then you should explicitly apply the stage
- * transform to an identity matrix and use
- * _clutter_actor_apply_modelview_transform like:
- *
- *   cogl_matrix_init_identity (&mtx);
- *   stage = _clutter_actor_get_stage_internal (self);
- *   _clutter_actor_apply_modelview_transform (stage, &mtx);
+ * actor, or eye coordinates if NULL is given for the ancestor.
  */
 /* FIXME: We should be caching the stage relative modelview along with the
  * actor itself */
@@ -2121,8 +2103,6 @@ _clutter_actor_get_relative_modelview (ClutterActor *self,
                                        ClutterActor *ancestor,
                                        CoglMatrix *matrix)
 {
-  g_return_if_fail (ancestor != NULL);
-
   cogl_matrix_init_identity (matrix);
 
   _clutter_actor_apply_modelview_transform_recursive (self, ancestor, matrix);
diff --git a/clutter/clutter-paint-volume.c b/clutter/clutter-paint-volume.c
index 2e39ac8..71dc8ee 100644
--- a/clutter/clutter-paint-volume.c
+++ b/clutter/clutter-paint-volume.c
@@ -1004,21 +1004,12 @@ _clutter_paint_volume_get_stage_paint_box (ClutterPaintVolume *pv,
 
   _clutter_paint_volume_copy_static (pv, &projected_pv);
 
-  /* NB: _clutter_actor_apply_modelview_transform_recursive will never
-   * include the transformation between stage coordinates and OpenGL
-   * eye coordinates, we have to explicitly use the
-   * stage->apply_transform to get that... */
   cogl_matrix_init_identity (&modelview);
 
   /* If the paint volume isn't already in eye coordinates... */
   if (pv->actor)
-    {
-      ClutterActor *stage_actor = CLUTTER_ACTOR (stage);
-      _clutter_actor_apply_modelview_transform (stage_actor, &modelview);
-      _clutter_actor_apply_modelview_transform_recursive (pv->actor,
-                                                          stage_actor,
-                                                          &modelview);
-    }
+    _clutter_actor_apply_modelview_transform_recursive (pv->actor, NULL,
+                                                        &modelview);
 
   _clutter_stage_get_projection_matrix (stage, &projection);
   _clutter_stage_get_viewport (stage,
@@ -1052,25 +1043,6 @@ _clutter_paint_volume_transform_relative (ClutterPaintVolume *pv,
   _clutter_paint_volume_set_reference_actor (pv, relative_to_ancestor);
 
   cogl_matrix_init_identity (&matrix);
-
-  if (relative_to_ancestor == NULL)
-    {
-      /* NB: _clutter_actor_apply_modelview_transform_recursive will never
-       * include the transformation between stage coordinates and OpenGL
-       * eye coordinates, we have to explicitly use the
-       * stage->apply_transform to get that... */
-      ClutterActor *stage = _clutter_actor_get_stage_internal (actor);
-
-      /* We really can't do anything meaningful in this case so don't try
-       * to do any transform */
-      if (G_UNLIKELY (stage == NULL))
-        return;
-
-      _clutter_actor_apply_modelview_transform (stage, &matrix);
-
-      relative_to_ancestor = stage;
-    }
-
   _clutter_actor_apply_modelview_transform_recursive (actor,
                                                       relative_to_ancestor,
                                                       &matrix);
diff --git a/clutter/clutter-texture.c b/clutter/clutter-texture.c
index b21fa06..de328f0 100644
--- a/clutter/clutter-texture.c
+++ b/clutter/clutter-texture.c
@@ -530,15 +530,7 @@ update_fbo (ClutterActor *self)
       if ((source_parent = clutter_actor_get_parent (priv->fbo_source)))
         {
           CoglMatrix modelview;
-
-          /* NB: _clutter_actor_apply_modelview_transform_recursive
-           * will never include the transformation between stage
-           * coordinates and OpenGL window coordinates, we have to
-           * explicitly use the stage->apply_transform to get that...
-           */
-
           cogl_matrix_init_identity (&modelview);
-          _clutter_actor_apply_modelview_transform (stage, &modelview);
           _clutter_actor_apply_modelview_transform_recursive (source_parent,
                                                               NULL,
                                                               &modelview);