Page MenuHome

Ruler: New Snap Gizmo and its use in the Ruler tool
ClosedPublic

Authored by Germano Cavalcante (mano-wii) on Mar 9 2020, 2:24 PM.
Tokens
"Love" token, awarded by Debuk."Love" token, awarded by gilberto_rodrigues."Love" token, awarded by Okavango."Mountain of Wealth" token, awarded by franMarz."100" token, awarded by Frozen_Death_Knight."Love" token, awarded by ugosantana."Love" token, awarded by wilBr."Love" token, awarded by jc4d."Love" token, awarded by duarteframos."Love" token, awarded by brilliant_ape."Love" token, awarded by amonpaike."Like" token, awarded by Schiette."Like" token, awarded by Leul.

Details

Summary

The main idea of this patch is to create a generic snap gizmo that can be used in different tools.
In this case, the ruler was the chosen tool.

technical information:

  • The Gizmo can be configured initially by the following properties -> "snap_elements_force", "active_on", "prev_point"
  • The following properties can be read as return -> "location", "normal", "snap_elem_index"
  • This property can be linked to another (tool_setting.snap_elements) -> "snap_elements"
  • 3 extra utilities have been created -> ED_gizmotypes_snap_3d_draw_util, ED_gizmotypes_snap_3d_context_get, ED_gizmotypes_snap_3d_update.

Diff Detail

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Sergey Sharybin (sergey) requested changes to this revision.Apr 15 2020, 11:48 AM
Sergey Sharybin (sergey) added inline comments.
source/blender/editors/gizmo_library/gizmo_types/snap3d_gizmo.c
95–96

Call it less generic than imat. view_inv would be way more clear.

98

Is there a more concrete name than size? What is this size of?

119

Pixel size = (magic constants) * ED_view3d_pixel_size()

Would better be scaled_pixel_size. Or something like this.

286–295

Intuitively draw() would use the input as const and just present it on the screen. But here there are some assignments happen.

Surely, there is a reason for this, but totally worth adding it as a comment.

330–331

Is there a way to avoid this one frame delay?

source/blender/editors/space_view3d/view3d_gizmo_ruler.c
945–947

Was this intentionally inverted compared to the "old" code?

This revision now requires changes to proceed.Apr 15 2020, 11:48 AM

Testing,

it would be good if this patch snapped before dragging on the ruler, this was one of the main advantages of using the gizmo.

Although if this is a bigger task, it could be separated into another patch, since this is at least no worse than current functionality.

source/blender/editors/gizmo_library/gizmo_types/snap3d_gizmo.c
331

We should have a way to force to be low priority - last.

This seems like something that should have some UI team involvement? Is there a design task, or at least some mockups/screenshots to see what this looks like or how it works? (Found this by accident)

Germano Cavalcante (mano-wii) marked 7 inline comments as done.
  • rename imat to view_inv
  • rename and describe size variable
  • rename and describe px_size variable
  • comment why we assign values in a drawing callback
  • avoid frame delay
  • comment why we inverted the snap toogle

This seems like something that should have some UI team involvement? Is there a design task, or at least some mockups/screenshots to see what this looks like or how it works? (Found this by accident)

The description has been updated

source/blender/editors/gizmo_library/gizmo_types/snap3d_gizmo.c
330–331

To avoid frame delay I did another solution by assigning an operator for gizmo_snap and testing the WM_GIZMO_STATE_HIGHLIGHT flag.

source/blender/editors/space_view3d/view3d_gizmo_ruler.c
945–947

This was intentional, to maintain the snap after activating the gizmo. Maintain continuity.

  • Document operator "VIEW3D_OT_ruler_add"
Sergey Sharybin (sergey) added inline comments.
source/blender/editors/gizmo_library/gizmo_types/snap3d_gizmo.c
19

2020.

125

Better to give description which will tell what the value denotes.

Germano Cavalcante (mano-wii) marked 2 inline comments as done.
  • Improve comments
  • Simplify drawing of the transform snap points

Getting some compile warnings here:

/src/blender/source/blender/editors/space_view3d/view3d_gizmo_ruler.c: In function ‘ruler_state_set’:
/src/blender/source/blender/editors/space_view3d/view3d_gizmo_ruler.c:274:39: warning: unused parameter ‘C’ [-Wunused-parameter]
  274 | static void ruler_state_set(bContext *C, RulerInfo *ruler_info, int state)
      |                             ~~~~~~~~~~^
/src/blender/source/blender/editors/space_view3d/view3d_gizmo_ruler.c: In function ‘view3d_ruler_item_mousemove’:
/src/blender/source/blender/editors/space_view3d/view3d_gizmo_ruler.c:360:14: warning: variable ‘prev_point’ set but not used [-Wunused-but-set-variable]
  360 |       float *prev_point = NULL;
      |              ^~~~~~~~~~
/src/blender/source/blender/editors/space_view3d/view3d_gizmo_ruler.c: In function ‘gizmo_ruler_invoke’:
/src/blender/source/blender/editors/space_view3d/view3d_gizmo_ruler.c:986:23: warning: comparison is always true due to limited range of data type [-Wtype-limits]
  986 |   if (inter->co_index != -1) {
      |                       ^~
  • Don't display tooltips for this gizmo
  • Cleanup: Remove unused property

Great task! Just throwing couple of ideas in, if i understood the scope of this task correctly...

Perhaps @Germano Cavalcante (mano-wii) you could consider giving the option to the user to spread the crosshaires of the cursor a little bit so the gap between them would be more prominent. The design idea of this gap in crosshaires in general was to make target selection more efficient - user's view of the target is not obstructed by the middle crossing of the crosshair lines.

Do you mean changing the cursor design?
While it is interesting, this is not something that an operator can edit. This type of design is in a different area.
I would start to escape the scope of this patch.

Ah, ok. Sorry for the off-toppic.

Campbell Barton (campbellbarton) requested changes to this revision.May 26 2020, 3:36 PM

Generally fine, although I think the default should be not to snap, then snap when holding Ctrl,
currently it's impossible not to snap - for example.

This could follow the scenes snap setting too.

This revision now requires changes to proceed.May 26 2020, 3:36 PM
  • Capture events that match user keymaps to invert scene snap.
  • Make WM_keymap_active take const 'wm' argument
  • Set the previous point property from the ruler
  • define USE_SNAP_DETECT_FROM_KEYMAP_HACK
  • Fix cursor update when releasing ctrl
  • Use WM_GIZMO_NO_TOOLTIP flag.
  • invert_snap: check snap_off keymap
  • Cleanup: minor edits, unused header
  • Cleanup: spelling
  • Use stored property variable instead of string
This revision was not accepted when it landed; it landed in state Needs Review.May 27 2020, 5:17 AM
This revision was automatically updated to reflect the committed changes.