Page MenuHome

Moved UI_view2_multi_grid_draw() to Gawain
ClosedPublic

Authored by Benjamin Summerton (define-private-public) on Nov 18 2016, 2:40 AM.

Diff Detail

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

Event Timeline

Benjamin Summerton (define-private-public) retitled this revision from to GL calls replaced, but it's buggy. Working on a fix.Nov 18 2016, 2:40 AM
Benjamin Summerton (define-private-public) retitled this revision from GL calls replaced, but it's buggy. Working on a fix to Moved UI_view2_multi_grid_draw() to Gawain.Nov 18 2016, 2:55 AM

Replicating loops & logic is a good first way to get the primitive counts. But it will become a nesting area for future bugs. You can calculate an upper bound then use immBeginAtMost. Keep hacking away! I spent at least 1.5 full days on the 3D view's ortho grids by the way.

Having two very similar for loops in this function doesn't seem that elegant. Can anyone recommend any alternatives?

In diff 7834 I changed this to make an estimate as per merwin's request.

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

There is a double negative here. I tested with removing it (as well as the other ones) and it didn't look like it messed with anything. Should I go ahead and submit a diff with those removed (also at lines 1486, 1521, and 1532)?

  • Makes an estimate of how many vertices are needed
  • Added a line drawing optimization
  • Removed some unecessary comments

I tested earlier and it was working but now issues with double assign of attributes.

I'll do more testing after I wrap my head around these loops.

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

Using immSkipAttrib means you can't setup color ahead of time here, have to remove this line and line 1520 to stop asserts.

  • Fixed issue causing Gawain to assert
Anthony Edlin (krash) requested changes to this revision.

I have to say, there is a lot wrong with this function. I think this is a case where just converting to gawain isn't really all that's needed because the original code isn't very good. Really have to try to understand what is going on and it's taken me a bit. Added a first pass review. There is more but have to go to work now so I'll do more tonight.

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

Few things here. I think you need to cast the ((v2d->cur.xmax) / lstep) to integers. I'm getting assert below at immBeginAtMost with vertex_count of 149. Other thing is that you can't just use "cur.xmax". If you put axis close to right of screen then xmax is close to 0, which will give you almost no vertexes. Try it and it'll assert because you go past vertex count. You probably want difference of xmax and xmin. Using totlevels is ok but it gives you way more verts then needed because verts needed for other levels is reduced. Actually it skips verts on first levels if they are on next level so you may not even need to multiply by totlevels, you'll have to test.

1563–1564

Not sure why drawing X and Y axis is inside the levels loop. Doesn't this just draw them repeatedly for no reason? It was inside the loop in the old code too, strange. Maybe I miss something.

This revision now requires changes to proceed.Nov 19 2016, 5:16 PM
Benjamin Summerton (define-private-public) marked 2 inline comments as done.
  • only draw axis once now
  • Removed the double negative
  • Fixed vertex counting
  • small simplification

This looks much cleaner than the first revision. After @Anthony Edlin (krash) accepts I'll put it in 2.8.

source/blender/editors/interface/view2d.c
1526–1527

C99 style thing -- can declare these below, first time they're used:

int i = (int)(v2d ...
float start = i * lstep;
1526–1527
for (int level = 0 ...

Delete declaration of level earlier this function.

1574

Empty (code) line between these 2 (GL) lines, for clarity.

  • Merge branch 'blender2.8' into imm.view2d.c.UI_view2d_multi_grid_draw
  • small code cleanup

I was still getting asserts in some cases, think I've found all corner cases remaining now.

source/blender/editors/interface/view2d.c
1515–1524

Can remove these, cur_xmin_positive and cur_ymin_positive

1522

You need a +1 on this for cases where cur spans just over grid lines. Simple example, let's say lstep is 40 (what it is on my setup), xmin is 30 and xmax is 90. xmax-xmin is 60. 60/40 turns into 1. But grid lines are drawn at 40 and 80 because xmin starts at 30.

1535

Need to increment "i" here if cur.xmin is positive before using it to set "start". Simple example again, converting a float to int throws away the decimal, so 1.5 becomes 1, and -1.5 becomes -1. This shows that converting to int will always round towards 0. This is fine for negative cur because it rounds up, but if cur is positive it will round down and you will be drawing the first grid line outside the cur rect.

1552

Same here, need to increment "i" if ymin > 0

  • Merge branch 'blender2.8' into imm.view2d.c.UI_view2d_multi_grid_draw
  • removed unused variables
  • added for a case where "cur spans just over grid lines"
  • Fixing a case where to need to incmrent "i"
Anthony Edlin (krash) accepted this revision.

Built and tested, seems good to go to me.

This revision is now accepted and ready to land.Nov 23 2016, 4:56 AM
Mike Erwin (merwin) accepted this revision.
Mike Erwin (merwin) closed this revision.

landed in blender2.8