Page MenuHome

Transform Snap: added nearest face snap mode, added snapping options, lightly refactored snapping code.
ClosedPublic

Authored by Jon Denning (gfxcoder) on Apr 7 2022, 8:26 PM.

Details

Summary

This diff adds a new face nearest snapping mode, adds new snapping options, and (lightly) refactors code around snapping.

The new face nearest snapping mode will snap transformed geometry to the nearest surface in world space. In contrast, the original face snapping mode uses projection (raycasting) to snap source to target geometry. Face snapping therefore only works with what is visible, while nearest face snapping can snap geometry to occluded parts of the scene. This new mode is critical for retopology work, where some of the target mesh might be occluded.

The nearest face snapping mode has two options: "Snap to Same Target" and "Face Nearest Steps". When the Snap to Same Object option is enabled, the selected source geometry will stay near the target that it is nearest before editing started, which prevents the source geometry from snapping to other targets. The Face Nearest Steps divides the overall transformation for each vertex into n smaller transformations, then applies those n transformations with surface snapping interlacing each step. This steps option handles transformations that cross U-shaped objects better.

The new snapping options allow the artist to better control which target objects (objects to which the edited geometry is snapped) are considered when snapping. In particular, the only option for filtering target objects was a "Project onto Self", which allowed the currently edited mesh to be considered as a target. Now, the artist can choose any combination of the following to be considered as a target: the active object, any edited object that isn't active (see note below), any non-edited object. Additionally, the artist has another snapping option to exclude objects that are not selectable as potential targets.

The Snapping Options dropdown has been lightly reorganized to allow for the additional options.

Included in this patch:

  • Snap target selection is more controllable for artist with additional snapping options.
  • Renamed a few of the snap-related functions to better reflect what they actually do now. For example, applySnapping implies that this handles the snapping, while applyProject implies something entirely different is done there. However, better names would be applySnappingAsGroup and applySnappingIndividual, respectively, where applySnappingIndividual previously only does Face snapping.
  • Added an initial coordinate parameter to snapping functions so that the nearest target before transforming can be determined (for "Snap to Same Object"), and so the transformation can be broken into smaller steps (for "Face Nearest Steps").
  • Separated the BVH Tree getter code from mesh/edit mesh to its own function to reduce code duplication.
  • Added icon for nearest face snapping.
  • The original "Project onto Self" was actually not correct! This option should be called "Project onto Active" instead, but that only matters when editing multiple meshes at the same time. This patch makes this change.

Not included in this patch / future updates:

  • Snapping "Target" is a confusing named, as "Target" is used as both the transformed items (or SCE_SNAP_TARGET_CLOSEST, etc.) and for the objects to which the transformed items are snapped (especially Shrinkwrap modifier). I plan to submit another patch to make this clearer after this is accepted.
  • Many of the functions do not specify in which space the point info (coordinates and normal) is defined.
  • Target selection code could be simplified by separating it from the uber snap_flag variable.
  • The snapping dropdown is feeling very disorganized. Also, since enabling both the Face Projection and the Face Nearest methods does not make sense, perhaps the switch between these methods could be a checkbox (similar to snapping to relative or absolute grid).

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jon Denning (gfxcoder) edited the summary of this revision. (Show Details)Apr 29 2022, 7:19 PM

@Germano Cavalcante (mano-wii) and @Campbell Barton (campbellbarton), I've reimplemented this patch to include far fewer changes, so the review process is much more focused. There is some light refactoring (converting some #define into enum, replacing enum-related char/short/ushort/int with enum, renamed a few functions so their names match what they do, ...), as the changes needed to add this snapping method and options touched these areas anyways.

  • Merge branch 'master' into surface_snap_limited
  • Merge branch 'master' into surface_snap_limited

This seems close to being ready, added some notes inline.

release/scripts/startup/bl_ui/space_view3d.py
6806

Prefer a name that uses col as a prefix col_targetsel for eg.

source/blender/editors/transform/transform_snap.c
430–444

Should this be removed?

451–455

*picky* use full sentences, capitalized with single full-stop.

494–498

A single line explanation for why this would be useful would be good. I assume this would allow many objects to snap onto the surface of an object and rotate aligning to it's surface.

source/blender/editors/transform/transform_snap_object.cc
1252

This comment should be expanded as I'm not sure what to make of it.

Is this something that ever happens in typical usage? If not, perhaps a BLI_assert_unreachable() can be used here so developers will be alerted if it ever happens.

1461

Underline should match length.

3280

Why was this change needed? It seems like changes to ray projection shouldn't be necessary for this patch.

If it's kept, the doc-string for ED_transform_snap_object_project_ray_all needs to be updated.

source/blender/makesdna/DNA_scene_defaults.h
340

*picky* space after comma.

342

Existing files should be version patched to assign this value, currently existing files have this value set to zero.

source/blender/makesdna/DNA_scene_types.h
2096

This bit can be reused as it's cleared in version patching (see !MAIN_VERSION_ATLEAST(bmain, 302, 6) check).

Again, the patch is too big, (scrooling up and down gets tiring). I still recommend to make a separate patch with just the changes:

  • renamed functions (applyProject, applySnapping, activeSnap_with_project)
  • enums (eTfmMode, eSnapMode, eSnapTargetSelect, eSnapFlag)
  • renamed members (tsnap.modeSelect to tsnap.target_select)
  • Changed arguments (-1 to FLT_MAX)
  • Rearrangement (snap_object_data_mesh_get)

