[Bf-committers] New glLineWidth Policy

[Brecht Van Lommel]

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