Page MenuHome

Sculpt: Multires Unsubdivide
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Apr 8 2020, 1:56 AM.
Tags
None
Tokens
"Love" token, awarded by Ssh4."Love" token, awarded by jorsh."Love" token, awarded by Alumx."Like" token, awarded by TheRedWaxPolice."Evil Spooky Haunted Tree" token, awarded by filibis."Love" token, awarded by hitrpr."Love" token, awarded by drx."Burninate" token, awarded by AnityEx."Love" token, awarded by ZohaibAli."Love" token, awarded by Yegor."Love" token, awarded by MetinSeven."Yellow Medal" token, awarded by imad_mss."Burninate" token, awarded by LapisSea."Love" token, awarded by Brandon777."Love" token, awarded by 14AUDDIN."Piece of Eight" token, awarded by TheAngerSpecialist."Mountain of Wealth" token, awarded by franMarz."Love" token, awarded by brilliant_ape."100" token, awarded by Frozen_Death_Knight.

Details

Summary

This implements the main unsubdivide algorithm which rebuilds a base mesh and extracts the grid's data from a high resolution mesh.
It includes the Rebuild Subdivisions operator, which generates all subdivision levels down to the level 0 base mesh.

What works:

  • Rebuilding an arbitrary number of levels or as many levels as possible down to level 0 in a single step (not available in the main function, but very easy to enable).
  • Rebuilding with already existing grids.
  • Meshes with n-gons and triangles
  • Base mesh made completely out of triangles
  • Meshes without poles
  • Meshes with multiple disconnected elements at the same subdivision level

The main function of the algorithm is multires_unsubdivide_to_basemesh
It expects an already subdivided mesh (original_mesh) and it will output the level 0 base mesh and an array of MultiresUnsubdivideGrid indexed per base mesh loop. These structs contain the coordinates in object space in the correct order to build the grid for each loop.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Fix deformation of non-quad base mesh faces

This is how it works right now. It supports way more cases than what would be needed for importing and sculpt mode.

May I suggest to use the numeric system used for subdividing, by using negative numbers? This would make it easier for procedural workflow, while making non-destructing operations.

I'm aware this would mean a rework of how this value reacts to the modifier

Wait, so this is a different algorithm than the current unsubdivide function? Then, please, add it as its own command in edit mode and in the decimate modifier where the current Unsubdivide is. It seems to handle more/better cases. The old algorithm is still useful, though, for those mid-unsubdivisions that make diagonal faces.

May I suggest to use the numeric system used for subdividing, by using negative numbers? This would make it easier for procedural workflow, while making non-destructing operations.

I'm aware this would mean a rework of how this value reacts to the modifier

I really like this idea, it keeps the spirit of the original data preserved and helps indicate to the user how many levels astray they are from the original mesh at all times.

  • Extract new grids from existing grids

This should allow to unsubdivide only one single level at a time and rebuild subdivs with already existing subdivisions in the modifier

Pablo Dobarro (pablodp606) planned changes to this revision.Apr 13 2020, 4:46 AM
  • Multires: Unsubdivide operator

@Sergey Sharybin (sergey) @Brecht Van Lommel (brecht) I think this ready. I included an Unsubdivide and a Rebuild Subdivisions operator. Both use the same internal logic, but Unsubdivide is limited to rebuild one level at a time. In case that you need more control on which level you want to be your base mesh to be, unsubdivide should be used. In case that you imported a high poly model with 7 subdivision levels and you want to get all of them back, Rebuild Subdivisions is more convenient as it will rebuild the base mesh and extract the new grids in a single step instead of copying the same grid data and rebuild the base mesh 7 times.

  • Rebase
  • Remove unrelated changes
Sergey Sharybin (sergey) requested changes to this revision.Apr 14 2020, 9:44 AM

Please give it a good real stress-test and make sure corner cases do not crash. Rebuilding subdivisions of any object in the attached file seems to cause a crash here

It might also be a good idea to assign preview/sculpt/render layers to the reconstructed ones.

