Details

Reviewers

Commits

rBcfd7b4d1cd48: BLI: New 'BLI_array_iter_spiral_square'

Summary

This is a part of the D10518 (which got really big but deserves attention as it seems to fix a problem with Grease Pencil).

This new function replaces some of the logic in DRW_select_buffer_find_nearest_to_point that traverses a buffer in a spiral path to search for a closer pixel.

The performance is practically the same (perhaps a difference of 0.01x).

So advantages:

It can be used in other places avoiding duplicate code.
Works with any rectangle shape (not just a square).
Operates on any type of data (float, int, array of arrays).

Disadvantages:

less readable.

Diff Detail

Repository: rB Blender

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Feb 26 2021, 5:11 PM

Germano Cavalcante (mano-wii) created this revision.

Harbormaster completed remote builds in B13132: Diff 34464.Feb 26 2021, 5:11 PM

Tweaks to improve performance

Now the current solution is more efficient corresponding to 0.9 of the time of the original function.

Harbormaster completed remote builds in B13138: Diff 34472.Feb 26 2021, 9:35 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Feb 26 2021, 9:37 PM

Remove redundant check
Cleanup: height before width
Update comments

Harbormaster completed remote builds in B13140: Diff 34475.Feb 26 2021, 9:44 PM

Cleanup comments

The memory cache confused me in the result of the previous profiler.
In fact, the performance of both is practically the same.

Harbormaster completed remote builds in B13150: Diff 34490.Feb 27 2021, 4:34 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Feb 27 2021, 4:36 PM

Brecht Van Lommel (brecht) requested changes to this revision.Mar 1 2021, 4:11 PM

Brecht Van Lommel (brecht) added inline comments.

source/blender/blenlib/intern/array_utils.c
325–326	Why flip the x/y order? I'm having a hard time reading the code since I have to mentally flip the axes all the time.
330	This stride handling is too complicated. All you need is the size of an element, and you can multiply with that at the very end when calling `test_fn`, using it earlier makes things hard to understand.
379–380	Use start/end rather then src/dst, the latter is usually used to copy something from src to dst, not for iteration.
391–398	Keep it simple: CLAMP(offset_src, 0, limits[axis]); CLAMP(offset_dst, 0, limits[axis]);
412	Computing `step_max` in advance looks tricky, can you terminate in a way similar to the old code, but checking both axes? if ((offset[0] < 0 \|\| offset[0] > limits[0]) && (offset[1] < 0 \|\| offset[1] > limits[1]))

This revision now requires changes to proceed.Mar 1 2021, 4:11 PM

Performance is not an issue here, even if this was 2x slower I doubt it would matter.

Rebase on master
Use elem_size instead of arr_stride[2]
Rename offset_src to offset_iter and offset_dst to offset_end
Simplify CLAMP
Do not compute step_max just check the limits

Harbormaster completed remote builds in B13181: Diff 34610.Mar 1 2021, 9:13 PM

Germano Cavalcante (mano-wii) added inline comments.Mar 1 2021, 9:14 PM

source/blender/blenlib/intern/array_utils.c
325–326	In fact, using the x, y order can be useful when, for example, the mouse coordinates are passed. But for most cases, the value of a buffer is obtained as follows: value = buff[h][w] Having the `h` before the `w`. If we consider `h` to be a height that is vertical and `w` to be a width (horizontal), the expected order to read a value from a buffer is `buff[y][x]`. But this is not mandatory. In fact, I had set the order x, y before.
330	(...) and you can multiply with that at the very end when calling `test_fn`, using it earlier makes things hard to understand. I feel that the patch will have to change a lot if it is to implement another way of dealing with the `offsets`. In this case, the `offset_other` here can be the `x` or the `y`: test_fn(arr + offset_other + offset_iter, user_data)

The new test for return false does not work in all possible cases.

I think the order should be made consistent, but that can be done without me reviewing it.

source/blender/blenlib/intern/array_utils.c
325–326	Please use the standard x,y and w,h order we use everywhere else in Blender.

Redo the code to make it more efficient and explanatory

This revision is now accepted and ready to land.Mar 8 2021, 11:59 PM

Harbormaster completed remote builds in B13346: Diff 34915.Mar 8 2021, 11:59 PM

Germano Cavalcante (mano-wii) marked 2 inline comments as done.Mar 9 2021, 12:00 AM

Closed by commit rBcfd7b4d1cd48: BLI: New 'BLI_array_iter_spiral_square' (authored by Germano Cavalcante (mano-wii)). · Explain WhyMar 9 2021, 8:05 PM

This revision was automatically updated to reflect the committed changes.

Germano Cavalcante (mano-wii) added a commit: rBcfd7b4d1cd48: BLI: New 'BLI_array_iter_spiral_square'.

BLI: New BLI_array_iter_spiral_square
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 34986

source/blender/blenlib/BLI_array_utils.h

source/blender/blenlib/intern/array_utils.c

source/blender/draw/intern/draw_select_buffer.c

BLI: New BLI_array_iter_spiral_squareClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 34986

source/blender/blenlib/BLI_array_utils.h

source/blender/blenlib/intern/array_utils.c

source/blender/draw/intern/draw_select_buffer.c

BLI: New BLI_array_iter_spiral_square
ClosedPublic
Actions

Revision Contents
Changeset List