Page MenuHome

Multires: Delete Lower
Needs ReviewPublic

Authored by Pablo Dobarro (pablodp606) on Apr 30 2020, 11:35 PM.
Tokens
"Love" token, awarded by bnzs."Burninate" token, awarded by AnityEx."Like" token, awarded by VertexSupa."100" token, awarded by Schamph."Love" token, awarded by brilliant_ape."Pterodactyl" token, awarded by hitrpr."Love" token, awarded by Isfuelo."Party Time" token, awarded by Regnas."Love" token, awarded by kouza."Love" token, awarded by lopoIsaac."Love" token, awarded by franMarz."Burninate" token, awarded by imad_mss."Love" token, awarded by Brandon777."Love" token, awarded by monio."Love" token, awarded by andruxa696."100" token, awarded by Frozen_Death_Knight."Like" token, awarded by TheRedWaxPolice.

Details

Summary

This implements the multires_lower_levels_delete operator, which creates
a new base mesh from the current sculpt or preview level and keeps the
higher sculpted detail.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D7582 (branched from master)
Build Status
Buildable 8796
Build 8796: arc lint + arc unit

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Apr 30 2020, 11:35 PM
Pablo Dobarro (pablodp606) created this revision.
Sergey Sharybin (sergey) requested changes to this revision.May 1 2020, 9:44 AM

Please base new work on existing fundamentals. Don't re-implement multires modifier apply logic, there is a function for this which can be easily made very simple to use, understand, and not have logic duplicated.

Study what is already done. For example we do have multiresModifier_del_levels() which has deletion of lower levels as a TODO. Solving such TODOs as a new code only adds confusion.

Work on a system as a whole, not as adding code to achieve localised goals.

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

Comment is odd. What's unsubdivide doing here?

2072–2077

I can't understand this. I would imagine you will be deleting all levels below the current one.

Why is it exactly you're deleting top_level - current_level levels?

2086

Comment starts with capital letter and ends with full stop.

2087–2100

result is very bad name here.
I guess you've copied it from modifier code, where it is indeed a result. Intuitively, result in this operator is a mesh with lower levels deleted.

We also have BKE_multires_create_mesh which implements almost the same logic you need here with the difference that it uses deformed mesh as a base for multires. We should have BKE_multires_create_mesh_from_base and BKE_multires_create_mesh_from_deform in multires.c.

This revision now requires changes to proceed.May 1 2020, 9:44 AM
Pablo Dobarro (pablodp606) marked 4 inline comments as done.
  • Cleanup
  • Rebase
  • Review update
  • Remove unused headers

Not sure if its permitted to comment here (if so, feel free to delete this). In the video, once Pablo "Deleted Lower" subdivisions, and all the numbers dropped to "0", I noticed how the lack of "number of subdivisions" information is prone to create confusion. Nowhere in the Multires panel it says how many subdivisions there are in total (or if there are higher levels than where the user is currently in). This could lead to situations where the user start sculpting at level 0 (maybe after Deleting lower levels or not) and doesn't know/notice that he is also interfering with higher levels of the sculpt (that he doesn't remember or doesn't know of).
This could be prevented by showing how many subdivs are there in total, for example with a "0/n" instead of only the number of the current level that the user is in. Another option is to display a warning red message in the panel ex: "There are sculpted levels above the current one" to indicate to the user that his sculpting will interfere with higher levels.
Here are the two examples:

or

@Evandro Costa (Arkhangels) It is already planned to add that info to the viewport (D7561) and we will also add that to the modifier panel after the drag and drop refactor, include more info like how many vertices the next subdivision will produce and so on.

Hope its allowed to poke this. Its seems ready for review again.

Sergey Sharybin (sergey) requested changes to this revision.Jun 2 2020, 12:38 PM

Is the Unsubdivide of top level guaranteed to give same exact topology as preview level created by multires?

Trying to delete lower on a deformed mesh causes some visual difference, which could indicate mistake in logic somewhere (mixing deformed and base mesh somewhere)

source/blender/blenkernel/intern/multires.c
289

This function was renamed to BKE_modifier_get_info.

803

Subtract the number of removed layers rather than assigning to zero, so that render look stays the same even if you had preview levels set to a lower value than final render.

source/blender/editors/object/object_modifier.c
1565–1566

The fact that depsgraph is not needed is relying to a current implementation. If that ever changes, you will have a hard time (a) remembering that depsgraph is not always passed (b) finding all codepaths where is it not passed.

Check the code paths which accesses depsgraph in the deletion operator. If they do not require depsgraph to be fully evaluated pass CTX_data_depsgraph(C) here.

If for the delete lower depsgraph must be evaluated:

  • Split public API into 2 separate functions: for deleting lower and deleting higher, with the latter not having depsgraph argument.
  • Document the delete higher function that it must receive fulyl evaluated depsgraph.
This revision now requires changes to proceed.Jun 2 2020, 12:38 PM
Pablo Dobarro (pablodp606) marked 3 inline comments as done.
  • Review Update

