WIP: Fix #108880: Skin and Miffor modifiers cause mesh collapse #108915

Germano Cavalcante · 2023-06-12T23:37:11+02:00

Germano Cavalcante commented

2023-06-12 23:37:11 +02:00

Before 4369627e71, the Mirror modifier merge did not interpolate customdata.

That merge just copied the customdata from the original vertex. Thus, flags like MVERT_SKIN_ROOT were kept.

#108880's solution would be simple if it weren't for one detail: the Skin mod requires only one "root" per island.

For that reason, this commit does two things:

copies the flags MVERT_SKIN_ROOT and MVERT_SKIN_LOOSE from the skin to the resulting vertex.
Removes the MVERT_SKIN_ROOT flag from different roots on the same island.

Before 4369627e71, the Mirror modifier merge did not interpolate customdata. That merge just copied the customdata from the original vertex. Thus, flags like `MVERT_SKIN_ROOT` were kept. #108880's solution would be simple if it weren't for one detail: the Skin mod requires only one "root" per island. For that reason, this commit does two things: - copies the flags `MVERT_SKIN_ROOT` and `MVERT_SKIN_LOOSE` from the skin to the resulting vertex. - Removes the `MVERT_SKIN_ROOT` flag from different roots on the same island.

Germano Cavalcante added 1 commit 2023-06-12 23:37:20 +02:00

4465ca1381 Fix #108880 : Skin and Miffor modifiers cause mesh collapse

Germano Cavalcante changed title from ~~Fix #108880: Skin and Miffor modifiers cause mesh collapse~~ to WIP: Fix #108880: Skin and Miffor modifiers cause mesh collapse

2023-06-12 23:41:32 +02:00

Hans Goudey commented

2023-06-13 03:17:17 +02:00

TBH I'm not sure it's worth the complexity to support the specific format the skin modifier needs here. It's clearly possible to do here, but I don't think it's really doable to maintain this data in general, especially given the performance cost. At least the behavior should be generalized a bit conceptually, so it won't just apply to this specific data type for the skin modifier (which will probably be replaced soon enough, like the other non-attribute types).

For example, another way to deal with this is to allow setting the root vertices after the merging. If the skin modifier used more generic inputs (attributes), that would already be possible with geometry nodes.

TBH I'm not sure it's worth the complexity to support the specific format the skin modifier needs here. It's clearly possible to do here, but I don't think it's really doable to maintain this data in general, especially given the performance cost. At least the behavior should be generalized a bit conceptually, so it won't just apply to this specific data type for the skin modifier (which will probably be replaced soon enough, like the other non-attribute types). For example, another way to deal with this is to allow setting the root vertices _after_ the merging. If the skin modifier used more generic inputs (attributes), that would already be possible with geometry nodes.

Germano Cavalcante commented

2023-06-13 06:14:11 +02:00

Oh @HooglyBoogly, thanks for the feedback!
I believe I was a bit rash in considering multiple roots on the same geometry island a bug. There are many cases where multiple roots can appear like:

apply mirror modifier;
create an Edge connecting two islands each containing a root;
Merging vertices from different islands...

So this is quite common and not really a buug.

So I submitted a simple fix: f8617fe71b

(This PR can be good for reference in case we need a function that computes the islands in the future).

Oh @HooglyBoogly, thanks for the feedback! I believe I was a bit rash in considering multiple roots on the same geometry island a bug. There are many cases where multiple roots can appear like: - apply mirror modifier; - create an Edge connecting two islands each containing a root; - Merging vertices from different islands... So this is quite common and not really a buug. So I submitted a simple fix: f8617fe71b (This PR can be good for reference in case we need a function that computes the islands in the future).

Germano Cavalcante closed this pull request

2023-06-13 06:14:11 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

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

WIP: Fix #108880: Skin and Miffor modifiers cause mesh collapse #108915

Pull request closed