Selection: Remove limit on number of items which can be selected at once #112491

Jesse Yurkovich · 2023-09-18T03:07:41+02:00

Jesse Yurkovich commented

2023-09-18 03:07:41 +02:00

This removes the long-standing static limit for the selection buffer in
favor of a dynamic approach.

It replaces the static array with our Vector type where the inline
buffer provides parity with existing code while also providing the
ability to grow as necessary.

Fixes #102487, #109178, #112350, #112493, and blender/blender-manual#102485

Notes

In many instances a single GPUSelectionBuffer is reused across multiple calls to the selection system and multiple hit-count calculations. This means that the APIs returning "hit" counts must continue to do so as we can't depend only on the .size of the buffer. It also means that we have to be careful to slice out the relevant hit segments in places like mixed_bones_object_selectbuffer where work needs to occur only on the new hits.

We can shrink the 2500 inline buffer down if we like now, though we've been fine with the amount of stack space consumed so far.

Unfortunately, even after this change, the 32-bit selection_id itself imposes an ultimate limit. It is constructed as follows:
[3-bit bone mask | 13-bit bone id | 16-bit object id]
Which means:

Only 8192 bones are selectable (#106542)
Only 65535 objects are selectable (#104008)

That said, this change will allow many more scenarios to work reliably.

This removes the long-standing static limit for the selection buffer in favor of a dynamic approach. It replaces the static array with our Vector type where the inline buffer provides parity with existing code while also providing the ability to grow as necessary. Fixes #102487, #109178, #112350, #112493, and https://projects.blender.org/blender/blender-manual/issues/102485 ---- Notes In many instances a single GPUSelectionBuffer is reused across multiple calls to the selection system and multiple hit-count calculations. This means that the APIs returning "hit" counts must continue to do so as we can't depend only on the .size of the buffer. It also means that we have to be careful to `slice` out the relevant hit segments in places like `mixed_bones_object_selectbuffer` where work needs to occur only on the new hits. We can shrink the 2500 inline buffer down if we like now, though we've been fine with the amount of stack space consumed so far. Unfortunately, even after this change, the 32-bit selection_id itself imposes an ultimate limit. It is constructed as follows: `[3-bit bone mask | 13-bit bone id | 16-bit object id]` Which means: - Only 8192 bones are selectable (#106542) - Only 65535 objects are selectable (#104008) That said, this change will allow many more scenarios to work reliably.

👍 9

Jesse Yurkovich added 4 commits 2023-09-18 03:07:57 +02:00

0c4204ca70 Use a dynamic selection buffer

efcb8d282a Modernize armature code a bit more

5f3b2176bf Better parameter names

b48f0e69ec Hide the buffer storage template inside a struct

Jesse Yurkovich added the

Module

Modeling

label 2023-09-18 04:07:02 +02:00

Jesse Yurkovich added 1 commit 2023-09-18 07:55:58 +02:00

2e5aeae399 Fully remove console message now

Jesse Yurkovich requested review from Campbell Barton 2023-09-19 20:52:55 +02:00

Jesse Yurkovich requested review from Clément Foucault 2023-09-19 20:52:55 +02:00

Clément Foucault requested changes 2023-09-28 15:23:36 +02:00

Clément Foucault left a comment

Spotted a few errors and compilation error in debug build. Here is the changes I did to make it working.

I think it's a nice addition. I think inline buffer size was motivated by the speed of the sorting / processing of the result hits. I don't know how much relevant this is nowadays. Maybe Campbell has more insight on this.

I'm just unsure about removing the extern "C" guard in the header. Maybe it is better to rename it to .hh so it's clear it is a C++ only header. But then the patch would get a bit noisy. So better do that afterwards.

Spotted a few errors and compilation error in debug build. Here is the changes I did to make it working. I think it's a nice addition. I think inline buffer size was motivated by the speed of the sorting / processing of the result hits. I don't know how much relevant this is nowadays. Maybe Campbell has more insight on this. I'm just unsure about removing the `extern "C"` guard in the header. Maybe it is better to rename it to `.hh` so it's clear it is a C++ only header. But then the patch would get a bit noisy. So better do that afterwards.

select_diff

2.7 KiB

source/blender/editors/armature/armature_select.cc Outdated

						
				@ -637,3 +640,3 @@

				{

				  const int ofs = hits12;

				  memcpy(buffer, buffer + ofs, hits5 * sizeof(*buffer));

				  results.copy_from(results.slice(ofs, hits5)); /* Shift results to beginning. */

Clément Foucault commented

2023-09-28 15:15:11 +02:00

These are incorrect. It should be
results.slice(0, hits5).copy_from(results.slice(ofs, hits5));
Same goes for the other 2 copy_from in this patch.

These are incorrect. It should be `results.slice(0, hits5).copy_from(results.slice(ofs, hits5));` Same goes for the other 2 `copy_from` in this patch.

deadpin marked this conversation as resolved

source/blender/editors/armature/armature_select.cc Outdated

						
				@ -706,25 +705,20 @@ static EditBone *get_nearest_editbonepoint(

				      ofs = hits12;

Clément Foucault commented

2023-09-28 15:00:11 +02:00

Warning: Unused variable

deadpin marked this conversation as resolved

source/blender/windowmanager/gizmo/intern/wm_gizmo_map.cc Outdated

						
				@ -614,3 +614,3 @@

				    for (int i = 0; i < hits; i++, buf_iter++) {

				    for (const GPUSelectResult &result : hit_results) {

				      BLI_assert(buf_iter->id != -1);

Clément Foucault commented

2023-09-28 15:01:06 +02:00

Should be BLI_assert(result.id != -1);

Should be `BLI_assert(result.id != -1);`

deadpin marked this conversation as resolved

Jesse Yurkovich added 2 commits 2023-09-28 21:10:47 +02:00

e5afafc1e2 Merge remote-tracking branch 'upstream/main' into fixselection

cefb7334c8 Feedback and fixes

Jesse Yurkovich commented

2023-09-28 21:13:03 +02:00

Thanks for that review! I did miss those .copy_froms...

Yes, I'll plan on changing the header afterwards to .hh

Thanks for that review! I did miss those .copy_froms... Yes, I'll plan on changing the header afterwards to `.hh`

Clément Foucault approved these changes 2023-09-28 23:57:00 +02:00

sanek122005 commented

2023-10-05 22:17:36 +02:00

First-time contributor

will this be available as a fix for 4.0?

Pratik Borhade referenced this pull request

2023-11-06 04:21:18 +01:00

Box Select fails with too many objects/instances #114514

Jesse Yurkovich added 1 commit 2023-11-15 23:35:58 +01:00

05ac81e165 Merge branch 'main' of projects.blender.org:blender/blender into fixselection

Campbell Barton reviewed 2023-11-30 10:52:55 +01:00

Campbell Barton left a comment

Generally very nice, only some minor details.

The Span<GPUSelectResult> declarations which aren´t modified should be made const.
There is an outdated reference to MAXPICKELEMS in comments which should be removed.

Generally very nice, only some minor details. - The `Span<GPUSelectResult>` declarations which aren´t modified should be made `const`. - There is an outdated reference to `MAXPICKELEMS` in comments which should be removed.

Campbell Barton approved these changes 2023-11-30 11:10:37 +01:00

Campbell Barton left a comment

Minor requests, accepting.

source/blender/gpu/intern/gpu_select.cc Outdated

						
				@ -273,3 +262,1 @@

				    if (buffer_src->id != select_id) {

				      if (buffer_dst != buffer_src) {

				        memcpy(buffer_dst, buffer_src, sizeof(GPUSelectResult));

				  int64_t index_src = 0;

Campbell Barton commented

2023-11-30 11:06:16 +01:00

These can be int, the range is easily enough and used elsewhere with selection indices.

These can be `int`, the range is easily enough and used elsewhere with selection indices.

deadpin marked this conversation as resolved

source/blender/gpu/intern/gpu_select_pick.cc Outdated

						
				@ -678,3 +666,1 @@

				      hits++;

				    }

				    BLI_assert(hits < maxhits);

				    g_pick_state.buffer->storage.append({depth_data[i].id, depth_data[i].depth});

Campbell Barton commented

2023-11-30 11:09:02 +01:00

Since the size is known ahead of time, reserve then use append_unchecked to avoid re-allocation.

Since the size is known ahead of time, reserve then use `append_unchecked` to avoid re-allocation.

deadpin marked this conversation as resolved

Clément Foucault commented

2023-11-30 14:30:11 +01:00

The Span declarations which aren´t modified should be made const.

The Span type itself is const. From the Span header doc:

An blender::Span<T> references an array that is owned by someone else. It is just a
pointer and a size. Since the memory is not owned, Span should not be used to transfer
ownership. The array cannot be modified through the Span. However, if T is a non-const
pointer, the pointed-to elements can be modified.

So I guess this is not important. MutableSpan is what should be used when the data is modified.

> The Span<GPUSelectResult> declarations which aren´t modified should be made const. The `Span` type itself is const. From the Span header doc: > An `blender::Span<T>` references an array that is owned by someone else. It is just a pointer and a size. Since the memory is not owned, Span should not be used to transfer ownership. The array cannot be modified through the Span. However, if T is a non-const pointer, the pointed-to elements can be modified. So I guess this is not important. `MutableSpan` is what should be used when the data is modified.

Jesse Yurkovich commented

2023-11-30 19:14:26 +01:00

I see now that const Span is used quite often so I can make the change here as well. Making it const basically prevents cases where someone could re-assign the Span itself.

I see now that `const Span` is used quite often so I can make the change here as well. Making it const basically prevents cases where someone could re-assign the Span itself.

👍 1

Jesse Yurkovich added 2 commits 2023-11-30 20:02:46 +01:00

2771f0736a Merge remote-tracking branch 'upstream/main' into fixselection

buildbot/vexp-code-patch-coordinator Build done. Details

0c9ff6df42 Feedback

Jesse Yurkovich commented

2023-11-30 20:05:25 +01:00

@blender-bot build

Clément Foucault approved these changes 2023-11-30 20:11:04 +01:00

Jesse Yurkovich merged commit abf59eb23a into main

2023-11-30 21:27:23 +01:00

Jesse Yurkovich referenced this issue from a commit