Skip to content

Conversation

@MarkGillespie
Copy link
Collaborator

I added implementations of quadric error simplification, Catmull-Clark subdivision, and Loop subdivision. Quadric error simplification and Loop subdivision optionally take a MutationManager argument. Unfortunately, Catmull-Clark doesn't, since MutationManager is tailored for triangle meshes at the moment.

I also changed MutationManager's mutations to return the newly-created mesh element (i.e. mutationManager.collapseEdge(e) now returns the new vertex).

@nmwsharp
Copy link
Owner

nmwsharp commented Nov 5, 2021

This looks great, thank you for putting it together! FYI there is also some additional remeshing stuff in #86 that I really need to get merged, if any of it is useful to you.

Design question: should high-level remeshing schemes (such as the quadric simplification added here) be contained in the mutation manager, or external to it? E.g., should it be MutationManager::quadricSimplifiy(), or should it be quadricSimplify(..., mutationManager)? At first consideration, it seems like anything we want to do should be possible with either strategy, but I haven't thought about it too much. Do you have a good reason for one design vs. the other? I suppose one advantage is that the latter moves more logic outside the mutation manager, preventing it from becoming a gigantic mega-class.

@MarkGillespie
Copy link
Collaborator Author

Interesting point. I went for the second option since it was the easiest way to incorporate mutation managers into some of my existing code which didn't originally use them.

Aesthetically, the second option appeals to me. But the first may be fit better with the rest of geometry-central. There are already a lot of powerful mega-classes floating around :) And it does seem nice to give the mutation manager ownership of the algorithms which primarily mutate meshes.

One other question to consider is whether we expect to pass mutation managers to functions which live inside of other classes. E.g., would we ever want to pass a mutation manager to IntrinsicTriangulation::DelaunayRefine()? If we wanted to support that use case, then it would be more consistent to go with the first design choice. But now that I think about it, offering a mutation manager member variable in the intrinsic triangulation class seems much better than taking in random other mutation managers as function arguments, so I'm not sure we would ever want to do this.

@nmwsharp
Copy link
Owner

nmwsharp commented Dec 8, 2021

Just double-checking, is this good-to-go as far as you're concerned? if so I think we can merge it in. I didn't test it myself but everything looks sane

@MarkGillespie MarkGillespie merged commit b704f0e into nmwsharp:master Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants