Re: clutter-mash: LightSet GLSL Missing decelerations (clutter-devel)

[Neil Roberts <neil linux intel com>]

From: neil linux intel com

Hi,

> I'm not sure if this project (clutter-mash) is still active, or if
> this is the right place to seek help but I'm having problems using
> Lights in mash. It seems the GLSL code is missing some variables.

I think this is because Mash was relying on certain behaviour of Cogl
that it maybe shouldn't have, and now Cogl has changed so it doesn't
work any more. The problem is that Mash is using the old CoglProgram
approach to using GLSL in Clutter and this only provides a way to
replace the entire of either the vertex or fragment processing. This
also means that it has to replicate the builtin texture coordinate
processing which means copying the texture coordinates from an
attribute to a varying in the vertex shader. This ought to be done for
each layer in the material. However, the code that generates the
shader ignores the material and just hard-codes a single set of
texture coordinates. This now breaks if there are no layers on the
material because Cogl won't declare the attribute and varying for
layers that aren't used. This used to work with previous versions of
Cogl because instead of declaring its own varyings and attributes the
cogl_* names were actually aliases for the gl_* builtins which are
always defined.

Here is a slightly hacky patch that would fix it by making it look
at the layers on the material when generating the program. A better
fix would be to make Mash use CoglSnippets instead because then it can
let Cogl handle the texture coordinate processing. If anyone is
interested in doing that change I would be very grateful and would be
happy to help. It would however require changes to Mash's public API
because some of the public functions return a CoglProgram handle.

Regards,
- Neil

------- >8 --------------- (use git am --scissors to automatically chop here)

From: Neil Roberts <bpeeluk yahoo co uk>
Subject: light-set: Don't reference layers that aren't in the material

Cogl has changed so that it no longer uses the builtin GLSL variables
for the texture coordinate varyings and attributes. This breaks Mash
because it was previously hard-coding the use of a single texture
attribute and varying. However if the material has no layers then this
attribute and varying will no longer exist. To work around the problem
this patch makes it look at the layer indices whenever it generates a
program and only generate the code for those layers.

This is a bit of a hack. A better solution would be to convert the
code to use CoglSnippets instead of the deprecated CoglPrograms.
---
 mash/mash-light-set.c | 139 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 118 insertions(+), 21 deletions(-)

diff --git a/mash/mash-light-set.c b/mash/mash-light-set.c
index 30c83d0..15d5bd3 100644
--- a/mash/mash-light-set.c
+++ b/mash/mash-light-set.c
@@ -59,6 +59,9 @@
 #include <config.h>
 #endif
 
+#define CLUTTER_ENABLE_EXPERIMENTAL_API
+#define COGL_ENABLE_EXPERIMENTAL_API
+
 #include <clutter/clutter.h>
 
 #include "mash-light-set.h"
@@ -127,6 +130,11 @@ struct _MashLightSetPrivate
 {
   CoglHandle program;
 
+  /* This is the layer indices that the pipeline contained the last
+   * time the program was generated. If these change then we need to
+   * regenerate the program */
+  GArray *layer_indices;
+
   GSList *lights;
 
   guint repaint_func_id;
@@ -161,6 +169,8 @@ mash_light_set_init (MashLightSet *self)
 
   priv->repaint_func_id =
     clutter_threads_add_repaint_func (mash_light_set_repaint_func, self, NULL);
+
+  priv->layer_indices = g_array_new (FALSE, FALSE, sizeof (int));
 }
 
 static void
@@ -191,6 +201,8 @@ mash_light_set_finalize (GObject *object)
   if (priv->program)
     cogl_handle_unref (priv->program);
 
+  g_array_free (priv->layer_indices, TRUE);
+
   G_OBJECT_CLASS (mash_light_set_parent_class)->finalize (object);
 }
 
@@ -207,6 +219,23 @@ mash_light_set_new (void)
   return g_object_new (MASH_TYPE_LIGHT_SET, NULL);
 }
 
