Page MenuHome

Moved UI_view2d_grid_draw() to immediate functions
ClosedPublic

Authored by Benjamin Summerton (define-private-public) on Nov 16 2016, 7:48 AM.

Diff Detail

Repository
rB Blender
Branch
imm.view2d.c.UI_view2d_grid_draw
Build Status
Buildable 287
Build 287: arc lint + arc unit

Event Timeline

Benjamin Summerton (define-private-public) retitled this revision from to Moved UI_view2d_grid_draw() to immediate functions.Nov 16 2016, 7:48 AM

You're akrashe in IRC, right? You said you wanted to review this.

You're akrashe in IRC, right? You said you wanted to review this.

Yeah thats me.

Compile works fine. I've done a first pass review with inline comments below.

I'll check some more later after work.

Thanks for patch.

source/blender/editors/interface/view2d.c
1296

I'm not fussed about style personally, but convention is to use style of surrounding code or file. This would be one of few functions in this file that would use camel case for local variables. @Mike Erwin (merwin) can decide how picky to be.

1298

You use float here for drawColor but then use "ubv" below for UI and attribute functions. I think either switch this to unsigned char or use "fv" functions below.

1332

I don't think you need smooth color between verts of single lines. Use GPU_SHADER_FLAT_COLOR.

1333

Not sure if vertexCount is ever zero in current use of grid drawing (I tried in several editors to get zero but it never happened), but it would debug assert if it was ever possible.

Anthony Edlin (krash) requested changes to this revision.
This revision now requires changes to proceed.Nov 16 2016, 2:33 PM
Benjamin Summerton (define-private-public) marked 3 inline comments as done.
  • Merge branch 'blender2.8' into imm.view2d.c
  • accidentally was using float instead of unsgined bytes for colors
  • Should be using a flat shader instead
  • Changed naming convenstions of variables
  • Added a check to exit early if there is nothing to draw

Did some more testing last night and found that gawain asserts if vertical_minor_step is 0. You should be able to test it by taking the default screen and making the time line at the bottom really small. It asserts on line 1361 and the reason is that it doesn't like that the color attribute was set on line 1347 but never used. To fix I think you'll have to wrap the vertical minor grid lines section in a "if (step != 0)". Try it out and see what you think.

It's not an issue for the horizontal grid part because it uses (a <= step) so it always does at least one vertex. Pretty confusing.

  • Merge branch 'blender2.8' into imm.view2d.c.UI_view2d_grid_draw
  • Add a fix to for the minor vertical gridlines
Anthony Edlin (krash) accepted this revision.

Tested and seems all issues are fixed.

As a more general comment, having 4 different 2d grid drawing functions in blender seems silly. Each one has had it's quirks, and especially they show up in converting to gawain. It seems they all have gotten more hard to follow. A more general refactor for all 2d grid drawing functions that considers gawain and all the different uses of a 2d grid I think would be in order, but for now maybe it's good enough to convert to gawain only.

I can't commit so @Mike Erwin (merwin) gets last say.

This revision is now accepted and ready to land.Nov 18 2016, 3:51 AM

Performance tip: when drawing flat-shaded lines, it's more efficient to

immSkipAttrib(color)
immVertex(pos, vec1)
immAttrib(color, draw_color)
immVertex(pos, vec2)

source/blender/editors/interface/view2d.c
1329

These comments are obvious & can be deleted:
Setup Gawain
Start Rendering
Stop Rendering

  • changed variable to a better name
  • Merwin told me adding the skips would make it faster
Mike Erwin (merwin) accepted this revision.