On 10/24/2011 05:37 PM, Paul Berry wrote:
On 24 October 2011 15:58, Brian Paul <bri...@vmware.com
<mailto:bri...@vmware.com>> wrote:
On 10/24/2011 03:38 PM, Paul Berry wrote:
This patch adds the bitfields InterpOverridesFlat,
InterpOverridesSmooth, and InterpOverridesNoperspective to
gl_fragment_program. These bitfields keep track of which fragment
shader inputs are overridden with the GLSL "flat", "smooth", and
"noperspective" interpolation qualifiers.
The names of those fields seems a little confusing to me. For
example, "InterpOverridesFlat" sounds like a field that overrides
flat shading with something else.
How about just "InterpFlat", "InterpSmooth", etc?
The meaning I was trying to convey with "overrides" is that bits are
only set in these bitfields if the interpolation mode is overridden in
GLSL. For fragment shader inputs that don't have their interpolation
mode overridden, all of the bitfields have a zero. (I had to do this
in order to avoid adding a bunch of code to the handling of fixed
function fragment shading and ARB fragment programs).
Or, how about an enum that indicates the interpolation mode for
each fragment shader input? Something like this:
enum interp_mode
{
INTERP_DEFAULT, /* I _think_ we need a default mode, right? */
INTERP_FLAT,
INTERP_SMOOTH,
INTERP_NO_PERSPECTIVE
};
struct gl_fragment_program
{
...
enum interp_mode InterpMode[FRAG_ATTRIB_MAX];
...
};
This would also prevent a non-sensical state where a particular
bit is accidentally set in more than one of the masks.
Yeah, I prefer this too, and that's essentially what I did in patch 2
of the series. To be honest I find all the bitfield stuff rather
ugly, and I would rather drop this patch entirely, but a lot of the
i965 code still relies on things being bitfields. Is it the only
driver that does?
Which bitfields are you referring to specifically? I think the ones
in gl_program are used in the gallium state tracker, but I'd have to
double-check.
If so, I suppose I might could be convinced to move
all the bitfield handling of interpolation into i965 where it won't
contaminate the other implementations.
I don't know what works best for i965 but using an enumerated type for
the interpolation modes seems cleaner for core Mesa.
Finally, maybe use "CONSTANT", "LINEAR" and "PERSPECTIVE" instead
of "FLAT", "NO_PERSPECTIVE" and "SMOOTH". That's what we have in
gallium.
Hmm, I'm ambivalent about this one. The terms "linear" and
"perspective" are certainly clearer than "noperspective" and "smooth",
but "constant" seems downright misleading for flatshaded attributes,
since they are only constant across a single primitive. I also think
there's a lot to be said for being consistent with GLSL terminology,
which is to call these things "flat", "noperspective", and "smooth".
It's not a huge deal to me either.
I also admit I'm a little surprised to hear that Gallium has already
dealt with this question. In my greps, I couldn't find any Gallium
code that was referring to the ir_variable::interpolation field, so I
assumed i965 was the first driver to try to implement interpolation
qualifiers. Did I miss something? If I did, then I probably broke
gallium when I renamed things in patch 1 of the series.
The gallium state tracker doesn't yet handle interpolation qualifiers
for Mesa's fragment programs, but we emit them when we create TGSI
fragment program input register declarations (frag pos is linear,
texcoords are perspective, etc). For implementing glShadeModel(),
however, we still rely on a rasterization state flag rather than emit
different fragment programs with different input qualifiers. That's
something I'd like to fix someday.
-Brian
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
