BLI: IndexMask: Comparison operators #117814

Iliya Katushenock · 2024-02-04T19:36:37+01:00

Iliya Katushenock commented

2024-02-04 19:36:37 +01:00

Support == and != operators for IndexMask.
This implementation is single thread and avoid any allocation.

Support `==` and `!=` operators for `IndexMask`. This implementation is single thread and avoid any allocation.

Iliya Katushenock added 1 commit 2024-02-04 19:36:45 +01:00

782878489d init

Iliya Katushenock added 3 commits 2024-02-04 22:11:03 +01:00

8adabd662d cleanup

028e96ab4f progress

add test

4b0ee0b1a0 fix one-of-error

Iliya Katushenock added this to the Core Libraries project 2024-02-04 22:11:26 +01:00

Iliya Katushenock requested review from Jacques Lucke 2024-02-04 22:11:30 +01:00

Iliya Katushenock added 1 commit 2024-02-04 22:55:26 +01:00

f788cd7c6c fix

Iliya Katushenock added 1 commit 2024-02-04 22:58:10 +01:00

b2bec439ef cleanup

Jacques Lucke requested changes 2024-02-05 16:58:34 +01:00

source/blender/blenlib/intern/index_mask.cc Outdated

						
				@ -760,2 +760,4 @@

				template void IndexMask::to_indices(MutableSpan<int64_t>) const;

				template<int Count, typename Func>

				static void foreach_sequence(std::array<IndexMask, Count> masks, Func &&func)

Jacques Lucke commented

2024-02-05 16:58:24 +01:00

Not sure I fully understand this function yet. Maybe you can add a comment and maybe also add a test for this function specifically?

Iliya Katushenock commented

2024-02-05 18:53:54 +01:00

I added comment to describe this function few more. Not sure this function can be covered by test: this function is template right now, not sure i want to move this in header or make it more generic.
I'd like to implement more general version of this function (to handle any number of any mask (offset and size) and default values) in order to make more optimized merge/diff/... functions for #117805 (as like Segment::== calls in this case). But i think this is enough for now.

I added comment to describe this function few more. Not sure this function can be covered by test: this function is template right now, not sure i want to move this in header or make it more generic. I'd like to implement more general version of this function (to handle any number of any mask (offset and size) and default values) in order to make more optimized merge/diff/... functions for #117805 (as like `Segment::==` calls in this case). But i think this is enough for now.

Iliya Katushenock added 2 commits 2024-02-05 18:47:26 +01:00

95742d02cc merge main

0e9485e93c cleanup

add comment

Iliya Katushenock added 1 commit 2024-02-05 18:56:40 +01:00

7e2e5ab152 cleanup

Iliya Katushenock requested review from Jacques Lucke 2024-02-05 18:57:02 +01:00

Jacques Lucke requested changes 2024-02-06 18:01:52 +01:00

source/blender/blenlib/intern/index_mask.cc Outdated

						
				@ -868,4 +868,104 @@ template IndexMask IndexMask::from_indices(Span<int64_t>, IndexMaskMemory &);

				template void IndexMask::to_indices(MutableSpan<int32_t>) const;

				template void IndexMask::to_indices(MutableSpan<int64_t>) const;

				/* Any mask is built from segments. Even if masks is the same, segments fragmentation might be

Jacques Lucke commented

2024-02-06 18:01:09 +01:00

I think an simple example as part of the description would make this more clear.

Iliya Katushenock commented

2024-02-06 18:46:29 +01:00

Added.

source/blender/blenlib/intern/index_mask.cc Outdated

						
				@ -871,0 +871,4 @@

				/* Any mask is built from segments. Even if masks is the same, segments fragmentation might be

				 * different. Produce re-fragmentation of each mask on the fly to have segment of each mask in some

				 * certain range and call function for them. */

				template<int Count, typename Func>

Jacques Lucke commented

2024-02-06 17:57:47 +01:00

IndexMaskNum

`IndexMaskNum`

mod_moder marked this conversation as resolved

