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

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

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

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.

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

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

Multires: Unsubdivide operator

Harbormaster completed remote builds in B7560: Diff 23698.Apr 13 2020, 11:11 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

Rebase
Remove unrelated changes

Harbormaster completed remote builds in B7563: Diff 23701.Apr 13 2020, 11:42 PM

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

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

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

rebuild_subdivisions.blend656 KBDownload

There is a crash when there is a loose geometry:

rebuild_subdivisions_loose_geometry.blend655 KBDownload

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

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

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

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

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

Vlad Kuzmin (Ssh4) awarded a token.Apr 29 2020, 11:30 AM

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

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.

Sculpt: Multires Unsubdivide
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 24243

release/scripts/startup/bl_ui/properties_data_modifier.py

source/blender/blenkernel/BKE_multires.h

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/multires_reshape.h

source/blender/blenkernel/intern/multires_reshape_util.c

source/blender/blenkernel/intern/multires_unsubdivide.h

source/blender/blenkernel/intern/multires_unsubdivide.c

source/blender/editors/object/object_intern.h

source/blender/editors/object/object_modifier.c

source/blender/editors/object/object_ops.c

Sculpt: Multires UnsubdivideClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 24243

release/scripts/startup/bl_ui/properties_data_modifier.py

source/blender/blenkernel/BKE_multires.h

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/multires_reshape.h

source/blender/blenkernel/intern/multires_reshape_util.c

source/blender/blenkernel/intern/multires_unsubdivide.h

source/blender/blenkernel/intern/multires_unsubdivide.c

source/blender/editors/object/object_intern.h

source/blender/editors/object/object_modifier.c

source/blender/editors/object/object_ops.c

Sculpt: Multires Unsubdivide
ClosedPublic
Actions

Revision Contents
Changeset List