Page MenuHome
Differential D7372

Sculpt: Multires Unsubdivide
ClosedPublic
Actions

Authored by Pablo Dobarro (pablodp606) on Apr 8 2020, 1:56 AM.
Tags
None
Subscribers
Casey (BagelCo)
Chris Kohl (ckohl_art)
Gerard Taulats (-Tabra-)
Julien Leménicier (mistajuliax)
LapisSea (LapisSea)
Martin Tégláš (drx)
Servando (Serva)
Volodymyr (linosia)
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

Reviewers
Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)
Commits
rBf28875a998d4: Multires: Unsubdivide and Rebuild Subdivisions
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
Branch
unsubdivide-basemesh (branched from master)
Build Status
Buildable 7556
Build 7556: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)Apr 11 2020, 3:57 PM
LapisSea (LapisSea) awarded a token.Apr 11 2020, 4:02 PM
LapisSea (LapisSea) added a subscriber: LapisSea (LapisSea).
imad (imad_mss) awarded a token.Apr 11 2020, 4:30 PM
Julien Leménicier (mistajuliax) added a subscriber: Julien Leménicier (mistajuliax).Apr 11 2020, 4:54 PM
Metin Seven (MetinSeven) awarded a token.Apr 11 2020, 5:10 PM
Pablo Dobarro (pablodp606) added a comment.Apr 11 2020, 6:47 PM
This comment was removed by Pablo Dobarro (pablodp606).
Pablo Dobarro (pablodp606) updated this revision to Diff 23656.EditedApr 11 2020, 7:31 PM
  • 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.

Harbormaster completed remote builds in B7543: Diff 23656.Apr 11 2020, 7:31 PM
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)Apr 11 2020, 7:38 PM
Gerard Taulats (-Tabra-) added a subscriber: Gerard Taulats (-Tabra-).Apr 11 2020, 8:00 PM

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

Wo!262 (wo262) added a subscriber: Wo!262 (wo262).Apr 11 2020, 8:09 PM

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.

Casey (BagelCo) added a subscriber: Casey (BagelCo).Apr 11 2020, 8:18 PM
In D7372#177337, @Gerard Taulats (-Tabra-) wrote:

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.

Yegor (Yegor) awarded a token.Apr 11 2020, 9:14 PM
Zohaib (ZohaibAli) awarded a token.Apr 11 2020, 9:14 PM
Chris Kohl (ckohl_art) added a subscriber: Chris Kohl (ckohl_art).Apr 11 2020, 11:45 PM
AnityEx (AnityEx) awarded a token.Apr 12 2020, 12:42 AM
Martin Tégláš (drx) awarded a token.Apr 12 2020, 1:51 AM
Martin Tégláš (drx) added a subscriber: Martin Tégláš (drx).
Vyacheslav (hitrpr) awarded a token.Apr 12 2020, 9:35 AM
Emir Sinan Gürlek (filibis) awarded a token.Apr 12 2020, 2:35 PM
Pablo Dobarro (pablodp606) updated this revision to Diff 23688.Apr 13 2020, 4:46 AM
  • 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

Harbormaster completed remote builds in B7556: Diff 23688.Apr 13 2020, 4:46 AM
Pablo Dobarro (pablodp606) planned changes to this revision.Apr 13 2020, 4:46 AM
Pablo Dobarro (pablodp606) updated this revision to Diff 23698.Apr 13 2020, 11:10 PM
  • Multires: Unsubdivide operator
Harbormaster completed remote builds in B7560: Diff 23698.Apr 13 2020, 11:11 PM
Pablo Dobarro (pablodp606) added a comment.Apr 13 2020, 11:17 PM

@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.

Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)Apr 13 2020, 11:18 PM
TheRedWaxPolice (TheRedWaxPolice) awarded a token.Apr 13 2020, 11:23 PM
Pablo Dobarro (pablodp606) updated this revision to Diff 23701.Apr 13 2020, 11:42 PM
  • Rebase
  • Remove unrelated changes
Harbormaster completed remote builds in B7563: Diff 23701.Apr 13 2020, 11:42 PM
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

unsubdivide_crash.blend1011 KBDownload

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
Pablo Dobarro (pablodp606) updated this revision to Diff 23750.Apr 15 2020, 12:08 AM
  • Fix crash when not lower subdivision level is found
  • Fix crash when freeing the extracted grid data