source/blender/blenlib/intern/index_mask.cc Outdated

						
				@ -871,0 +875,4 @@

				static void foreach_sequence(std::array<IndexMask, Count> masks, Func &&func)

				{

				  BLI_assert(std::all_of(masks.begin() + 1, masks.end(), [&](const IndexMask &maks) {

				    return masks[0].size() == maks.size();

Jacques Lucke commented

2024-02-06 18:01:36 +01:00

typo: maks.

typo: `maks`.

mod_moder marked this conversation as resolved

source/blender/blenlib/intern/index_mask.cc Outdated

						
				@ -871,0 +886,4 @@

				  std::array<IndexMaskSegment, Count> segments;

				  while (segment_iter[0] != masks[0].segments_num()) {

Jacques Lucke commented

2024-02-06 17:59:24 +01:00

Where is masks[1].segments_num() taken into consideration? Looks like we could go out of range somewhere.

Where is `masks[1].segments_num()` taken into consideration? Looks like we could go out of range somewhere.

Iliya Katushenock commented

2024-02-06 18:45:33 +01:00

Added comment about this.

source/blender/blenlib/intern/index_mask.cc Outdated

						
				@ -871,0 +893,4 @@

				      }

				    }

				    int16_t min_sequence_size = std::numeric_limits<int16_t>::max();

Jacques Lucke commented

2024-02-06 17:55:26 +01:00

Maybe use max_segment_size instead?
Maybe can also call this next_common_sequence_size.

Maybe use `max_segment_size` instead? Maybe can also call this `next_common_sequence_size`.

mod_moder marked this conversation as resolved

source/blender/blenlib/intern/index_mask.cc Outdated

						
				@ -871,0 +920,4 @@

				  }

				}

				static bool operator==(const IndexMaskSegment &a, const IndexMaskSegment &b)

Jacques Lucke commented

2024-02-06 17:53:05 +01:00

Need to be a bit careful here, because IndexMaskSegment is currently actually an alias for OffsetSpan<int64_t, int16_t>, but this comparison function does not work for OffsetSpan<int64_t, int16_t> in general.

Solution could be to make it so that IndexMaskSegment derives from OffsetSpan instead.

Need to be a bit careful here, because `IndexMaskSegment` is currently actually an `alias` for `OffsetSpan<int64_t, int16_t>`, but this comparison function does not work for `OffsetSpan<int64_t, int16_t>` in general. Solution could be to make it so that `IndexMaskSegment` derives from `OffsetSpan` instead.

Iliya Katushenock commented

2024-02-06 18:10:42 +01:00

Solution could be to make it so that IndexMaskSegment derives from OffsetSpan instead.

I really want to make this inheritance at some point to gather all methods to process mask segments in segment class.

This is static operator, more specific overload instead of template operator for this type (if such will be added). I see that this might be not so clear and be error-prone, for now, this operator could be done as just function.

> Solution could be to make it so that IndexMaskSegment derives from OffsetSpan instead. I really want to make this inheritance at some point to gather all methods to process mask segments in segment class. This is static operator, more specific overload instead of template operator for this type (if such will be added). I see that this might be not so clear and be error-prone, for now, this operator could be done as just function.

mod_moder marked this conversation as resolved

Iliya Katushenock added 2 commits 2024-02-06 18:45:12 +01:00

003ed1301d Merge branch 'main' into index_mask_cmp

61fc02fd29 add comments, renaming

Iliya Katushenock requested review from Jacques Lucke 2024-02-06 18:46:31 +01:00

Iliya Katushenock added 1 commit 2024-02-06 18:48:59 +01:00

13cdb34d92 progress

Jacques Lucke requested changes 2024-02-08 16:55:15 +01:00

source/blender/blenlib/intern/index_mask.cc

						
				@ -871,0 +882,4 @@

				 * in positions space. Indices can be any.

				 */

				template<int IndexMaskNum, typename Func>

				static void foreach_sequence(std::array<IndexMask, IndexMaskNum> masks, Func &&func)

Jacques Lucke commented

2024-02-08 16:55:07 +01:00

The function generally looks ok, but I think it would still be nice if we could have tests specifically for it. I recommend exposing the function as a static method on IndexMask like so:

  /**
   * Iterate over the indices in multiple masks which have the same size. The given function is
   * called for groups of segments where each segment has the same size and comes from a different
   * input mask.
   * For example, if the input masks are (both have size 18):
   *   A: [0, 15), {20, 24, 25}
   *   B: [0, 5), [10, 15], {20, 30, 40, 50, 60, 70, 80, 90}
   * Then the function will be called multiple times, each time with two segments:
   *   1. [0, 5), [0, 5)
   *   2. [5, 10), [10, 15)
   *   3. [10, 15), {20, 30, 40, 50, 60}
   *   4. {20, 24, 25}, {70, 80, 90}
   */
  static void foreach_segment_zipped(Span<IndexMask> masks,
                                     FunctionRef<bool(Span<IndexMaskSegment> segments)> fn);

It doesn't seem necessary that the function is templated for now. Maybe that can be an optimization later if it ever becomes necessary.

The function generally looks ok, but I think it would still be nice if we could have tests specifically for it. I recommend exposing the function as a static method on `IndexMask` like so: ```cpp /** * Iterate over the indices in multiple masks which have the same size. The given function is * called for groups of segments where each segment has the same size and comes from a different * input mask. * For example, if the input masks are (both have size 18): * A: [0, 15), {20, 24, 25} * B: [0, 5), [10, 15], {20, 30, 40, 50, 60, 70, 80, 90} * Then the function will be called multiple times, each time with two segments: * 1. [0, 5), [0, 5) * 2. [5, 10), [10, 15) * 3. [10, 15), {20, 30, 40, 50, 60} * 4. {20, 24, 25}, {70, 80, 90} */ static void foreach_segment_zipped(Span<IndexMask> masks, FunctionRef<bool(Span<IndexMaskSegment> segments)> fn); ``` It doesn't seem necessary that the function is templated for now. Maybe that can be an optimization later if it ever becomes necessary.

Iliya Katushenock commented

2024-02-08 21:27:36 +01:00

Much better example case of fragmentation.

mod_moder marked this conversation as resolved

Iliya Katushenock added 2 commits 2024-02-08 21:26:03 +01:00

92fc2b4a03 Merge branch 'main' into index_mask_cmp

7547715851 progress

Iliya Katushenock added 5 commits 2024-02-09 12:21:37 +01:00

7921ae26c1 Merge branch 'main' into index_mask_cmp

b08507c7d1 cleanup

68606cdf09 progress