`bmesh.ops.find_doubles` with `keep_verts` doesn't work correctly #120750

New Issue

Open

opened 2024-04-17 19:49:26 +02:00 by Max Schlecht · 6 comments

Max Schlecht commented

2024-04-17 19:49:26 +02:00

Contributor

Blender Version
Broken: 4.1

Short description of error

I found this while investigating another issue I had with keep_verts ~~(haven't been able to reproduce that one yet)~~ (I confirmed that it also gets caused by this bug).
When using bmesh.ops.find_doubles(...) with the keep_verts argument, together with bmesh.ops.weld_verts(...), one extra vertex (not present in keep_verts) gets kept.

Exact steps for others to reproduce the error

Run this script in object mode:

from random import randint
import bpy, bmesh

bpy.ops.mesh.primitive_monkey_add()
#bpy.ops.mesh.primitive_torus_add()
obj = bpy.context.object

bm = bmesh.new()
bm.from_mesh(obj.data)
bm.verts.ensure_lookup_table()
doubles = bmesh.ops.find_doubles(bm, verts=bm.verts, keep_verts=[bm.verts[randint(0, 500)], bm.verts[randint(0, 500)]], dist=1000)
print(doubles)
bmesh.ops.weld_verts(bm, targetmap=doubles["targetmap"])
bm.to_mesh(obj.data)
bm.free()

You can try with different primitives by commenting the ops calls in/out.

For the Suzanne it always keeps vert 250 and for the Torus 358 (to check this, simply create another primitive of the same type, go into edit mode with Indices enabled and also enable wireframe rendering).

**Blender Version** Broken: 4.1 **Short description of error** I found this while investigating another issue I had with `keep_verts` ~~(haven't been able to reproduce that one yet)~~ (I confirmed that it also gets caused by this bug). When using `bmesh.ops.find_doubles(...)` with the `keep_verts` argument, together with `bmesh.ops.weld_verts(...)`, one extra vertex (not present in `keep_verts`) gets kept. **Exact steps for others to reproduce the error** Run this script in object mode: ```py from random import randint import bpy, bmesh bpy.ops.mesh.primitive_monkey_add() #bpy.ops.mesh.primitive_torus_add() obj = bpy.context.object bm = bmesh.new() bm.from_mesh(obj.data) bm.verts.ensure_lookup_table() doubles = bmesh.ops.find_doubles(bm, verts=bm.verts, keep_verts=[bm.verts[randint(0, 500)], bm.verts[randint(0, 500)]], dist=1000) print(doubles) bmesh.ops.weld_verts(bm, targetmap=doubles["targetmap"]) bm.to_mesh(obj.data) bm.free() ``` You can try with different primitives by commenting the ops calls in/out. For the Suzanne it always keeps vert 250 and for the Torus 358 (to check this, simply create another primitive of the same type, go into edit mode with Indices enabled and also enable wireframe rendering).

Max Schlecht added the

labels 2024-04-17 19:49:27 +02:00

Iliya Katushenock added the

labels 2024-04-17 20:02:08 +02:00

Max Schlecht changed title from ~~`bmesh.ops.find_doubles` with `keep_verts` keeps extra verts~~ to `bmesh.ops.find_doubles` with `keep_verts` doesn't work correctly

2024-04-17 20:25:29 +02:00

Max Schlecht commented

2024-04-17 20:37:55 +02:00

Author

Contributor

Okay I found out what the bug is here.

The current behavior is:

Do find_doubles and just keep all the vertices in keep_verts. (Yes, this generally sounds correct, but it isn't).

While the advertised behavior from the docs is:

If keep_verts is used, vertices outside that set can only be merged with vertices in that set.

So any merging should only target vertices from keep_verts. While you could of course simply change the docs now, to reflect the current behavior, I'd strongly advise against that, because this behavior is actually useful (while the current one is not).

Okay I found out what the bug is here. ### The current behavior is: Do `find_doubles` and just keep all the vertices in `keep_verts`. (Yes, this generally sounds correct, but it isn't). ### While the advertised behavior from the docs is: > If keep_verts is used, vertices outside that set **can only be merged with vertices in that set**. So any merging should only target vertices from keep_verts. While you could of course simply change the docs now, to reflect the current behavior, I'd strongly advise against that, because this behavior is actually useful (while the current one is not).

Max Schlecht commented

2024-04-17 20:40:24 +02:00

Author

Contributor

This matches up with my test script aswell. The extra vertex that gets kept simply is the vertex all other vertices get merged into. Nothing actually gets merged into the defined keep_verts, they just don't get merged, thus they stay.

This matches up with my test script aswell. The extra vertex that gets kept simply is the vertex all other vertices get merged into. Nothing actually gets merged into the defined `keep_verts`, they just don't get merged, thus they stay.

Germano Cavalcante commented

2024-04-17 23:10:08 +02:00

Member

I edited the script to be easier to investigate.

In fact, the vertices set in "keep_verts" do not become targets, they can become targets, but they do not appear as source in the resulting "targetmap".

Therefore the documentation is wrong.

print('----------------------------')

from random import randint
import bpy, bmesh

bpy.ops.mesh.primitive_cube_add()
obj = bpy.context.object
bm = bmesh.new()
bm.from_mesh(obj.data)
bm.verts.ensure_lookup_table()

vert_to_keep = bm.verts[1]
doubles = bmesh.ops.find_doubles(bm, verts=bm.verts, keep_verts=[vert_to_keep], dist=1000)

print(vert_to_keep)
print('targetmap:')
for src, target in doubles['targetmap'].items():
    print(src, target)

bm.free()

Even though it doesn't seem like a useful parameter, changing the behavior could break compatibility with addons. So it might not be the best approach.

Cc. @ideasman42
Ref. https://docs.blender.org/api/current/bmesh.ops.html#bmesh.ops.find_doubles

I edited the script to be easier to investigate. In fact, the vertices set in "keep_verts" do not become targets, they can become targets, but they do not appear as source in the resulting "targetmap". Therefore the documentation is wrong. ```python print('----------------------------') from random import randint import bpy, bmesh bpy.ops.mesh.primitive_cube_add() obj = bpy.context.object bm = bmesh.new() bm.from_mesh(obj.data) bm.verts.ensure_lookup_table() vert_to_keep = bm.verts[1] doubles = bmesh.ops.find_doubles(bm, verts=bm.verts, keep_verts=[vert_to_keep], dist=1000) print(vert_to_keep) print('targetmap:') for src, target in doubles['targetmap'].items(): print(src, target) bm.free() ``` Even though it doesn't seem like a useful parameter, changing the behavior could break compatibility with addons. So it might not be the best approach. Cc. @ideasman42 Ref. https://docs.blender.org/api/current/bmesh.ops.html#bmesh.ops.find_doubles

Germano Cavalcante added

and removed

labels 2024-04-17 23:10:29 +02:00

Max Schlecht commented

2024-04-18 00:35:48 +02:00

Author

Contributor

Even though it doesn't seem like a useful parameter, changing the behavior could break compatibility with addons. So it might not be the best approach.

Well guess why I'm here ... because my addon is broken ^^

This was how I was handling things before, I'm basically manually doing what find_doubles with keep_verts is supposed to do there.

This is much slower obviously and simply unnecessary.

As said previously, I'd strongly advise against simply changing the docs. There's probably like a dozen addons which actually use this functionality and I somewhat doubt any of them actually expect/know the how it currently behaves. I really can't think of a single valid usecase for the current behavior.

My addon pretty much breaks every second release and I simply take a look at the patch notes and change things accordingly. Fixing this and adding a simple note that this function now finally works as it's supposed to be would be sufficient.

> Even though it doesn't seem like a useful parameter, changing the behavior could break compatibility with addons. So it might not be the best approach. Well guess why I'm here ... because my addon is broken ^^ [This](https://github.com/30350n/pcb2blender/blob/24cd5ae8fbadd2b26ed34eddff0e56faef63ab38/pcb2blender_importer/importer.py#L459-L470) was how I was handling things before, I'm basically manually doing what `find_doubles` with `keep_verts` is *supposed* to do there. This is much slower obviously and simply unnecessary. As said previously, **I'd strongly advise against simply changing the docs**. There's probably like a dozen addons which actually use this functionality and I somewhat doubt any of them actually expect/know the how it currently behaves. I really can't think of a single valid usecase for the current behavior. My addon pretty much breaks every second release and I simply take a look at the patch notes and change things accordingly. Fixing this and adding a simple note that this function now finally works as it's supposed to be would be sufficient.

Max Schlecht commented

2024-04-18 03:13:27 +02:00

Author

Contributor

Tracked down the commit where this got broken too: 4d148471b6

Seems like it was meant to fix a performance issue, but completely broke the implementation while doing so sadly. From what I can tell though, the correct behavior is probably not that easy to implement with the KDTrees? Not sure.
It would add quite a lot of code, but maybe the function should just fallback to the old implementation (or something similar) if keep_verts are provided.

Tracked down the commit where this got broken too: https://projects.blender.org/blender/blender/commit/4d148471b66a7f7c2533e3d1d2b037fd838b9bc4 Seems like it was meant to fix a performance issue, but completely broke the implementation while doing so sadly. From what I can tell though, the correct behavior is probably not that easy to implement with the KDTrees? Not sure. It would add quite a lot of code, but maybe the function should just fallback to the old implementation (or something similar) if `keep_verts` are provided.

Philipp Oeser commented

2024-04-18 11:10:29 +02:00

Member

Tracked down the commit where this got broken too: 4d148471b6

CC @ideasman42 ^^

> Tracked down the commit where this got broken too: https://projects.blender.org/blender/blender/commit/4d148471b66a7f7c2533e3d1d2b037fd838b9bc4 CC @ideasman42 ^^

Sign in to join this conversation.

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#120750

No description provided.