This revision now requires changes to proceed.Apr 14 2020, 9:44 AM
  • Fix crash when not lower subdivision level is found
  • Fix crash when freeing the extracted grid data

@Sergey Sharybin (sergey) What do you mean with assigning the layers? It is already assigning the lowest level when it can reconstruct it, so I think that is fine for most cases.

What I mean is: clicking on "Rebuild Subdivisions" currently makes it so object shape changes in the viewport. Try with the attached file.
More intuitive would be if Rebuild Subdivisions did assign preview/render levels to the reconstructed ones.

There is a crash when there is a loose geometry:

Sergey Sharybin (sergey) requested changes to this revision.Apr 15 2020, 11:09 AM

Marking as requested changes since there is still work to be done.
With explicit request to change the patch goes out of "Needs review" query.

This revision now requires changes to proceed.Apr 15 2020, 11:09 AM
  • Review update
  • Fix crash with loose geometry
  • Rebuild subdiv now keeps the preview, render and sculpt levels

Where this sculpting experimental branch can be downloaded?
Thanks in advance! :)

Sergey Sharybin (sergey) requested changes to this revision.Apr 28 2020, 12:33 PM
Sergey Sharybin (sergey) added inline comments.
source/blender/blenkernel/intern/multires_unsubdivide.c
57

Why do you use data layers? Why can't you use array aligned with the size of loops/verticies? Are you expecting them to be interpolated/reset when performing modifications?

If you do need some functionality specific to CD layers it needs to be explained here why. Otherwise don't use CD but use simple array.

329

Shadowing.

843–844

I still don't know what is it for. Some hardcoded generically named variables, with an unknown purpose.

Is this a part of Use the datalayers to map vertices from the base mesh to the original mesh and original to base mesh. ?

846

static

861

static

880

static

943

static

This revision now requires changes to proceed.Apr 28 2020, 12:33 PM
Pablo Dobarro (pablodp606) marked 7 inline comments as done.
  • Review update
source/blender/blenkernel/intern/multires_unsubdivide.c
57

Yes, I use datalayers to keep track of the indices of the original vertices when removing and dissolving elements when generating a new base mesh in each unsubdivide step. This is the only part where this is needed. All the other maps that are required are already created as arrays.

The loop indices are needed in multires_unsubdivide_extract_grids to get the index of the loop from the BMesh loop. I don't know if there is a better way to do this.

Some feedback on naming.

Think the plan of action should be:

  • Get feedback from the UI team (I have weird feeling of the look and feel of the modifier interface)
  • Try to make naming more clear (in the pointed places)
  • Move development to master
  • Fix all discovered things after merge
source/blender/blenkernel/intern/multires_unsubdivide.c
80–81

Is this safe form access past array boundaries point of view?

530

What is area?

540

What is p?

582

grid_size. Also in other occurrences.

source/blender/blenkernel/intern/multires_unsubdivide.h
41

Are they in object or tangent space?

52

num_original_levels

58

max_new_levels?

65

num_total_levels?

Basically, if it's a number use num prefix. And try to make it explicit without looking up comment what levels the field referring to.

source/blender/editors/object/object_modifier.c
1810

Is 13 referring to multires_max_levels ?

Not sure we should have such a hard limit actually. I don't think we have it in Subdivision Surface modifier, and the new Subdivide operator also doesn't use this limit.

If you strongly think we should have this limit, make an accessor function in multires.c to access multires_max_levels.

Pablo Dobarro (pablodp606) marked 8 inline comments as done.
  • Review update
source/blender/blenkernel/intern/multires_unsubdivide.c
80–81

That should never happen, the elem_id array is always the same size as the vertices array. If you test an elem id higher than the total number of ids it just returns false

From reading LGTM.

Maybe poke UI team, and move development to master.

source/blender/blenkernel/intern/multires_unsubdivide.c
547–554

Can order be changed to 0,1,2,3 ?

569–570

Add something like BLI_assert(!"Should never happen")

This revision is now accepted and ready to land.Apr 29 2020, 8:43 PM