Re: [11/23] wined3d: Implement primitive restart index.

[Matteo Bruni]

2016-11-13 12:35 GMT-06:00 Andrew Wesie :
> Signed-off-by: Andrew Wesie
>
> dlls/wined3d/directx.c     |  3 +++
>  dlls/wined3d/glsl_shader.c | 14 ++++++++++++++
>  dlls/wined3d/wined3d_gl.h  |  2 ++
>  3 files changed, 19 insertions(+)
>
> diff --git a/dlls/wined3d/directx.c b/dlls/wined3d/directx.c
> index a8c8c9f..c930cf6 100644
> --- a/dlls/wined3d/directx.c
> +++ b/dlls/wined3d/directx.c
> @@ -121,6 +121,7 @@ static const struct wined3d_extension_map gl_extension_map[] =
>      {"GL_ARB_draw_elements_base_vertex",    ARB_DRAW_ELEMENTS_BASE_VERTEX },
>      {"GL_ARB_draw_instanced",               ARB_DRAW_INSTANCED            },
>      {"GL_ARB_ES2_compatibility",            ARB_ES2_COMPATIBILITY         },
> - {"GL_ARB_ES3_compatibility",            ARB_ES3_COMPATIBILITY         },
> {"GL_ARB_explicit_attrib_location",     ARB_EXPLICIT_ATTRIB_LOCATION  },
> {"GL_ARB_fragment_coord_conventions",   ARB_FRAGMENT_COORD_CONVENTIONS},
> {"GL_ARB_fragment_program",             ARB_FRAGMENT_PROGRAM          },
>
> @@ -228,6 +229,7 @@ static const struct wined3d_extension_map gl_extension_map[] =
>      {"GL_NV_half_float",                    NV_HALF_FLOAT                 },
>      {"GL_NV_light_max_exponent",            NV_LIGHT_MAX_EXPONENT         },
>      {"GL_NV_point_sprite",                  NV_POINT_SPRITE               },
> - {"GL_NV_primitive_restart",             NV_PRIMITIVE_RESTART          },
> {"GL_NV_register_combiners",            NV_REGISTER_COMBINERS         },
> {"GL_NV_register_combiners2",           NV_REGISTER_COMBINERS2        },
> {"GL_NV_texgen_reflection",             NV_TEXGEN_REFLECTION          },
>
> @@ -3125,6 +3127,7 @@ static void load_gl_funcs(struct wined3d_gl_info *gl_info)
>      USE_GL_FUNC(glMapBuffer)                /* OpenGL 1.5 */
>      USE_GL_FUNC(glPointParameteri)          /* OpenGL 1.4 */
>      USE_GL_FUNC(glPointParameteriv)         /* OpenGL 1.4 */
> - USE_GL_FUNC(glPrimitiveRestartIndex)
> USE_GL_FUNC(glShaderSource)             /* OpenGL 2.0 */
> USE_GL_FUNC(glStencilFuncSeparate)      /* OpenGL 2.0 */
> USE_GL_FUNC(glStencilOpSeparate)        /* OpenGL 2.0 */

AFAICS this function was added in OpenGL 3.1 and there is no specific
ARB extension for it. Maybe adding a new WINED3D_GL_ entry to flag the
availability of the function is a good idea.
Also you probably want to MAP_GL_FUNCTION() the NV variant to the core
one and add an entry for GL_ARB_ES3_compatibility to core_extensions.
> diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c
> index f136536..27a5533 100644
> --- a/dlls/wined3d/glsl_shader.c
> +++ b/dlls/wined3d/glsl_shader.c
> @@ -8613,6 +8613,20 @@ static void shader_glsl_init_context_state(struct wined3d_context *context)
>  gl_info->gl_ops.gl.p_glEnable(GL_PROGRAM_POINT_SIZE);
>  checkGLcall("GL_PROGRAM_POINT_SIZE");
> - if (gl_info->supported[ARB_ES3_COMPATIBILITY])
> - {
> -
>    /* We prefer this method because it correctly handles 16-bit and 32-bit indices. */
> -
>    gl_info->gl_ops.gl.p_glEnable(GL_PRIMITIVE_RESTART_FIXED_INDEX);
> -
>    checkGLcall("GL_PRIMITIVE_RESTART_FIXED_INDEX");
> - }
> - else if (gl_info->supported[NV_PRIMITIVE_RESTART] && GL_EXTCALL(glPrimitiveRestartIndex))
> - {
> -
>    /* FIXME: Does this handle 16-bit indices correctly? */
> -
>    GL_EXTCALL(glPrimitiveRestartIndex(0xFFFFFFFF));
> -
>    checkGLcall("glPrimitiveRestartIndex");
> -
>    gl_info->gl_ops.gl.p_glEnable(GL_PRIMITIVE_RESTART);
> -
>    checkGLcall("GL_PRIMITIVE_RESTART");

Probably not, but nothing prevents you to use 0xffff for 16-bit
indices and force re-setting the restart index when changing between
16-bit and 32-bit indices.

[11/23] wined3d: Implement primitive restart index.

[Henri Verbeet]

On 13 November 2016 at 23:42, Matteo Bruni  wrote:
> AFAICS this function was added in OpenGL 3.1 and there is no specific
> ARB extension for it. Maybe adding a new WINED3D_GL_ entry to flag the
> availability of the function is a good idea.
> Also you probably want to MAP_GL_FUNCTION() the NV variant to the core
> one and add an entry for GL_ARB_ES3_compatibility to core_extensions.

Do we need the NV extension? I'd somewhat expect any supported driver
that has NV_primitive_restart to also have ARB_ES3_compatibility, but
I don't have a good overview.

[11/23] wined3d: Implement primitive restart index.

[Matteo Bruni]

, if we care about it2016-11-14 8:52 GMT-06:00 Henri Verbeet
:
> On 13 November 2016 at 23:42, Matteo Bruni  wrote:
> > AFAICS this function was added in OpenGL 3.1 and there is no specific
> > ARB extension for it. Maybe adding a new WINED3D_GL_ entry to flag the
> > availability of the function is a good idea.
> > Also you probably want to MAP_GL_FUNCTION() the NV variant to the core
> > one and add an entry for GL_ARB_ES3_compatibility to core_extensions.
>
> Do we need the NV extension? I'd somewhat expect any supported driver
> that has NV_primitive_restart to also have ARB_ES3_compatibility, but
> I don't have a good overview.

Me neither. A quick look at
https://people.freedesktop.org/~imirkin/glxinfo suggests that's a
valid expectation on Mesa for GPUs which can potentially support SM4.
Apparently macOS doesn't report neither ARB_ES3_compatibility nor
NV_primitive_restart, though. The only option there is to use OpenGL
3.1 glPrimitiveRestartIndex() without PRIMITIVE_RESTART_FIXED_INDEX
i.e. essentially the NV codepath.

That said, it would be easier to just drop the NV_ part altogether for
the time being and add a pre - ARB_ES3_compatibility codepath later
on, if and when it will make sense.