Since we are renaming some functions and creating new ones, it's best to follow a more current convention in Blender's code style.
The camelCase style still only exists in the transform code for historical reasons.

The transform code doesn't have a convention yet, but we currently see that:

  • static functions, while not following a convention, tend to have a common prefix (Eg: constraint_, snap_) but never transform_.
  • For other functions, the ones in the header, the prefix is the name of the file (transform_snap_, transform_mode_).
  • For functions that only return a bool and nothing else, the function name is better with _is_, _has_, _use_ or _do_. For example, activeSnap_with_project could be transform_snap_is_active_with_project.

It's not such an important rule, but it would make the review a little easier because, at first glance, I still don't know what activeSnap_SnappingIndividual and activeSnap_SnappingAsGroup do.

Are there any design tasks or discussion links showing the proposed solution? The description could show some images and videos showing the proposal and also explain why this is so useful.
Remembering that there are other snapping options waiting to be added (Grid, Face Center, Face Perpendicular, see T69342 for example). I fear it might start to get crowded.

@Germano Cavalcante (mano-wii), I created D15037 to pull a considerable amount of the refactoring into its own patch. That patch does (should) not change the functionality of snapping, only improving/correcting terms and names. (note: it does have a few notes left in for future work, as that starts to touch the Python API).

I'll address your (and @Campbell Barton (campbellbarton)'s) comments soon.

  • Merge branch 'master' into arcpatch-D14591
  • sync with work on laptop
  • Merge branch 'master' into D14591-transform_snap_nearest
  • use face raycast initially with face nearest as fallback
  • Merge branch 'master' into D14591-transform_snap_nearest
  • addressed reviewer comments, updated versioning (untested)
  • improved comments
  • minor variable name change
  • added comment
  • explicit tests against 0 rather than implicit bool coversion
  • replaced static_cast with enum operator

rebased on recent refactor in D15037 and addressed reviewer comments.

Few questions:

  • Not sure if I need to modify release/datafiles/startup.blend now that versioning is updated (modified startup.blend to set nearest face step to 1 rather than 0, but that should be done through versioning now). should I revert the change to startup.blend?
  • Can I simply rename attribute ToolSettings.snap_target to snap_source in DNA_scene_types.h, or is changing DNA like that more involved?
  • Should snap to active be set by default? (set in versioning)

Todo:

  • Move API changes to bpy.ops.transform.translate to separate patch
    • If snap=True is passed when invoking bpy.ops.transform.translate, snapping is enabled beyond the invoke (all other snapping settings are set only temporarily while transforming). I'm not sure why.
    • expose other snap options through transform ops

thanks!

release/scripts/startup/bl_ui/space_view3d.py
6806

done

source/blender/editors/transform/transform_snap.c
430–444

done

451–455

done

494–498

reworked

source/blender/editors/transform/transform_snap_object.cc
1252

I'm not sure that this should not happen, so I just left it as a return. updated comment

1461

done

3280

reverted. will apply as separate patch

source/blender/makesdna/DNA_scene_defaults.h
340

done

342

thanks! i was trying to figure out how to set this without modifying startup.blend (see general comments)

source/blender/makesdna/DNA_scene_types.h
1505–1507

snap_flag_node, snap_flag_seq, and snap_uv_flag need their types changed from char to short due to pointer magic happening in saveTransform in transform.c (not code that I wrote initially).

snap_node_mode and snap_uv_mode can remain as char since they use only a limited subset of eSnapMode.

2096

done

Jon Denning (gfxcoder) edited the summary of this revision. (Show Details)Jun 7 2022, 6:31 PM
  • minor change to capitalization of label
  • revert some transform_ops.c
  • revert transform API changes (moved to another patch)

ok. i think this is ready to review now.

Rebase on master, also use versioning check which only runs once.

  • make format (wrap lines)

Cleanup: superficial changes

  • Remove
// TODO: rename `snap` to `use_snap`?

...for operator snap, these conventions aren't followed as strictly for operator booleans.

  • C-style comments.
  • Capitalization.
  • Remove updated startup.blend, versioning code must handle updating the startup.

Remove col.active = tool_settings.use_snap assignment from the snap UI.

It's common to keep snap disabled and instead use Ctrl to enable while transforming.
This change greyed out the entire snap-popover contents making it difficult to see values users might want to change before transforming.

Campbell Barton (campbellbarton) requested changes to this revision.Jun 23 2022, 9:10 AM

Testing this patch, it introduces a bug in object mode where an object always snaps to it's self.

This is simple to test.

  • Use the default cube.
  • Change the snap mode to face-project.
  • Move the cursor over the middle of the cube.
  • Press G, Hold Ctrl.
  • Move the cursor around.

The cube is attempting to snap to it's self.

The same issue seems to exist with "Face Nearest" too.

This revision now requires changes to proceed.Jun 23 2022, 9:10 AM
  • fixed bug where object would snap to self
  • Merge branch 'master' into arcpatch-D14591
  • added face nearest snap icons

@Campbell Barton (campbellbarton) that bug is now fixed. thanks!

I did notice another bug, but it also exists in current 3.2.0. i'll dig into it soon.

Campbell Barton (campbellbarton) added inline comments.
release/scripts/startup/bl_ui/space_view3d.py
6808–6833

This can be removed.

This revision is now accepted and ready to land.Jun 29 2022, 10:56 AM
  • merged in master
  • removed old, commented out code
  • merged in master
Jon Denning (gfxcoder) edited the summary of this revision. (Show Details)Jun 30 2022, 2:30 AM
  • copied face nearest icon over master icons