Harbormaster completed remote builds in B7582: Diff 23750.Apr 15 2020, 12:08 AM
Pablo Dobarro (pablodp606) added a comment.Apr 15 2020, 12:10 AM

@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.

Sergey Sharybin (sergey) added a comment.Apr 15 2020, 10:01 AM

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.

rebuild_subdivisions.blend656 KBDownload

There is a crash when there is a loose geometry:

rebuild_subdivisions_loose_geometry.blend655 KBDownload

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
Pablo Dobarro (pablodp606) updated this revision to Diff 23777.Apr 15 2020, 6:29 PM
  • Review update
  • Fix crash with loose geometry
  • Rebuild subdiv now keeps the preview, render and sculpt levels
Harbormaster completed remote builds in B7590: Diff 23777.Apr 15 2020, 6:29 PM
Vas (Alumx) awarded a token.Apr 26 2020, 6:47 AM
Volodymyr (linosia) added a subscriber: Volodymyr (linosia).Apr 26 2020, 10:25 AM

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) updated this revision to Diff 24155.Apr 28 2020, 4:31 PM
Pablo Dobarro (pablodp606) marked 7 inline comments as done.
  • Review update
Harbormaster completed remote builds in B7763: Diff 24155.Apr 28 2020, 4:31 PM
Wo!262 (wo262) removed a subscriber: Wo!262 (wo262).Apr 28 2020, 4:31 PM
Pablo Dobarro (pablodp606) added inline comments.Apr 28 2020, 4:31 PM
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.

Pablo Dobarro (pablodp606) added a child revision: D7560: Object: Switch Subdivision Level operator.Apr 28 2020, 9:17 PM
jorge (jorsh) awarded a token.Apr 29 2020, 12:38 AM
Sergey Sharybin (sergey) added a comment.Apr 29 2020, 11:05 AM

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
1804

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.

Vlad Kuzmin (Ssh4) awarded a token.Apr 29 2020, 11:30 AM
Pablo Dobarro (pablodp606) updated this revision to Diff 24212.Apr 29 2020, 7:46 PM
Pablo Dobarro (pablodp606) marked 8 inline comments as done.
  • Review update
Harbormaster completed remote builds in B7800: Diff 24212.Apr 29 2020, 7:46 PM
Pablo Dobarro (pablodp606) added inline comments.Apr 29 2020, 7:46 PM
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

Sergey Sharybin (sergey) accepted this revision.Apr 29 2020, 8:43 PM

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
Pablo Dobarro (pablodp606) mentioned this in D7522: Add Multires experimental features.Apr 30 2020, 2:42 PM
Closed by commit rBf28875a998d4: Multires: Unsubdivide and Rebuild Subdivisions (authored by Pablo Dobarro (pablodp606)). · Explain WhyApr 30 2020, 4:53 PM
This revision was automatically updated to reflect the committed changes.
Pablo Dobarro (pablodp606) added a commit: rBf28875a998d4: Multires: Unsubdivide and Rebuild Subdivisions.

Revision Contents
Changeset List

PathSize
intern/
ghost/
intern/
4 lines
release/
scripts/
startup/
bl_ui/
1 line
source/
blender/
blenkernel/
3 lines
2 lines
intern/
5 lines
33 lines
94 lines
1164 lines
editors/
object/
1 line
53 lines
1 line

Diff 23688

View Options

intern/ghost/intern/GHOST_WindowWin32.h

Loading...
View Options

release/scripts/startup/bl_ui/properties_data_modifier.py

Loading...
View Options

source/blender/blenkernel/BKE_multires.h

Loading...
View Options

source/blender/blenkernel/CMakeLists.txt

Loading...
View Options

source/blender/blenkernel/intern/multires_reshape.h

Loading...
View Options

source/blender/blenkernel/intern/multires_reshape_util.c

Loading...
View Options

source/blender/blenkernel/intern/multires_unsubdivide.h

Loading...
View Options

source/blender/blenkernel/intern/multires_unsubdivide.c

Loading...
View Options

source/blender/editors/object/object_intern.h

Loading...
View Options

source/blender/editors/object/object_modifier.c

Loading...
View Options

source/blender/editors/object/object_ops.c

Loading...