[Bf-committers] New glLineWidth Policy

[Julian Eisel]

Mike, you didn't get notified cause I accidentally commented on the
commit of the staging repo (see rBS sha1 prefix). I didn't get any
notifications either.

On 19 February 2016 at 01:12, Brecht Van Lommel
<brechtvanlommel at pandora.be> wrote:
> Here's the latest report by the way:
> https://developer.blender.org/T47454
>
> On Fri, Feb 19, 2016 at 1:09 AM, Brecht Van Lommel
> <brechtvanlommel at pandora.be> wrote:
>> On Thu, Feb 18, 2016 at 11:37 PM, Mike Erwin <significant.bit at gmail.com> wrote:
>>> 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.
>>
>> But it also breaks addon backwards compatibility, which I don't think
>> is intentional for 2.77?
>>
>> Up to now there have been multiple fixes based on bug reports from
>> users. If we rely on just bug reports then there's likely more corner
>> cases that are still broken. If we keep these commits then I think
>> someone should go over all GL_LINE drawing and verify that it's
>> properly setting the line widths.
>>
>>> 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?
>>
>> Not sure, you could check the email preferences in your account settings?
>>
>>> 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
>>>>
>>> _______________________________________________
>>> 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