Data Transfer modifier's Max Distance field working strangely #54526

New Issue

Nathan Vasil · 2018-04-04T17:29:12+02:00

Nathan Vasil commented

2018-04-04 17:29:12 +02:00

Using Blender 2.89b. EVGA GTX1070. Win10. All non-default add-ons disabled and new scene loaded.

Attached demonstration file contains a cube using a data transfer with max distance enabled to copy weights from a nearby plane, and an armature for that cube. Max Distance settings should put four verts in the domain of the modifier; as you can see, seven verts are affected.

Problem encountered in less abstract situations as well, this is just my simplest reduction of the problem in order to verify that it's an issue and make it plain to see and reproduce.

For reference, official documentation reads, "This allows to transfer a small sub-detailed mesh onto a more complete one (e.g. from a “hand” mesh towards a “full body” one)," which would be useful if it were working properly.
DataDistance1.blend

Using Blender 2.89b. EVGA GTX1070. Win10. All non-default add-ons disabled and new scene loaded. Attached demonstration file contains a cube using a data transfer with max distance enabled to copy weights from a nearby plane, and an armature for that cube. Max Distance settings should put four verts in the domain of the modifier; as you can see, seven verts are affected. Problem encountered in less abstract situations as well, this is just my simplest reduction of the problem in order to verify that it's an issue and make it plain to see and reproduce. For reference, official documentation reads, "This allows to transfer a small sub-detailed mesh onto a more complete one (e.g. from a “hand” mesh towards a “full body” one)," which would be useful if it were working properly. [DataDistance1.blend](https://archive.blender.org/developer/F2597954/DataDistance1.blend)

Nathan Vasil commented

2018-04-04 17:29:12 +02:00

Added subscriber: @vasiln

Philipp Oeser commented

2018-04-05 10:41:13 +02:00

Added subscriber: @lichtwerk

Philipp Oeser commented

2018-04-05 10:41:13 +02:00

Confirming on first sight, having a closer look now...

Philipp Oeser self-assigned this 2018-04-09 10:16:06 +02:00

Philipp Oeser commented

2018-04-09 10:16:06 +02:00

Added subscriber: @mont29

Philipp Oeser commented

2018-04-09 10:16:06 +02:00

I'll claim for the time being and do some further investiagtion. If that doesnt succeed, I'll get @mont29 on board to help me out...

Nathan Vasil commented

2019-01-30 18:23:49 +01:00

Coming back to add that there's a reasonable workaround, which is to use a vertex weight proximity modifier to determine the distance from each vertex before the weight transfer, use a custom (constant) curve to modulate that vertex group, and then use it to modulate the data transfer. It's a bit more work for the end user, but perfectly reasonable, and it wouldn't seem unreasonable to just drop support for max distance from the data transfer.

Philipp Oeser commented

2019-05-07 19:53:47 +02:00

Added subscriber: @angavrilov

Philipp Oeser commented

2019-05-07 19:53:47 +02:00

Had a look at this again and here are some findings:

BKE_mesh_remap_calc_verts_from_mesh / mesh_remap_bvhtree_query_nearest is where things happen
in this loop , code keeps the previous "nearest" [but doesnt reset nearest.index to -1]
if BLI_bvhtree_find_nearest() is called with a "nearest" provided, code will not update anything on "nearest" (esp. not index, not dist_sq) if it fails to find something.
so the comment here is actually misleading I think, index is not neccessarily -1 if it fails to find something.
if we have previously found something [making nearest.index non-zero], and then for the next vertex dont find anything, we still have a non-zero index leading to false positives in the following code.

It actually works fine if the index is reset to -1 for each vert.
Could be like this (would have to be done for a couple of other loops as well):
P967: T54526_snippet_1



