Paths

Table of Contentst

DrawManager: Multires Smooth Sculpting
AbandonedPublic
Actions

Authored by Jeroen Bakker (jbakker) on May 29 2019, 4:29 PM.

Details

Reviewers

Clément Foucault (fclem)
Brecht Van Lommel (brecht)

Maniphest Tasks

T62282: Multires: normals not updating when using smooth shading

Summary

When sculpting multires the normals were not updated. This is really
anoying to sculpters. This patch ports back the original code over to
the draw manager.

Perhaps we want to optimize this as this code is updating normals for every taa sample .... seems to be a lot of overhead. Perhaps we should only do this and add a boolean when it needs to be updated. (first sample)

Diff Detail

Repository

rB Blender

Branch

T62282 (branched from master)

Build Status

Buildable 3775
Build 3775: arc lint + arc unit

Event Timeline

Jeroen Bakker (jbakker) created this revision.May 29 2019, 4:29 PM

Harbormaster completed remote builds in B3771: Diff 15682.May 29 2019, 4:29 PM

Harbormaster completed remote builds in B3771: Diff 15682.

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)May 29 2019, 4:33 PM

Jeroen Bakker (jbakker) added reviewers: Clément Foucault (fclem), Brecht Van Lommel (brecht).

Jeroen Bakker (jbakker) added inline comments.May 29 2019, 4:44 PM

source/blender/draw/intern/draw_manager_data.c
719 ↗	(On Diff #15682)	I ported over the old code. but it does not feel right to put this heavy code in the draw manager. Currently the normals will be updated for every TAA sample. I would expect that we don't want this. A quick fix would be to add a parameter when the normals may be updated and trigger this from EEVEE and Workbench when they draw the first TAA sample. We could also add a check availability check for the cases where the normals are actually needed `mmd`. But this seams to be more of a hack than a solution. Wouldn't it be better to update this in the actual sculpting/painting code.

Clément Foucault (fclem) added inline comments.May 29 2019, 5:11 PM

source/blender/draw/intern/draw_manager_data.c
719 ↗	(On Diff #15682)	I'm not sure that this should be done at this level. There should be some tagging happening from the sculpt stroke code and we should just check this here.

Not sure why this was in the drawing code in 2.7.

I'd expect updating PBVH normals to happen in sculpt_flush_update, next to the bounding boxes update.

Or if that's too late, maybe mesh_calc_modifier_final_normals in the depsgraph eval. Though I wouldn't expect the PBVH to be modified by the depsgraph so probably not needed?

Brecht Van Lommel (brecht) requested changes to this revision.May 29 2019, 6:13 PM

This revision now requires changes to proceed.May 29 2019, 6:13 PM

Moved update normals to sculpt_flush_update.

sculpt_flush_update normally only tags objects, but now it also updates the normals. Would be better to hide calculate it as mart of the MULTIRES_COORDS_MODIFIED.

There are some glitches during sculpting strokes that some normals are flickering.

Harbormaster completed remote builds in B3774: Diff 15695.May 31 2019, 8:28 AM

Added comment

Harbormaster completed remote builds in B3775: Diff 15696.May 31 2019, 8:50 AM

Jeroen Bakker (jbakker) marked an inline comment as done.May 31 2019, 9:07 AM

Clément Foucault (fclem) accepted this revision.May 31 2019, 12:15 PM

This doesn't quite work because it only runs after sculpting strokes, and leaves some missing updates.

I found there's a call to BKE_sculpt_update_mesh_elements that got disabled in the depsgraph code, that is causing all kinds of refresh issues. Let me look into that before we commit this.

Alternative fix committed in rBf87bba0368ea: Fix T62282: multires sculpting does not update smooth normals.

Revision Contents
Changeset List

Path

Size

source/

blender/

editors/

sculpt_paint/

sculpt.c

17 lines

Diff 15696

View Options

source/blender/editors/sculpt_paint/sculpt.c

DrawManager: Multires Smooth SculptingAbandonedPublicActions