I was trying to fix the bug with the deform modifier. I found this P1448, but it does not solve the problem.
The base mesh after unsubdivide should match the current preview level as it doesn't refit and it works fine in meshes without deform modifiers before multires.
Thinking about this, isn't what is happening in the file the correct behavior? If the base mesh changes after using delete lower the limit surface will change slightly. If you deform the mesh and apply a displacement that was calculated using the new limit surface the result should be different. If you move the deform modifier after multires it works as expected.

Thinking about this, isn't what is happening in the file the correct behavior?

As far as I remember, the last step of unsubdivide was related on reshape to the input. It totally makes sense because then no matter what happens with tangent space of the base mesh the final result never have unexpected deformation or artefacts. But then if the reshape happens to the proper state, then I am not sure why the result looks different.

If you move the deform modifier after multires it works as expected.

I am not sure what you're trying to imply here.

Deformation modifiers are supposed to be leading multires Generally, you rig as lowpoly mesh as you can, and add details on top.
It is also expected that at leats at current multires level / modifier stack state operations do not introduce unexpected deformation or artefacts.

source/blender/blenkernel/BKE_multires.h
95–102

State that deform state of the mesh is acquired from the current evaluated state of the object, and, hence, the caller might need to ensure depsgraph is fully evaluated.

Also, do it in a separate commit, which will not be intended to have any functional changes.

99

Base mesh is an input for multires/subdivision surface modifier. So from terminology point of view function description and naming does not do what one would expect it to.

Are we sure that this is not expected behavior? Delete Lower should be a similar operation to applying the multires modifier as they both generate a new base mesh, and they both slightly deform the mesh in the file you provided. In fact, if you apply the multires modifier the result is identical to using delete lower on the highest level, which is expected.

On the other hand, when unsubdivide converts the new grids from object space to tangent space right now it is using the new base mesh for the reshape context. I would also expect that using the object to create the new reshape context will convert the grids using the deformed tangent space created by the deformed base mesh, but it does not work (same result). Is the the correct way to do it?

DEG_id_tag_update(&object->id, ID_RECALC_COPY_ON_WRITE);
Depsgraph *evaluated_deg = CTX_data_ensure_evaluated_depsgraph(C);
Object *evaluated_ob = DEG_get_evaluated_object(evaluated_deg, object);

if (!multires_reshape_context_create_from_object(&reshape_context, evaluated_deg, evaluated_ob, mmd)) {
  return 0;
}
source/blender/blenkernel/BKE_multires.h
99

What naming and description would you suggest for these functions?

About expected behavior story.

Currently the reshape part of multires module was designed in a way that no visible changes happen at the current object state.

If some specific operation does not follow this behavior, it is fine if is clearly communicated. But also, need to make sure there is no mistakes in the code (note that in my initial comment on this issue I've said it could indicate the issue, not that it does have an issue ;).

From working on other tools such undesired deformations where an indication that logically wrong base mesh was wrong. If we know everything is used properly, we can accept the behavior and document it as such.

About the tangent space story.

Of course, changing base mesh changes the tangent space. For the Apply Base I have a code which refines the OpenSubdiv's mesh to the new base mesh coordinates so that tangent space is "updated" and the object is properly reshaped to its original object space coordinates, without causing any difference in the final shape (see the use of multires_reshape_apply_base_refine_from_deform).

Is similar idea easily exploitable for this tool as well?

Just a quick note.
It seems the patch is broken because of the drag and drop modifier patch.

  • Rebase

I rebased the patch to include the changes in the modifier UI. I also tried to fix the bug in unsubdivde using the reshape functions and creating the reshape context using the object, but it still does not work. Maybe we can commit it as it is and handle this issue as a bug in unsubdivide in a separate task?

I tested it and found a bug.
When you have a multires object and hide some verts in edit mode before you use the delete lower operator it will take endless to finish. Hope it helps.

Maybe we can commit it as it is and handle this issue as a bug in unsubdivide in a separate task?

I need more information first.
Is the behavior the same for "Unsubdivide" operator (outside of "Delete Lower" codepath)? Is there some fundamental design/mathematical problem which prevents to fix this bug now?

source/blender/blenkernel/intern/multires.c
796

@Sergey Sharybin (sergey) I tried to find the deformation bug again. If I remove everything from here (the unsubdivide step), so the operator only sets the base mesh of the object to subdiv_mesh, it appears as deformed. That is expected, right?
Maybe what is missing is storing the coordinates of the current subdivided mesh and then do a reshape after subdiv_mesh is copied to the object

source/blender/blenkernel/intern/multires.c
796

A bit hard to give definitive answer. Can you share patch with the removal and .blend file, so we are sure we are talking about same thing?

Also, did you test unsubdivide operator with deform modifier prior to the multires?

  • Add DELETE_LOWER_DEBUG option

@Sergey Sharybin (sergey) I added a DELETE_LOWER_DEBUG define to that part of the code. When it is 1 (unsubdivide step is skipped, so it is just copying the output from BKE_multires_create_mesh_from_base) the objects is deformed:

When it is 0 (unsubdivide step runs), the unsubdivide code seems to keep the previously introduced deformation correctly:

This is the file I'm using