[Bf-committers] New glLineWidth Policy

[Mike Erwin]

For 2.77 what is left to fix? If it's a small set I say fix those and keep
the change. (but of course I would say that) It gets us closer to clean GL
usage.


I agree that plenty of state should have a default value. Blending
(disabled), line smooth (disabled), line stipple (disabled) etc. And
polygon mode (filled, not outline)! Because these are only changed in
special cases.

Point size and line width are specified pretty much every time you want to
draw, so I think maintaining a default value is counterproductive there.
Really it's a fuzzy boundary (a wide antialiased line?) separating what
should or shouldn't have a default value.

The GPU_data_request branch has a declarative state-tied-to-drawing API
that was a joy to use. Basically you start with a LineDrawState object
which starts with defaults, change what you want, then pass it to the draw
command. That style API is cleaner in C++ but the C version implemented
worked fine. You know exactly what will be drawn and can't mess it up,
totally independent of what was drawn before. It minimized calls to GL &
worked for nested drawing too.


On an admin / phabricator note, I've never gotten emails about the concern
raised or new posts on that concern. Severin told me about it on IRC. Is
there some button or setting I overlooked?

Mike Erwin
musician, naturalist, pixel pusher, hacker extraordinaire

On Thu, Feb 18, 2016 at 4:34 PM, Brecht Van Lommel <
brechtvanlommel at pandora.be> wrote:

> I replied to the commit, but basically my opinion is that if we can't
> be reasonably sure that we can fix all issues before 2.77, then we
> should just revert these commits now.
>
> I think explicitly specifying the line width is fine, but then let's
> design a line drawing API such that you can't possibly make the
> mistake of forgetting to specify the line width, by making it a
> parameter to the line drawing API function(s). As long as we are
> refactoring OpenGL code I prefer code to follow the convention that
> OpenGL state should always revert to the default state as specified in
> GPU_state_init(), specifically because it's difficult to understand
> and track all the hidden assumptions in the existing code. Even if we
> fix all cases in Blender we might break addons that do custom OpenGL
> drawing.
>
> I think that's a general principle we should follow, not just for line
> widths but for most OpenGL state. Think about line stipple, line
> smooth, do we also want to disable those everywhere before drawing
> lines? We are already assuming OpenGL is in the default state for most
> things, I don't think line width should be the exception.
>
>
> On Thu, Feb 18, 2016 at 9:44 PM, Campbell Barton <ideasman42 at gmail.com>
> wrote:
> > Hi, think its OK to keep this policy change.
> >
> > Even though its been a bit of a hassle to clear up all the
> > line-width's the past few weeks.
> > Before this - we would run into occasional bugs where the state wasn't
> > properly reset.
> > Finding out which function left the state unset could be quite
> > involved ... especially with a lot of nested calls in the drawing
> > code, where its not always clear at what point the state should be
> > reset.
> >
> > Even though the number of calls to glLineWidth may be higher,
> > (since previous code assumed width of 1 everywhere),
> > the driver *should* check if the width is the same as the previous,
> > and exit-early
> > (verified with Mesa and NVidia's drivers).
> >
> > On Fri, Feb 19, 2016 at 6:32 AM, Mike Erwin <significant.bit at gmail.com>
> wrote:
> >> Hey Julian, thanks for bringing this up for discussion. The intentions
> of
> >> that recent change were twofold, and you summed up the difference
> between
> >> the new and old policy perfectly.
> >>
> >> First intention was to reduce GL state changes -- which you say might
> not
> >> be reduced. And more importantly to make each bit of line-drawing code
> >> responsible for its own line width. To me it's way more paranoid for
> each
> >> site to reset it back to 1.0 just in case some other unrelated draw call
> >> forgets to set its own line width. Specifying state at the draw call
> site
> >> is also more Vulkan-friendly, whenever we get to that.
> >>
> >> I did a similar change set with point size a week or two before and it
> >> seemed to work really well. Lines are drawn in many more places (and
> more
> >> ways) so some things were not caught and updated to the new policy.
> >>
> >> So yes, let's review whether or not the new policy is a good one.
> Naturally
> >> I'm on Team Yes!
> >>
> >> Mike Erwin
> >> musician, naturalist, pixel pusher, hacker extraordinaire
> >>
> >> On Thu, Feb 18, 2016 at 1:53 PM, Julian Eisel <eiseljulian at gmail.com>
> wrote:
> >>
> >>> Hey there,
> >>>
> >>> We talked about this in IRC before, but I'd like to get a final
> >>> conclusion on this topic and give other devs the chance to raise their
> >>> voice.
> >>>
> >>> In a recent commit [1] we changed our policy for glLineWidth calls.
> >>> Previously, convention was to always reset to default (1.0) after
> >>> drawing with non-default width. New convention is to always ensure
> >>> line width is set to correct value before drawing lines (so there
> >>> basically is no default state anymore).
> >>>
> >>> The changed policy caused quite a few regressions, which we can fix of
> >>> course - issue is finding them. The main reasons I'm not sure about
> >>> this however, are in the following:
> >>> Although it might look like state changes were reduced, I'm afraid
> >>> we'll end up with more state changes than before, since we do much
> >>> more line draw calls than glLineWidth calls to change to non-default
> >>> (>400 vs <100). Campbell made the point that we shouldn't need to be
> >>> too paranoid, adding glLineWidth all over, but I guess a fair amount
> >>> of paranoia would actually be needed since it's often hard to predict
> >>> all possible previous line-width states. And not to forget, when
> >>> adding code with changed line-width you also had to check possible
> >>> following draw calls.
> >>> Another point Campbell made, is that old policy would be hard to
> >>> follow for nested draw calls. Although I agree it's a valid point, we
> >>> managed to do it all the time, so it's obviously doable (Sergey added
> >>> we should avoid nested draw calls anyway).
> >>>
> >>> So IMHO, this is too much hassle, and output isn't guaranteed to be
> worth
> >>> it...
> >>> All that said, I could live with the new convention of course, but
> >>> let's review it first again :)
> >>>
> >>> [1] https://developer.blender.org/rBSe25ba162c0b
> >>>
> >>> Cheers,
> >>> - Julian -
> >>> _______________________________________________
> >>> Bf-committers mailing list
> >>> Bf-committers at blender.org
> >>> http://lists.blender.org/mailman/listinfo/bf-committers
> >>>
> >> _______________________________________________
> >> Bf-committers mailing list
> >> Bf-committers at blender.org
> >> http://lists.blender.org/mailman/listinfo/bf-committers
> >
> >
> >
> > --
> > - Campbell
> > _______________________________________________
> > Bf-committers mailing list
> > Bf-committers at blender.org
> > http://lists.blender.org/mailman/listinfo/bf-committers
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers
>