[Mesa-dev] [PATCH] glsl: add support for gl_InstanceID

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/10/2011 03:30 PM, Marcin Slusarz wrote:
> ...by copying support for gl_InstanceIDARB, but without "#extension" check,
> because EXT_draw_instanced spec does not say anything about it (as opposed
> to ARB_draw_instanced / gl_InstanceIDARB) and NVIDIA driver already allow it

NAK for a couple reasons.

This extension depends on EXT_gpu_shader4, which we don't support.
There's no #extension bit required for this extension because the GLSL
changes are implemented by EXT_gpu_shader4.  Without EXT_draw_instanced,
gl_InstanceID always reads 0.

Note that EXT_gpu_shader4 says that a #extension line is required to use
the features of the extension, including gl_InstanceID.  If their driver
allows gl_InstanceID without a #extension line, it is a bug in their driver.

If OilRush is trying to use EXT_gpu_shader4 features without enabling
EXT_gpu_shader4 (or at least checking for the extension!) or using GLSL
1.30, it's a bug in OilRush.

In addition, this patch enables gl_InstanceID even if the driver doesn't
support it.

> ---
> I'm not sure this is correct.
> 
> With this patch applied (+ merged floating branch) I can run OilRush on nv50
> (run == watch the game crash in a few seconds ;)
> 
>  src/glsl/ir_variable.cpp |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/src/glsl/ir_variable.cpp b/src/glsl/ir_variable.cpp
> index 18a3e0f..bd776ee 100644
> --- a/src/glsl/ir_variable.cpp
> +++ b/src/glsl/ir_variable.cpp
> @@ -34,6 +34,10 @@ static void
>  generate_ARB_draw_instanced_variables(exec_list *,
>                                        struct _mesa_glsl_parse_state *,
>                                        bool, _mesa_glsl_parser_targets);
> +static void
> +generate_EXT_draw_instanced_variables(exec_list *,
> +                                      struct _mesa_glsl_parse_state *,
> +                                      bool, _mesa_glsl_parser_targets);
>  
>  static ir_variable *
>  add_variable(const char *name, enum ir_variable_mode mode, int slot,
> @@ -335,6 +339,8 @@ initialize_vs_variables(exec_list *instructions,
>     if (state->ARB_draw_instanced_enable)
>        generate_ARB_draw_instanced_variables(instructions, state, false,
>                                              vertex_shader);
> +   generate_EXT_draw_instanced_variables(instructions, state, false,
> +                                            vertex_shader);
>  }
>  
>  
> @@ -454,6 +460,25 @@ generate_ARB_draw_instanced_variables(exec_list *instructions,
>     }
>  }
>  
> +static void
> +generate_EXT_draw_instanced_variables(exec_list *instructions,
> +                                      struct _mesa_glsl_parse_state *state,
> +                                      bool warn,
> +                                      _mesa_glsl_parser_targets target)
> +{
> +   /* gl_InstanceID is only available in the vertex shader.
> +    */

Some additional commentary is necessary here.  gl_InstanceID is only
available in the vertex shader in EXT_gpu_shader4.  In GLSL 1.50+ or
ARB_geometry_shader4 (only) it is also available in the geometry shader.

> +   if (target == vertex_shader) {
> +      ir_variable *const inst =
> +         add_variable("gl_InstanceID", ir_var_system_value,
> +                      SYSTEM_VALUE_INSTANCE_ID,
> +                      glsl_type::int_type, instructions, state->symbols);
> +
> +      if (warn)
> +         inst->warn_extension = "GL_EXT_draw_instanced";
> +   }
> +}
> +
>  
>  static void
>  generate_ARB_shader_stencil_export_variables(exec_list *instructions,

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk1+mq4ACgkQX1gOwKyEAw8cYgCfY2fQyprwyHHSKQmt/C2321/e
2+QAnApkWC9SrNocLmJjI36GOeHHGXks
=WPdS
-----END PGP SIGNATURE-----