+static void
+add_layer_indices (MashLightSet *light_set,
+                   GString *string)
+{
+  MashLightSetPrivate *priv = light_set->priv;
+  int i;
+
+  for (i = 0; i < priv->layer_indices->len; i++)
+    {
+      int layer_index = g_array_index (priv->layer_indices, int, i);
+
+      g_string_append_printf (string,
+                              "  cogl_tex_coord%i_out = cogl_tex_coord%i_in;\n",
+                              layer_index, layer_index);
+    }
+}
+
 static CoglHandle
 mash_light_set_get_program (MashLightSet *light_set)
 {
@@ -268,15 +297,17 @@ mash_light_set_get_program (MashLightSet *light_set)
                            main_source->str,
                            main_source->len);
       /* Perform the standard vertex transformation and copy the
-         texture coordinates. FIXME: This is limited to CoglMaterials
-         that only have one layer. Hopefully this could be fixed when
-         Cogl has a way to insert shader snippets rather than having
-         to replace the whole pipeline. */
+         texture coordinates. FIXME: Ideally this could should be
+         updated to use CoglSnippets so that it doesn't have to do
+         this. */
       g_string_append (uniform_source,
                        "  cogl_position_out =\n"
                        "    cogl_modelview_projection_matrix *\n"
-                       "    cogl_position_in;\n"
-                       "  cogl_tex_coord_out[0] = cogl_tex_coord_in;\n"
+                       "    cogl_position_in;\n");
+
+      add_layer_indices (light_set, uniform_source);
+
+      g_string_append (uniform_source,
                        "}\n");
 
       full_source = g_string_free (uniform_source, FALSE);
@@ -321,6 +352,82 @@ mash_light_set_get_program (MashLightSet *light_set)
   return priv->program;
 }
 
+static void
+mash_light_set_dirty_program (MashLightSet *light_set)
+{
+  MashLightSetPrivate *priv = light_set->priv;
+
+  /* If we've added or removed a light then we need to regenerate the
+     shader */
+  if (priv->program != COGL_INVALID_HANDLE)
+    {
+      cogl_handle_unref (priv->program);
+      priv->program = COGL_INVALID_HANDLE;
+    }
+}
+
+typedef struct
+{
+  MashLightSet *light_set;
+  CoglPipeline *pipeline;
+  gboolean is_matching;
+  int index_num;
+} UpdateLayerIndicesData;
+
+static CoglBool
+update_layer_indices_cb (CoglPipeline *pipeline,
+                         int layer_index,
+                         void *user_data)
+{
+  UpdateLayerIndicesData *data = user_data;
+  MashLightSetPrivate *priv = data->light_set->priv;
+
+  if (data->is_matching)
+    {
+      if (data->index_num >= priv->layer_indices->len ||
+          g_array_index (priv->layer_indices,
+                         int,
+                         data->index_num) != layer_index)
+        {
+          g_array_set_size (priv->layer_indices, data->index_num);
+          data->is_matching = FALSE;
+        }
+    }
+
+  if (!data->is_matching)
+    g_array_append_val (priv->layer_indices, layer_index);
+
+  data->index_num++;
+
+  return TRUE;
+}
+
+static void
+update_layer_indices (MashLightSet *light_set,
+                      CoglHandle material)
+{
+  MashLightSetPrivate *priv = light_set->priv;
+  UpdateLayerIndicesData data;
+
+  data.light_set = light_set;
+  data.pipeline = COGL_PIPELINE (material);
+  data.is_matching = TRUE;
+  data.index_num = 0;
+
+  cogl_pipeline_foreach_layer (data.pipeline,
+                               update_layer_indices_cb,
+                               &data);
+
+  /* If the layer indices have changed then we need to regenerate the
+   * program */
+  if (!data.is_matching ||
+      data.index_num != priv->layer_indices->len)
+    {
+      g_array_set_size (priv->layer_indices, data.index_num);
+      mash_light_set_dirty_program (light_set);
+    }
+}
+
 /**
  * mash_light_set_begin_paint:
  * @light_set: A #MashLightSet instance
@@ -350,9 +457,13 @@ mash_light_set_begin_paint (MashLightSet *light_set,
                             CoglHandle material)
 {
   MashLightSetPrivate *priv = light_set->priv;
-  CoglHandle program = mash_light_set_get_program (light_set);
+  CoglHandle program;
   int i;
 
+  update_layer_indices (light_set, material);
+
+  program = mash_light_set_get_program (light_set);
+
   if (priv->uniforms_dirty)
     {
       GSList *l;
@@ -457,20 +568,6 @@ mash_light_set_repaint_func (gpointer data)
   return TRUE;
 }
 
-static void
-mash_light_set_dirty_program (MashLightSet *light_set)
-{
-  MashLightSetPrivate *priv = light_set->priv;
-
-  /* If we've added or removed a light then we need to regenerate the
-     shader */
-  if (priv->program != COGL_INVALID_HANDLE)
-    {
-      cogl_handle_unref (priv->program);
-      priv->program = COGL_INVALID_HANDLE;
-    }
-}
-
 /**
  * mash_light_set_add_light:
  * @light_set: A #MashLightSet instance
-- 
1.8.3.1