[Bf-committers] New glLineWidth Policy

Fri Feb 19 01:09:48 CET 2016

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