On 25 October 2011 10:21, Brian Paul <bri...@vmware.com> wrote: > 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. Ok, after rereading your comments and discussing this with Eric, Ian, and Ken, I reconsidered and actually tried implementing this as an array of enums, as you suggested. It pushes a little bit of complication into i965, but it's not bad, and core mesa gets a lot cleaner. On balance, I think it's an improvement. So now that I've seen your perspective, you can color me convinced. I'll rebase and send out a revised patch series.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