diff --git a/source/blender/blenkernel/intern/mesh_remap.c b/source/blender/blenkernel/intern/mesh_remap.c
index db158ca8fb2..c1dd62d2719 100644
--- a/source/blender/blenkernel/intern/mesh_remap.c
+++ b/source/blender/blenkernel/intern/mesh_remap.c
@@ -526,9 +526,12 @@ void BKE_mesh_remap_calc_verts_from_mesh(const int mode,
 
     if (mode == MREMAP_MODE_VERT_NEAREST) {
       BKE_bvhtree_from_mesh_get(&treedata, me_src, BVHTREE_FROM_VERTS, 2);
-      nearest.index = -1;
+      //nearest.index = -1;
 
       for (i = 0; i < numverts_dst; i++) {
+        /* resetting index here would work */
+        nearest.index = -1;
+
         copy_v3_v3(tmp_co, verts_dst- [x].co);
 
         /* Convert the vertex to tree coordinates, if needed. */

Or more general like:
P968: T54526_snippet_2



diff --git a/source/blender/blenkernel/intern/mesh_remap.c b/source/blender/blenkernel/intern/mesh_remap.c
index db158ca8fb2..fb4ce68fed3 100644
--- a/source/blender/blenkernel/intern/mesh_remap.c
+++ b/source/blender/blenkernel/intern/mesh_remap.c
@@ -67,6 +67,7 @@ static bool mesh_remap_bvhtree_query_nearest(BVHTreeFromMesh *treedata,
     nearest->dist_sq = max_dist_sq;
   }
   /* Compute and store result. If invalid (-1 index), keep FLT_MAX dist. */
+  nearest->index = -1;
   BLI_bvhtree_find_nearest(treedata->tree, co, nearest, treedata->nearest_callback, treedata);
 
   if ((nearest->index != -1) && (nearest->dist_sq <= max_dist_sq)) {

@angavrilov, @mont29: what do you think?

Had a look at this again and here are some findings: - `BKE_mesh_remap_calc_verts_from_mesh` / `mesh_remap_bvhtree_query_nearest` is where things happen - in [this loop ](https://developer.blender.org/diffusion/B/browse/master/source/blender/blenkernel/intern/mesh_remap.c$531), code keeps the previous "nearest" [but doesnt reset `nearest.index` to -1] - if `BLI_bvhtree_find_nearest()` is called with a "nearest" provided, code will not update anything on "nearest" (esp. not `index`, not `dist_sq`) if it fails to find something. - so [the comment here ](https://developer.blender.org/diffusion/B/browse/master/source/blender/blenkernel/intern/mesh_remap.c$69) is actually misleading I think, `index` is not neccessarily -1 if it fails to find something. - if we have previously found something [making `nearest.index` non-zero], and then for the next vertex dont find anything, we still have a non-zero `index` leading to false positives in the following code. It actually works fine if the index is reset to -1 for each vert. Could be like this (would have to be done for a couple of other loops as well): [P967: T54526_snippet_1](https://archive.blender.org/developer/P967.txt) ``` diff --git a/source/blender/blenkernel/intern/mesh_remap.c b/source/blender/blenkernel/intern/mesh_remap.c index db158ca8fb2..c1dd62d2719 100644 --- a/source/blender/blenkernel/intern/mesh_remap.c +++ b/source/blender/blenkernel/intern/mesh_remap.c @@ -526,9 +526,12 @@ void BKE_mesh_remap_calc_verts_from_mesh(const int mode, if (mode == MREMAP_MODE_VERT_NEAREST) { BKE_bvhtree_from_mesh_get(&treedata, me_src, BVHTREE_FROM_VERTS, 2); - nearest.index = -1; + //nearest.index = -1; for (i = 0; i < numverts_dst; i++) { + /* resetting index here would work */ + nearest.index = -1; + copy_v3_v3(tmp_co, verts_dst- [x].co); /* Convert the vertex to tree coordinates, if needed. */ ``` Or more general like: [P968: T54526_snippet_2](https://archive.blender.org/developer/P968.txt) ``` diff --git a/source/blender/blenkernel/intern/mesh_remap.c b/source/blender/blenkernel/intern/mesh_remap.c index db158ca8fb2..fb4ce68fed3 100644 --- a/source/blender/blenkernel/intern/mesh_remap.c +++ b/source/blender/blenkernel/intern/mesh_remap.c @@ -67,6 +67,7 @@ static bool mesh_remap_bvhtree_query_nearest(BVHTreeFromMesh *treedata, nearest->dist_sq = max_dist_sq; } /* Compute and store result. If invalid (-1 index), keep FLT_MAX dist. */ + nearest->index = -1; BLI_bvhtree_find_nearest(treedata->tree, co, nearest, treedata->nearest_callback, treedata); if ((nearest->index != -1) && (nearest->dist_sq <= max_dist_sq)) { ``` @angavrilov, @mont29: what do you think?

Bastien Montagne commented

2019-05-08 15:58:37 +02:00

@lichtwerk nice catch. Wouldn’t it be even simpler to always reset that nearest->index directly in BLI_bvhtree_find_nearest_ex()? Keeping previous nearest distance there if provided makes sense, but I think item index should be always reset, at least I cannot see a case where keeping previous value would be useful?

@lichtwerk nice catch. Wouldn’t it be even simpler to always reset that `nearest->index` directly in `BLI_bvhtree_find_nearest_ex()`? Keeping previous nearest distance there if provided makes sense, but I think item index should be always reset, at least I cannot see a case where keeping previous value would be useful?

Philipp Oeser commented

2019-07-09 12:02:28 +02:00

Added subscribers: @mano-wii, @brecht

Philipp Oeser commented

2019-07-09 12:02:28 +02:00

I went over usages of BLI_bvhtree_find_nearest / BLI_bvhtree_find_nearest_projected and here are some findings:

these are the ones that always set item index to -1 before calling, it looks like an index of -1 ALWAYS should be OK:

dynamic_paint_paint_mesh_cell_point_cb_ex (called in a loop, item index set to -1 prior every time)
BKE_bmbvh_find_vert_closest (called once, item index set to -1 prior)
BKE_bmbvh_find_face_closest (called once, item index set to -1 prior)
closest_point_on_surface (called once, item index set to -1 prior)
obstacles_from_mesh_task_cb (smoke code, called in a loop, item index set to -1 prior every time)
sample_mesh (smoke code, called once, item index set to -1 prior)
remap_hair_emitter (called in a loop, item index set to -1 prior every time)
rna_Object_closest_point_on_mesh (called once, item index set to -1 prior)
nearestVert (surface deform modifier, called once, item index set to -1 prior)
vert2geom_task_cb_ex (weight proximity modifier, called once, item index set to -1 prior every time)
py_bvhtree_find_nearest (called once, item index set to -1 prior)
find_nearest_points_test (called with NULL)

snapping: these are the ones that set item index to -1 before calling but then could enter multiple times (vert, edge, ...), not sure if an index of -1 ALWAYS would be OK? maybe @mano-wii knows more?:

snapMesh (item index set to -1 prior, but multiple usages here which could all be entered?... not sure)
snapEditMesh (item index set to -1 prior, but multiple usages here which could all be entered?... not sure)

shrinkwrap: not 100% sure, but it looks like an index of -1 ALWAYS should be OK

shrinkwrap_calc_nearest_vertex_cb_ex (resets index to -1, but in ParallelRangeSettings, see shrinkwrap_calc_nearest_surface_point, not 100% sure...)
BKE_shrinkwrap_find_nearest_surface (does not reset index to -1, needs checking, called from shrinkwrap_get_tarmat [where index is set to -1] / shrinkwrap_calc_nearest_vertex_cb_ex [see above])

our candidate for this report:), looks like an index of -1 ALWAYS should be OK

mesh_remap_bvhtree_query_nearest (does not reset index to -1 atm., results in bug)

Will also subscribe @brecht as lead architect here, but if we want to have this in for RC1, propose to actually play safe and go with D5206?

I went over usages of `BLI_bvhtree_find_nearest` / `BLI_bvhtree_find_nearest_projected` and here are some findings: these are the ones that always set item index to -1 before calling, it looks like an index of -1 **ALWAYS** should be OK: - `dynamic_paint_paint_mesh_cell_point_cb_ex` (called in a loop, item index set to -1 prior every time) - `BKE_bmbvh_find_vert_closest` (called once, item index set to -1 prior) - `BKE_bmbvh_find_face_closest` (called once, item index set to -1 prior) - `closest_point_on_surface` (called once, item index set to -1 prior) - `obstacles_from_mesh_task_cb` (smoke code, called in a loop, item index set to -1 prior every time) - `sample_mesh` (smoke code, called once, item index set to -1 prior) - `remap_hair_emitter` (called in a loop, item index set to -1 prior every time) - `rna_Object_closest_point_on_mesh` (called once, item index set to -1 prior) - `nearestVert` (surface deform modifier, called once, item index set to -1 prior) - `vert2geom_task_cb_ex` (weight proximity modifier, called once, item index set to -1 prior every time) - `py_bvhtree_find_nearest` (called once, item index set to -1 prior) - `find_nearest_points_test` (called with NULL) snapping: these are the ones that set item index to -1 before calling but then could enter multiple times (vert, edge, ...), not sure if an index of -1 **ALWAYS** would be OK? maybe @mano-wii knows more?: - `snapMesh` (item index set to -1 prior, but multiple usages here which could all be entered?... not sure) - `snapEditMesh` (item index set to -1 prior, but multiple usages here which could all be entered?... not sure) shrinkwrap: not 100% sure, but it looks like an index of -1 **ALWAYS** should be OK - `shrinkwrap_calc_nearest_vertex_cb_ex` (resets index to -1, but in ParallelRangeSettings, see `shrinkwrap_calc_nearest_surface_point`, not 100% sure...) - `BKE_shrinkwrap_find_nearest_surface` (does not reset index to -1, needs checking, called from `shrinkwrap_get_tarmat` [where index is set to -1] / `shrinkwrap_calc_nearest_vertex_cb_ex` [see above]) our candidate for this report:), looks like an index of -1 **ALWAYS** should be OK - `mesh_remap_bvhtree_query_nearest` (does not reset index to -1 atm., results in bug) Will also subscribe @brecht as lead architect here, but if we want to have this in for RC1, propose to actually play safe and go with [D5206](https://archive.blender.org/developer/D5206)?

Alexander Gavrilov commented

2019-07-09 12:24:27 +02:00

Keeping the previous search result when doing multiple nearest searches is a very important heuristic optimization. Not doing that in shrinkwrap for instance will absolutely butcher performance.

This heuristic however obviously relies on the previous search result being a valid candidate for the new search (i.e. it can be reused as is if it's still the nearest point).

More specifically, BVH nearest search code by default doesn't bother to ensure an optimal search order through the tree, because using the previous search result to prime the search and prune the tree works out to be faster. When implementing the new more complicated shrinkwrap mode that can't use this heuristic, I had to add BLI_bvhtree_find_nearest_ex and BVH_NEAREST_OPTIMAL_ORDER to get good performance.

Keeping the previous search result when doing multiple nearest searches is a very important heuristic optimization. Not doing that in shrinkwrap for instance will absolutely butcher performance. This heuristic however obviously relies on the previous search result being a valid candidate for the new search (i.e. it can be reused as is if it's still the nearest point). More specifically, BVH nearest search code by default doesn't bother to ensure an optimal search order through the tree, because using the previous search result to prime the search and prune the tree works out to be faster. When implementing the new more complicated shrinkwrap mode that can't use this heuristic, I had to add `BLI_bvhtree_find_nearest_ex` and `BVH_NEAREST_OPTIMAL_ORDER` to get good performance.

Alexander Gavrilov commented

2019-07-09 12:42:14 +02:00

Perhaps BVH_NEAREST_OPTIMAL_ORDER should be the default behavior instead (and also ignore the previous result), while the heuristic behavior should be invoked through BVH_NEAREST_REUSE_RESULT or something like that. I did it the way it is currently to avoid changing old code all over the place.

Perhaps `BVH_NEAREST_OPTIMAL_ORDER` should be the default behavior instead (and also ignore the previous result), while the heuristic behavior should be invoked through `BVH_NEAREST_REUSE_RESULT` or something like that. I did it the way it is currently to avoid changing old code all over the place.

Philipp Oeser commented

2019-07-09 13:37:40 +02:00

@angavrilov : thx for the heads up, will check on this (might take a bit to go over all cases), for the "quick fix": D5206 should still be fine, right?

@angavrilov : thx for the heads up, will check on this (might take a bit to go over all cases), for the "quick fix": [D5206](https://archive.blender.org/developer/D5206) should still be fine, right?

Germano Cavalcante commented

2019-07-09 15:50:17 +02:00

In #54526#716048, @lichtwerk wrote:
(...)
snapping: these are the ones that set item index to -1 before calling but then could enter multiple times (vert, edge, ...), not sure if an index of -1 ALWAYS would be OK? maybe @mano-wii knows more?:

snapMesh (item index set to -1 prior, but multiple usages here which could all be entered?... not sure)

snapEditMesh (item index set to -1 prior, but multiple usages here which could all be entered?... not sure)

(...)

These are cases where changing the nearest.index to -1 between calls would result in problems.

> In #54526#716048, @lichtwerk wrote: > (...) > snapping: these are the ones that set item index to -1 before calling but then could enter multiple times (vert, edge, ...), not sure if an index of -1 **ALWAYS** would be OK? maybe @mano-wii knows more?: > - `snapMesh` (item index set to -1 prior, but multiple usages here which could all be entered?... not sure) > - `snapEditMesh` (item index set to -1 prior, but multiple usages here which could all be entered?... not sure) > > (...) These are cases where changing the nearest.index to -1 between calls would result in problems.

Philipp Oeser was unassigned by Dalai Felinto

2019-12-23 16:36:15 +01:00

Philipp Oeser commented

2020-02-12 13:22:41 +01:00

Still an issue. Will have to go through the points raised here (and in D5206) again

Still an issue. Will have to go through the points raised here (and in [D5206](https://archive.blender.org/developer/D5206)) again

blender-admin commented

2020-10-16 12:59:03 +02:00

This issue was referenced by 3b84dce969

This issue was referenced by 3b84dce96967e7a9fe568e2518ee774843d48733

Bastien Montagne commented

2020-10-16 13:00:49 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Bastien Montagne closed this issue

2020-10-16 13:00:49 +02:00

Bastien Montagne self-assigned this 2020-10-16 13:00:49 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Data Transfer modifier's Max Distance field working strangely #54526