Part of T49043
Details
Details
Diff Detail
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
Comment Actions
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. | |
Comment Actions
- 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
Comment Actions
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.
Comment Actions
- Merge branch 'blender2.8' into imm.view2d.c.UI_view2d_grid_draw
- Add a fix to for the minor vertical gridlines
Comment Actions
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.
Comment Actions
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: | |
Comment Actions
- changed variable to a better name
- Merwin told me adding the skips would make it faster