Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1814)

Issue 6135048: Sculpt masking (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 7 months ago by nicholasbishop
Modified:
12 years, 6 months ago
Reviewers:
brechtvl, bf-codereview
Base URL:
https://svn.blender.org/svnroot/bf-blender/trunk/blender/
Visibility:
Public.

Description

This patch implements a mask layer for sculpt mode. Masking attenuates other sculpt brushes. Documentation: http://wiki.blender.org/index.php/User:Nicholasbishop/PaintMasks Branch on github: https://github.com/nicholasbishop/blender-bishop/tree/masking I'm appending the commit log here, hopefully gives useful explanations of the changes: commit addeb52 Code cleanup for parameters of subsurf_make_derived_from_derived. Replaced four boolean parameters with a single flag and a new enum, SubsurfFlags. commit 440c385 Code cleanup for multires_dm_create_from_derived(). Changed name to multires_make_derived_from_derived() and parameter order to be more similar to subsurf_make_derived_from_derived(). Added MultiresFlags enum with flag values to replace the local_mmd and useRenderParams parameters. commit db383bb Add an RNA access function to get an enum item name from its value. New function is RNA_enum_name_from_value. commit 6d74d04 Add new options to PAINT_OT_brush_select, toggle and create_missing. The toggle option, if enabled, will toggle back and forth between two brushes. (The first brush of the desired tool type will be toggled to, running the toggle again switches back to the previously selected brush.) If no brush of the desired type is found, and the create_missing option is enabled, a new brush of that type will be created and set. commit 0c36a12 Modify CCGSubsurf to subdivide an arbitrary number of (float) layers. The layout of vert data in CCGSubSurf is almost the same; previously it was three floats (for xyz coordinate) optionally followed by three floats for the normal. The only change is that the first three floats can now be any number of floats. * _getSubSurf takes a numLayers parameter to set the number of layers, stored in CCGMeshIFC.numLayers. * All calls to _getSubSurf currently have numLayers set to 3, except for UV subsurf, where it is reduced to 2 (with a corresponding change when reading the results out to use float (*)[2] rather than float (*)[3].) * The various VertData* macros in CCGSubSurf.c are now functions that take a CCGSubSurf pointer, which provides access to CCGMeshIFC, which has numLayers. * Add ccgSubSurf_setNumLayers() to the API. Only changes the number of layers that get subdivided, doesn't change the amount of memory allocated. So if space for N layers is allocated, it's safe to set the number of layers to less than N, but not more. * The rest of the changes are just adding the 'ss' parameter. commit 8569e47 Add CCGKey/CCGElem for accessing CCGSubSurf elements. CCGKey caches information about the CCGSubSurf element layout. This data, along with the CCG_* inline functions, allows access to CCGSubSurf elements with an arbitrary number of layers (as opposed to the hardcoded DMGridData structure which assumes xyz coordinates followed by three normal components.) The CCGElem structure is declared but not defined anywhere, just used as a convenient type. commit e1c9b8c Replace hardcoded DMGridData structure with CCGElem/CCGKey. * Changes to DerivedMesh interface: DMGridData has been removed, getGridData() now returns an array of CCGElem pointers. Also added getGridKey() to initialize a CCGKey (implemented only by CCGDerivedMesh.) * PBVH: added BLI_pbvh_get_grid_key(). * A lot of code is affected, but mainly is just replacing DMGridData.co, DMGridData.no, and sizeof(DMGridData) with the CCG_*_elem functions, removing the reliance on grid elements of exactly six floats. commit ef319c0 Add DNA and customdata entries for paint masks. CD_PAINT_MASK is a layer of per-vertex floats for non-multires meshes. Multires meshes use CD_GRID_PAINT_MASK, which is a layer of per-loop GridPaintMask structures. GridPaintMask is similar to MDisp, but contains an array of scalar floats. Note: the GridPaintMask could be folded into MDisp, but this way should be easier to add mask layers in the future (if we do fold GridPaintMask into MDisp, the mask array should probably be an array of arrays with a 'totmask' field so that mask layers can be easily supported.) Includes blenload read/write support for CD_PAINT_MASK and CD_GRID_PAINT_MASK. commit 1663d60 Add access to mesh vertex customdata to the PBVH. commit 84f4de4 Add paint mask access to the PBVH vertex iterator. commit e51abec Add GridPaintMask accessor to paint.c. commit 93e0537 Add mask support to CCGSubSurf and multires. * Add new CCG function ccgSubSurf_setAllocMask(). Similar to to ccgSubSurf_setCalcVertexNormals(), it sets whether the CCG elements have a mask layer and what that layer's offset is. Unlike normals however, it doesn't change any behavior during CCG calculation; it's there only to give CCGKey information on the mask. * Add a new flag to _getSubSurf(), CCG_ALLOC_MASK. If set, space for an extra layer is allocated, but the number of CCG layers is not set to include it. This is done because GridPaintMasks are absolute, rather than being relative to the subdivided output (as MDisp displacements are), so we skip subdividing paint masks here. * Add a new flag to subsurf_make_derived_from_derived(), SUBSURF_ALLOC_PAINT_MASK. This controls whether CCG_ALLOC_MASK is set for _getSubSurf(). Related, masks are never loaded in during ss_sync_from_derivedmesh(). After subdivision is finished, if the alloc mask flag is set, the number of CCG layers is increase to 4 with ccgSubSurf_setNumLayers(). * Add a new flag to multires_make_from_derived(), MULTIRES_ALLOC_PAINT_MASK. Not all multires functions need paint mask data (e.g. multiresModifier_base_apply.) This flag is always set in MOD_multires.c so that subdividing a mesh with a mask updates properly even when not in sculpt mode. * Update multiresModifier_disp_run() to apply, calculate, and add mask elements. It's almost the same as the existing operations with xyz coordinates, but treats masks as absolute rather than displacements relative to subdivided values. * Update multires_customdata_delete to free CD_GRID_PAINT_MASK in addition to CD_MDISPS. * Update multires_del_higher() to call the new function multires_grid_paint_mask_downsample(), which allocates a lower-resolution paint mask grid and copies values over from the high-resolution grid. commit cc8addc Copy GridPaintMask to vertex paint mask when applying multires. Adds new subsurf_copy_grid_paint_mask() function similar to subsurf_copy_grid_hidden(). commit 4945e0b Ensure mask layers are always present in sculpt mode. commit f29b5b2 Add undo/redo support for paint masks. commit e6a7082 Use paint mask when calculating sculpt strength. commit 03b47bf Add new mask-brush icon from Julio Iglesias. commit ec2835b Add mask brush for sculpt mode. The mask brush currently has two modes, 'draw' and 'smooth'. commit ed796af Update the keymap for the mask brush. * Add MKEY as a toggle for the mask brush. We could use ALT similar to SHIFT toggling the smooth brush, but it would conflict with MMB emulation (not to mention many window managers.) * When the mask brush is active, SHIFT toggles it into smooth mode. commit cb6a3c0 Add a paint mask operator to clear, fill, or invert the mask. commit dde5522 Add support for hiding masked regions. Add a new mode, PARTIALVIS_MASKED, to the PAINT_OT_hide_show operator. commit c817577 Add keymap and menu entries for masking. * Add CTRL+IKEY to invert the mask. * Add ALT+MKEY to clear the mask. * Change the 'Hide' menu in sculpt mode to 'Hide/Mask', adds entires for clearing, filling, and inverting the mask, as well as hiding masked regions. commit 5190202 Use VertexBufferFormat for multires VBO. commit 86e61e6 Add mask-drawing support to GPU_Buffers. * For VBO, add color to the VertexBufferFormat structure as three unsigned bytes. Since mask elements are scalar the three color components are identical to eachother, but the fixed-function OpenGL pipeline requires colors to be either three or four components. * For the same reason, multires VBO drawing now copies into the VertexBufferFormat format as well. * Regression: material colors will not show up correctly now, masks colors are overriding. Not sure how to fix this nicely (would be much easier to fix if drawing with vertex shaders.) * Also, masks will only draw PBVH drawing, so only 'solid' drawing will work correctly with masks.

Patch Set 1 : Let's try that again with the correct diff attached #

Patch Set 2 : Update to apply cleanly against trunk, no other changes #

Patch Set 3 : Update to apply cleanly against trunk, no other changes #

Patch Set 4 : Update to apply cleanly against trunk, no other changes #

Patch Set 5 : Update to apply cleanly against trunk and fixed up some code style #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+2701 lines, -825 lines) Patch
release/datafiles/brushicons/mask.png View 0 chunks +-1 lines, --1 lines 0 comments Download
release/scripts/startup/bl_ui/space_view3d.py View 1 2 3 4 3 chunks +20 lines, -3 lines 0 comments Download
release/scripts/startup/bl_ui/space_view3d_toolbar.py View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
source/blender/blenkernel/BKE_DerivedMesh.h View 1 2 3 4 3 chunks +4 lines, -6 lines 0 comments Download
source/blender/blenkernel/BKE_ccg.h View 1 chunk +169 lines, -0 lines 1 comment Download
source/blender/blenkernel/BKE_multires.h View 1 2 3 4 2 chunks +11 lines, -3 lines 0 comments Download
source/blender/blenkernel/BKE_paint.h View 1 2 3 4 3 chunks +6 lines, -0 lines 0 comments Download
source/blender/blenkernel/BKE_subsurf.h View 1 2 3 4 4 chunks +16 lines, -5 lines 0 comments Download
source/blender/blenkernel/intern/CCGSubSurf.h View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
source/blender/blenkernel/intern/CCGSubSurf.c View 1 2 3 4 36 chunks +325 lines, -229 lines 0 comments Download
source/blender/blenkernel/intern/cdderivedmesh.c View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
source/blender/blenkernel/intern/customdata.c View 1 2 3 4 5 chunks +47 lines, -5 lines 0 comments Download
source/blender/blenkernel/intern/multires.c View 1 2 3 4 36 chunks +215 lines, -108 lines 0 comments Download
source/blender/blenkernel/intern/paint.c View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
source/blender/blenkernel/intern/shrinkwrap.c View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
source/blender/blenkernel/intern/subsurf_ccg.c View 1 2 3 4 62 chunks +287 lines, -168 lines 0 comments Download
source/blender/blenlib/BLI_pbvh.h View 1 2 3 4 10 chunks +22 lines, -11 lines 0 comments Download
source/blender/blenlib/intern/pbvh.c View 1 2 3 4 20 chunks +42 lines, -25 lines 0 comments Download
source/blender/blenloader/intern/readfile.c View 1 2 3 4 2 chunks +15 lines, -0 lines 0 comments Download
source/blender/blenloader/intern/writefile.c View 1 2 3 4 3 chunks +26 lines, -0 lines 0 comments Download
source/blender/editors/datafiles/CMakeLists.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
source/blender/editors/datafiles/mask.png.c View 1 chunk +400 lines, -0 lines 0 comments Download
source/blender/editors/include/ED_datafiles.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
source/blender/editors/include/ED_sculpt.h View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
source/blender/editors/include/UI_icons.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
source/blender/editors/interface/interface_icons.c View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
source/blender/editors/object/object_bake.c View 1 2 3 4 8 chunks +22 lines, -17 lines 0 comments Download
source/blender/editors/object/object_modifier.c View 1 2 3 4 3 chunks +9 lines, -1 line 0 comments Download
source/blender/editors/sculpt_paint/CMakeLists.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
source/blender/editors/sculpt_paint/paint_hide.c View 1 2 3 4 9 chunks +33 lines, -17 lines 0 comments Download
source/blender/editors/sculpt_paint/paint_intern.h View 1 2 3 4 2 chunks +12 lines, -1 line 0 comments Download
source/blender/editors/sculpt_paint/paint_mask.c View 1 chunk +143 lines, -0 lines 0 comments Download
source/blender/editors/sculpt_paint/paint_ops.c View 1 2 3 4 9 chunks +86 lines, -16 lines 0 comments Download
source/blender/editors/sculpt_paint/sculpt.c View 1 2 3 4 40 chunks +355 lines, -107 lines 0 comments Download
source/blender/editors/sculpt_paint/sculpt_intern.h View 1 2 3 4 3 chunks +4 lines, -2 lines 0 comments Download
source/blender/editors/sculpt_paint/sculpt_undo.c View 1 2 3 4 8 chunks +76 lines, -6 lines 0 comments Download
source/blender/editors/uvedit/uvedit_unwrap_ops.c View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
source/blender/gpu/GPU_buffers.h View 1 2 3 4 2 chunks +7 lines, -8 lines 0 comments Download
source/blender/gpu/intern/gpu_buffers.c View 1 2 3 4 17 chunks +218 lines, -56 lines 1 comment Download
source/blender/makesdna/DNA_brush_types.h View 1 2 3 4 4 chunks +9 lines, -2 lines 0 comments Download
source/blender/makesdna/DNA_customdata_types.h View 1 2 3 4 3 chunks +8 lines, -2 lines 0 comments Download
source/blender/makesdna/DNA_meshdata_types.h View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
source/blender/makesrna/RNA_access.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
source/blender/makesrna/intern/rna_access.c View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
source/blender/makesrna/intern/rna_brush.c View 1 2 3 4 6 chunks +24 lines, -2 lines 0 comments Download
source/blender/modifiers/intern/MOD_multires.c View 1 2 3 4 4 chunks +20 lines, -2 lines 0 comments Download
source/blender/modifiers/intern/MOD_subsurf.c View 1 2 3 4 2 chunks +12 lines, -4 lines 0 comments Download
source/tools/check_style_c.py View 1 2 3 4 6 chunks +7 lines, -16 lines 0 comments Download

Messages

Total messages: 8
nicholasbishop
12 years, 7 months ago (2012-04-28 03:51:57 UTC) #1
nicholasbishop
Let's try that again with the correct diff attached
12 years, 7 months ago (2012-04-28 03:53:39 UTC) #2
nicholasbishop
Update to apply cleanly against trunk, no other changes
12 years, 7 months ago (2012-04-28 20:26:49 UTC) #3
nicholasbishop
Update to apply cleanly against trunk, no other changes
12 years, 6 months ago (2012-05-04 03:34:28 UTC) #4
nicholasbishop
Update to apply cleanly against trunk, no other changes
12 years, 6 months ago (2012-05-06 15:12:36 UTC) #5
nicholasbishop
Update to apply cleanly against trunk and fixed up some code style
12 years, 6 months ago (2012-05-07 00:19:33 UTC) #6
brechtvl
Well, I can't find much wrong with this. The changes are clear and make sense. ...
12 years, 6 months ago (2012-05-10 11:42:58 UTC) #7
nicholasbishop
12 years, 6 months ago (2012-05-10 21:26:53 UTC) #8
Thanks for reviewing :)

