[Modifiers] Screw modifier generates doubles.
AbandonedPublic
Actions

Authored by Ray Molenkamp (LazyDodo) on Jun 24 2017, 8:26 PM.

Details

Reviewers

Bastien Montagne (mont29)
Campbell Barton (campbellbarton)

Summary

The screw modifier generates doubles for points that lie on the axis of rotation, this patch adds and option to remove those doubles so you can safely stack for instance a solidify modifier on top of the screw (this would generate a hole in the mesh otherwise)

Example file:

vase.blend192 KBDownload

without merged verticies:

with merged vertices:

I put the function in MOD_util so other modifiers may in the future call this function to get rid of their doubles.
I decided not to add a merge distance in the UI since only vertices on the axis will be generating doubles and FLT_EPSILON will catch them.

Diff Detail

Repository: rB Blender

Event Timeline

Ray Molenkamp (LazyDodo) created this revision.Jun 24 2017, 8:26 PM

looks good :)

mathieu menuet (bliblubli) awarded a token.Jun 30 2017, 6:47 PM

It's been over month with no activity, no idea who normally does modifier code review, @Bastien Montagne (mont29) can you take a peek here?

Hmmmm… this is way too late to add anything like that in master for 2.79 now… :/

Also, making this merge code generic is a good idea, but it would be even better to also deduplicate already existing one (see e.g. merge code from array modifier).

This revision now requires changes to proceed.Jul 26 2017, 3:09 PM

In D2724#64721, @Bastien Montagne (mont29) wrote:

Hmmmm… this is way too late to add anything like that in master for 2.79 now… :/

Yeahh no need to rush this into 2.79 (it'll go into a later update, no biggie), but still it sat idle for over a month.. we've kinda been slacking at coderviews lately understandable (with the 2.8 stuff and the 2.79 bug crunch), but still, not great... D2731 brand new contributor, submitted a patch, got crickets... same for D2728 , D2701 seemed to have already moved on to other things once we finally got back to them, D2606 with reviewers assigned, crickets since april. (and those are just the ones i tracked down in a few minutes, i assume there's more) If we're trying to attract new devs, this is not a great way to do it. Adding @Ton Roosendaal (ton) just to inform him this is an area where we *should* do better (but no action required from him for this ticket)

Also, making this merge code generic is a good idea, but it would be even better to also deduplicate already existing one (see e.g. merge code from array modifier).

I did look at the array modifier but it seemed to be performing the merge in stages, figured there was a reason for that that i did not understand and decided not to mess with it.

Cleaned up the array modifier

Rmhh, sorry about that, but array code is already as factorized as possible in fact (did not check/remember it precisely the other day), your change just nuke all the carefully crafted creation of the double_map, which is just bad. ;)

Main part of the work is already factorized into CDDM_merge_verts, so better leave array code untouched.

dm_mvert_map_double() also needs changes, details inlined.

source/blender/modifiers/intern/MOD_util.c
237	use `\return` (doxygen syntax).
245	please follow our code style (space after comma, between operators, etc.)
247–266	This whole block will be horribly slow with big meshes (roughly `num_verts * num_verts / 2`, 1k verts lead to 500k potential calls to `len_squared_v3v3()`…), should rather use BVHtree (see e.g. how vgroup proximity modifier does it).
253–254	While this may look smart on first sight, it has one main drawback: you cannot use default BVHTree callbacks in case you want to keep that behavior (since comparison func for bvhtree walking would have to be aware of this). So i would rather just follow mapping in case closest vert is already mapped. This may lead to merging vertices slightly more distant than given threshold, but it will also give more stable results even if vert order changes. And unless threshold is poorly chosen given the mesh density (leading to long 'mapping chains'), it won't have any noticeable impact in the end.

This revision now requires changes to proceed.Sep 5 2017, 3:17 PM

Bastien Montagne (mont29) mentioned this in T52474: Bevel modifier - Clamp Overlap broken.Sep 5 2017, 3:18 PM

reverted the array modifier changes.
Changed the finding of doubles to use the bvhtree, but I have to admit, i'm a little out of my comfort zone there. the parameters to bvhtree_from_mesh_verts might as well have been magic numbers.
I *think* this is working, but I have 1 test case, so it doesn't cover much surface.
The Transfering of data between dm_remove_doubles and remove_doulbes_callback feels kinda hacky
I don't like that I can't call BLI_bvhtree_free cause it's some stored in the DM which will take care of freeing it?

In conclusion, did the changes you wanted, but i'm a little uncomfortable with the whole thing and wouldn't be surprised if i did things terribly wrong.

Ideally the doubles would not be created in the first place (create triangles instead of quads for vertices on the axis).

But this gets a bit involved and complicates existing code.

OTOH - doing a full remove doubles is also overkill in this case.

Unless I miss something, here is how I think it should work:

Loop over input, tag vertices close to the revolution axis (in an array for eg). Then the doubles-map can be generated by merging all copies into the first vertex on the axis that's marked as a double, this gives predictable geometry & topology (higher merge limit wont merge edges or unrelated vertices) without any need for spatial search.

The code for this shouldn't be all that complicated and can be isolated in a single function that runs at the end.

This revision now requires changes to proceed.Sep 6 2017, 6:55 AM

I give up, I'm done.

first this is ignored for weeks on end.
Hey it's not related to the screw bug you had, but please go clean up the array modifier well!
undo your array hack, and use bvhtree instead!
don't use bvhtree! do something else!

doing a 180 once,fair enough, every time I do what you say? nope

this is how we lose devs guys...

Hey @Ray Molenkamp (LazyDodo), it's understandable and agree it's not nice to get pulled in different directions.

I've committed the changes suggested rB584523e0adeb2663077602953f0d3288c4c60fe4

@Bastien Montagne (mont29) - maybe some of this patch could still be used?

I found this patch investigating T52474

Since we now have the Weld modifier, we could move a large part of its code to MOD_util.c.
I don't recommend getting the duplicate within the utility since this is the most costly part of the function.
Duplicates should be passed as a parameter.