Fix: calculating duplicates in kdtree does not work with selection #107535

Jacques Lucke · 2023-05-02T12:41:24+02:00

Jacques Lucke commented

2023-05-02 12:41:24 +02:00

This fixes a crash where merging vertices by distance leads to a crash since 0652945dbd. The change did not cause the bug though, it just made the underlying issue visible.

The crash was found by Simon in the /pro/lib/nodes/shading.blend production file (just opening the file leads to the crash).

This bug was already hinted at by @MMMM in #107312 (comment).

The issue was that kdtree_order assumed that the KDTreeNode.index didn't have gaps, i.e. every index in a certain range is used. However, that is not the case when only a subset of the vertices of a mesh are added to the kdtree.

This fixes a crash where merging vertices by distance leads to a crash since 0652945dbda76e5ba329a7e5aee45afc0d3a83d3. The change did not cause the bug though, it just made the underlying issue visible. The crash was found by Simon in the `/pro/lib/nodes/shading.blend` production file (just opening the file leads to the crash). This bug was already hinted at by @MMMM in https://projects.blender.org/blender/blender/pulls/107312#issuecomment-928995. The issue was that `kdtree_order` assumed that the `KDTreeNode.index` didn't have gaps, i.e. every index in a certain range is used. However, that is not the case when only a subset of the vertices of a mesh are added to the kdtree.

👍 1

Jacques Lucke added 1 commit 2023-05-02 12:41:35 +02:00

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

b931dd7aca fix

Jacques Lucke commented

2023-05-02 12:41:51 +02:00

@blender-bot build

Jacques Lucke requested review from Iliya Katushenock 2023-05-02 12:47:22 +02:00

Jacques Lucke requested review from Hans Goudey 2023-05-02 12:47:22 +02:00

Iliya Katushenock reviewed 2023-05-02 13:29:11 +02:00

source/blender/blenlib/intern/kdtree_impl.h Outdated

						
				@ -887,3 +892,3 @@

				  if (use_index_order) {

				    uint *order = kdtree_order(tree);

				    int *order = kdtree_order(tree);

				    for (uint i = 0; i < tree->nodes_len; i++) {

Iliya Katushenock commented

2023-05-02 13:29:11 +02:00

for (int i = 0; i < tree->max_node_index; i++) { ?

`for (int i = 0; i < tree->max_node_index; i++) {` ?

Iliya Katushenock commented

2023-05-02 13:40:33 +02:00

Looking at this implementation, I'm a little disappointed (when I read the comment about the huge cost of this feature, I thought it was something expensive, something like a lot of recursivity...).
But reading this now, it looks like the C++ version would use the analog map for this just by default.
But yes, I think, for now, allocation is the only way out (or sorting node indexes with comparison of internal node indexes).

Looking at this implementation, I'm a little disappointed (when I read the comment about the huge cost of this feature, I thought it was something expensive, something like a lot of recursivity...). But reading this now, it looks like the C++ version would use the analog map for this just by default. But yes, I think, for now, allocation is the only way out (or sorting node indexes with comparison of internal node indexes).

Iliya Katushenock approved these changes 2023-05-02 13:40:52 +02:00

Jacques Lucke added 1 commit 2023-05-02 14:16:24 +02:00

f3a484cc2f fix

Hans Goudey approved these changes 2023-05-02 14:20:21 +02:00

Jacques Lucke merged commit 20b19f2b12 into main

2023-05-02 14:26:02 +02:00

Jacques Lucke referenced this issue from a commit

2023-05-02 14:26:02 +02:00

Fix: calculating duplicates in kdtree does not work with selection

Jacques Lucke deleted branch kdtree-calc-duplicates-selection

2023-05-02 14:26:03 +02:00

Howard Trickey referenced this issue from a commit

2023-05-29 02:51:43 +02:00

Merge in orign/main:

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: calculating duplicates in kdtree does not work with selection #107535