Fix: False positives/negatives in curves_selection.cc#contains #116834

Thomas Barlow · 2024-01-06T05:06:51+01:00

Thomas Barlow commented

2024-01-06 05:06:51 +01:00

The last element of each 2048-element section was being skipped because
the size of an IndexRange was being calculated like
range.last() - range.start(), but it should have been like
range.one_after_last() - range.start(). For example, the IndexRange
starting at 0 with a size of 3 contains the values 0, 1 and 2,
so .start() == 0, .last() == 2 and .one_after_last() == 3.

False positives could occur because the entirety of the values array
was always being checked, even when only the first few elements were
initialized. An end iterator matching the end of the initialized
elements is now used instead of the end of the array itself.

The value argument was ignored by some code paths, which instead
always checked for true. This didn't cause any issues currently,
because all uses of #contains are searching for true, but this may not
be the case in the future.

In Curves Sculpt mode, Select>Random is supposed to select all curve points in advance if run on an empty selection.
During my testing for !116835, I noticed that, occasionally, performing Select>Random on an empty selection would not run the code to select all curve points in advance, the cause being that uninitialized parts of the values array were being checked too, which could sometimes cause Select>Random to think some curve points were selected even when there weren't any. The other fixes have come about from me trying to understand and debug the issue.

I couldn't seem to get the false positives from accessing uninitialized parts of the values array to occur in a debug build, making me wonder if maybe debug builds force array initialization, though I couldn't try Select>Random very many times due to it being much slower on a debug build.

The last element of each 2048-element section was being skipped because the size of an `IndexRange` was being calculated like `range.last() - range.start()`, but it should have been like `range.one_after_last() - range.start()`. For example, the `IndexRange` starting at `0` with a size of `3` contains the values `0`, `1` and `2`, so `.start() == 0`, `.last() == 2` and `.one_after_last() == 3`. False positives could occur because the entirety of the `values` array was always being checked, even when only the first few elements were initialized. An end iterator matching the end of the initialized elements is now used instead of the end of the array itself. The `value` argument was ignored by some code paths, which instead always checked for `true`. This didn't cause any issues currently, because all uses of #contains are searching for `true`, but this may not be the case in the future. --- In Curves Sculpt mode, `Select`>`Random` is supposed to select all curve points in advance if run on an empty selection. During my testing for !116835, I noticed that, occasionally, performing `Select`>`Random` on an empty selection would not run the code to select all curve points in advance, the cause being that uninitialized parts of the `values` array were being checked too, which could sometimes cause `Select`>`Random` to think some curve points were selected even when there weren't any. The other fixes have come about from me trying to understand and debug the issue. I couldn't seem to get the false positives from accessing uninitialized parts of the `values` array to occur in a debug build, making me wonder if maybe debug builds force array initialization, though I couldn't try `Select`>`Random` very many times due to it being much slower on a debug build.

Thomas Barlow added 4 commits 2024-01-06 05:06:59 +01:00

171adbbbf7 Fix: off-by-1 in curves_selection.cc#contains

Consider an IndexRange of 8 elements starting at 0:
`range.start()` will be `0` and `range.last()` will be `7`.

The size of the range is not `range.last() - range.start()` because that
will be 7, but rather `range.one_after_last() - range.start()`.

The code was using `last()` when it should have been using
`one_after_last()`, which would cause the very last element of each
2048-element section (as per the grain size) of `varray` to be skipped.

bc9ad440ea Fix: Searching uninitialized `values` array in curves_selection.cc#contains

The `values` array has a fixed size of `MaxChunkSize`. When `size` is
smaller than `MaxChunkSize`, only the first `size` elements of `values`
will have been initialized by `varray.materialize_compressed`, but
`std::find` was searching through the entirety of `values` every time.

This could cause false positive results due to the uninitialized values
having a chance to match the value being searched for.

fdd50f2b72 Fix: The search value is ignored in curves_selection.cc#contains

A couple of parts of the function always searched for `true` instead of
the `value` argument.

This may have gone unnoticed because the `contains` function is only
ever used to search for `true`.

2108a6b619 Make format

Thomas Barlow changed title from ~~WIP: Fix: False positives/negatives in curves_selection.cc#contains~~ to Fix: False positives/negatives in curves_selection.cc#contains

2024-01-09 15:02:02 +01:00

Thomas Barlow commented

2024-01-09 15:07:58 +01:00

Hi @filedescriptor, this seems to be your area, can you review please? If preferred, I can create bug reports for the first two issues and split up this PR so that there's one PR for each issue.

Falk David requested review from Hans Goudey 2024-01-09 15:10:35 +01:00

Hans Goudey reviewed 2024-01-09 17:09:48 +01:00

Hans Goudey left a comment

Thanks for the fixes!

source/blender/editors/curves/intern/curves_selection.cc

						
				@ -191,13 +191,15 @@ static bool contains(const VArray<bool> &varray,

				          return init;

				        }

				        constexpr int64_t MaxChunkSize = 512;

Hans Goudey commented

2024-01-09 17:08:30 +01:00

How about using a variable to simplify this a bit:

const int64_t slice_end = range.one_after_last();

How about using a variable to simplify this a bit: ```cpp const int64_t slice_end = range.one_after_last(); ```

Thomas Barlow added 1 commit 2024-01-09 17:32:01 +01:00

1a8e82dbf3 Use a variable for range.one_after_last() to simplify code

Make format.

Hans Goudey approved these changes 2024-01-09 17:39:51 +01:00

Hans Goudey merged commit 96f93ff42e into main

2024-01-09 17:40:23 +01:00

Hans Goudey referenced this issue from a commit

2024-01-09 17:40:24 +01:00

Fix: False positives/negatives in curves_selection.cc#contains

Thomas Barlow deleted branch fix_curves_selection_contains

2024-01-09 17:49:11 +01:00

Kazashi Yoshioka referenced this issue from a commit

2024-01-12 19:10:53 +01:00

Fix: False positives/negatives in curves_selection.cc#contains

Arye Ramaty referenced this issue from a commit

2024-01-21 15:37:39 +01:00

Fix: False positives/negatives in curves_selection.cc#contains

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix: False positives/negatives in curves_selection.cc#contains #116834