> Turning DMGridData into something more flexible is fine with me. I wonder a
bit
> about performance with those CCG_elem_* accessor functions used everywhere, is
> there any performance regression sculpting high resolution meshes?

On my ad hoc testing I didn't find performance to be noticeably different, but
on the other hand I don't have a good automated way of verifying this. No
performance drop reported by testers either.

> If there is a significant performance regression, I guess inlining these
> functions and put CCGKey on the stack so the compiler can keep the used
members
> in registers might help.

The functions are BLI_INLINE (except for the CCGKey initialization functions),
so that should be OK.

> Also it seems the mask is now created everytime sculpting is used, increasing
> memory usage a bit. I guess it doesn't take up too much memory since it's just
> one float?

Yep, figured it was an OK tradeoff to make masks always present since they'll
probably be a frequently used part of the sculpt workflow.

> 
> LGTM to go into trunk now.
> 
>
http://codereview.appspot.com/6135048/diff/20003/source/blender/blenkernel/BK...
> File source/blender/blenkernel/BKE_ccg.h (right):
> 
>
http://codereview.appspot.com/6135048/diff/20003/source/blender/blenkernel/BK...
> source/blender/blenkernel/BKE_ccg.h:36: #include <stdio.h>
> Just always include it, no need to check for NDEBUG.
> 
>
http://codereview.appspot.com/6135048/diff/20003/source/blender/gpu/intern/gp...
> File source/blender/gpu/intern/gpu_buffers.c (right):
> 
>
http://codereview.appspot.com/6135048/diff/20003/source/blender/gpu/intern/gp...
> source/blender/gpu/intern/gpu_buffers.c:1273: drivers (Gallium/Radeon) */
> I'd remove the XXX, for me that indicates something broken. It makes sense
that
> it helps to align struct members.

Changes made, and masking committed as revisions 46509 - 46532.

Thanks,
-Nicholas
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b