UI: fix/rework 'align' computing code.
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on Oct 22 2015, 4:46 PM.

Details

Reviewers

Julian Eisel (Severin)
Campbell Barton (campbellbarton)

Commits

rBS3d39b98f94a6: New uialign code, based on 2D positions of widgets.
rB3d39b98f94a6: New uialign code, based on 2D positions of widgets.

Summary

Previoous code would simply epic fail in complex cases mixing horizontal and vertical layouts,
especially 'column-major' ones.

This ones tries to be more generic and robust.

Note that it's roughly 50% slower than previous code - but remains acceptable imho.

DISCLAIMER
Tortured Dimensions panel of Scene buttons to have a really complex aligned layout.
Those changes (and related ones in interface_template.c) are obviously not aimed at
master, they are only here to provide nice dirty test case.
Same goes for debug prints and switch with old align code, all this is to be cleaned up in
final version of the patch!

Old algorithm with stressing layout:

New algorithm with exact same stressing layout:

Diff Detail

Repository: rB Blender

Event Timeline

Bastien Montagne (mont29) updated this revision to Diff 5266.Oct 22 2015, 4:46 PM

Bastien Montagne (mont29) retitled this revision from to UI: fix/rework 'align' computing code..

Bastien Montagne (mont29) added reviewers: Julian Eisel (Severin), Campbell Barton (campbellbarton).

Bastien Montagne (mont29) updated this object.

Bastien Montagne (mont29) updated this object.Oct 22 2015, 4:58 PM

Aaron Carlisle (Blendify) awarded a token.Oct 22 2015, 5:13 PM

Merge branch 'master' into ui-align-rework

In general this is reasonable, have one concern about possible worst case O(n^2), which may seem paranoid... but think a stack of 50x aligned buttons isn't so unreasonable, and thats going to give 1225 comparisons.

Suggested solution:

Sort ButAlign by (alignnm, ymin, xmin), (Y since we're most likely to have many stacked vertically).
Loop over buttons but exit early when: (butal_other->but->rect.ymin > butal->but->rect.ymax).

(note this is similar to how our remove-vertex-doubles code works already :) )

In typical cases this will cut down comparisons,
also check order we get from panels so the button list may be close to sorted already.

source/blender/editors/interface/interface_intern.h
178	picky, for internal ifdefs UI currently uses `USE_` prefix. - maybe `USE_UIBUT_SPATIAL_ALIGN` ?

This revision now requires changes to proceed.Nov 4 2015, 1:20 PM

Great work on this, tested and mostly working as I'd expect :) (Agree with Cambo's notes of course) The only thing I find a bit weird is that buttons are stitched to checkboxes. Noticed this happens in master too, but maybe we can solve this with this patch somehow?

source/blender/editors/include/UI_interface.h
207	Typo: stiched->stitched
source/blender/editors/interface/interface.c
2999	Typo: chacks->checks
3096	Can use Doxygen \a command ... `in \a side_s1 direction` ...
3167	Typos: alorithm->algorithm, alignement->alignment
3218	Line is indented with spaces.
3219	Think you should use `i < num_buttons - 1` here? Loop below won't run for last button, but looks like below, butal_array is accessed out of bounds in case i + 1 == num_buttons.
3227	Shouldn't this be `!(butal_can_align && butal_other_can_align)`?
3248	Can be `side < TOTSIDES` ;)

Commenting on some details.

source/blender/editors/interface/interface.c
37	Note that `interface.c` is nearly 5k lines ~ mainly for interface access and defining new buttons, I think this code could be move into `interface_align.c` (since it defines own struct + flags + utility functions). For the purpose of patch review its easier to keep as is but think it'd be good to split out.
3090–3094	ascii art can be inside `<pre>...</pre>` (to skip doxy formatting), also wouldn't bother to indent (same for other ascii art in this patch). eg: http://fossies.org/dox/blender-2.76b/bmesh__queries_8c.html#a01cc24309f04206082c6b4198d46912c
3149–3159	Would put this in ifdef... `#ifdef DEBUG_PRINT`, which can be defined at top of file (annoying to change all comments to see logging output).

Thanks both for the reviews update incomming. :)

@Campbell Barton (campbellbarton) managed to get 30% speedup on biggest (and most complex) groups with some tweaks based on your suggestions - and probably even better with bigger groups (biggest one I use for tests has 25 buttons, and now runs in about 4us instead of 6us).

@Julian Eisel (Severin) This is a separated issue. Button is not stitched to the two checkboxes, but to the five other controls to the right. Currently, if a button’s border is stitched “somewhere”, both matching corners are “hard”, changing this is really out of the scope of this patch, it would require storing also stitched corners in buttons' flags, and edit of the drawing code… Possible but nothing really crucial for me here, this is mere visual glitch, nothing like disappearing buttons from old align. ;)

source/blender/editors/interface/interface.c
37	Agree, will do for final commit.
3149–3159	Well… my idea was to nuke all those prints for master commit… Those are really dev stuff, only kept until code is considered finished. :)
3219	No out-of-bound access, since we are only manipulating a pointer in `butal_other = &butal_array[i + 1]`. Nice catch though!
3227	Totally not! :) Thing is, if you have to alignable buttons separated by a (thin!) separator, you want to take into account the (button, separator) pairs, to avoid merging the two buttons together (separator is closest neighbor of both buttons).

Update from reviews, mainly limiting comparison between buttons for big groups.
Merge branch 'master' into ui-align-rework

Campbell Barton (campbellbarton) requested changes to this revision.Nov 6 2015, 3:14 PM

Campbell Barton (campbellbarton) edited edge metadata.

Campbell Barton (campbellbarton) added inline comments.

source/blender/editors/interface/interface.c
3201–3208	Since this is pixels, its probably not breaking, but this could stop qsort from working properly. Better practice not to convert float -> int here, just do `(a < b) ? -1 : 1;`