On 24 October 2011 15:58, Brian Paul <span dir="ltr"><<a href="mailto:brianp@vmware.com">brianp@vmware.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">On 10/24/2011 03:38 PM, Paul Berry wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This patch adds the bitfields InterpOverridesFlat,<br>
InterpOverridesSmooth, and InterpOverridesNoperspective to<br>
gl_fragment_program. These bitfields keep track of which fragment<br>
shader inputs are overridden with the GLSL "flat", "smooth", and<br>
"noperspective" interpolation qualifiers.<br>
</blockquote>
<br></div>
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.<br>
<br>
How about just "InterpFlat", "InterpSmooth", etc?<br></blockquote><div><br>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). <br>
</div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Or, how about an enum that indicates the interpolation mode for each fragment shader input? Something like this:<br>
<br>
enum interp_mode<br>
{<br>
INTERP_DEFAULT, /* I _think_ we need a default mode, right? */<br>
INTERP_FLAT,<br>
INTERP_SMOOTH,<br>
INTERP_NO_PERSPECTIVE<br>
};<br>
<br>
struct gl_fragment_program<br>
{<br>
...<br>
enum interp_mode InterpMode[FRAG_ATTRIB_MAX];<br>
...<br>
};<br>
<br>
<br>
This would also prevent a non-sensical state where a particular bit is accidentally set in more than one of the masks.<br></blockquote><div><br>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? 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.<br>
</div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Finally, maybe use "CONSTANT", "LINEAR" and "PERSPECTIVE" instead of "FLAT", "NO_PERSPECTIVE" and "SMOOTH". That's what we have in gallium.</blockquote><div>
<br>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".<br>
<br>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.<br>
</div></div>