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 <[email protected]> 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 > <[email protected]> wrote: >> On Thu, Feb 18, 2016 at 11:37 PM, Mike Erwin <[email protected]> >> 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 < >>> [email protected]> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 <[email protected]> >>>> 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 >>>> >>> [email protected] >>>> >>> http://lists.blender.org/mailman/listinfo/bf-committers >>>> >>> >>>> >> _______________________________________________ >>>> >> Bf-committers mailing list >>>> >> [email protected] >>>> >> http://lists.blender.org/mailman/listinfo/bf-committers >>>> > >>>> > >>>> > >>>> > -- >>>> > - Campbell >>>> > _______________________________________________ >>>> > Bf-committers mailing list >>>> > [email protected] >>>> > http://lists.blender.org/mailman/listinfo/bf-committers >>>> _______________________________________________ >>>> Bf-committers mailing list >>>> [email protected] >>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>> >>> _______________________________________________ >>> Bf-committers mailing list >>> [email protected] >>> http://lists.blender.org/mailman/listinfo/bf-committers > _______________________________________________ > Bf-committers mailing list > [email protected] > http://lists.blender.org/mailman/listinfo/bf-committers _______________________________________________ Bf-committers mailing list [email protected] http://lists.blender.org/mailman/listinfo/bf-committers
