Details

Reviewers

Julian Eisel (Severin)
Campbell Barton (campbellbarton)

Maniphest Tasks

T90198: Object Snapping on Drag & Drop

Commits

rBa84f1c02d251: Assets: Snapping with visual feedback while dragging

Summary

Draws a box over the snap cursor with placement and snaps the object
being dragged in 3DView.

Most of this commit reuses what was already done in D12823. Regarding:

The object bounding box and world matrix are written to the asset meatadata on file save.
- This is done through a PreSaveFn callback set for object assets via a new AssetTypeInfo struct. This struct may become more important once we support non-ID assets in future.
The object preview image is disabled while dragging objects over the 3D view.
Change the "Add Named Object" drag into string to "Add Object". The "Named" part is kinda useless info and relates to an implementation detail.

The difference in this patch is in the way the placement cursor is drawn:

Activate the cursor in the dropbox pool function

NOTE: The cursor uses the placement tool parameters if the tool is enabled.

1. TODO ##
Allow snapping to both positive and negative normal direction. Currently a ground plane with -Z up will place the object the wrong way around, for example.
Don't use this for object types with no real geometry, e.g. cameras and lamps.

Diff Detail

Repository

rB Blender

Branch

arcpatch-D12912_1 (branched from master)

Build Status

Buildable 18100
Build 18100: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Julian Eisel (Severin) added a task: T90198: Object Snapping on Drag & Drop.Oct 19 2021, 7:07 PM

Rebase on master
Improve drawing

Harbormaster completed remote builds in B18038: Diff 43632.Oct 20 2021, 2:46 PM

Looking pretty good!

There are issues when the origin isn't at the center of the bounding box. E.g. the origin of the cube in this file is moved away, dragging and dropping the object gives unexpected results:

asset_snap_drag_moved_origin.blend834 KBDownload

While dragging:

And after dropping:

The origin shouldn't matter for the bounding box snapping/dropping.

Julian Eisel (Severin) requested changes to this revision.Oct 20 2021, 8:06 PM

This revision now requires changes to proceed.Oct 20 2021, 8:06 PM

Germano Cavalcante (mano-wii) mentioned this in D12948: Drag Drop: allow customizable drawing.Oct 20 2021, 10:52 PM

Merge changes in D12948
Fix orientation issues

Harbormaster completed remote builds in B18099: Diff 43741.Oct 21 2021, 10:56 PM

Use GPU_BLEND_ALPHA

Harbormaster completed remote builds in B18100: Diff 43744.Oct 21 2021, 11:44 PM

There is a problem changing the operator properties in the redo panel after navigating the viewport.
A solution would be to add the parameter "matrix" in the operator's properties (and remove those x and y).

Add void *draw_data member to wmDropBox
Add parameter "matrix" to the OBJECT_OT_add_named operator
Remove parameters "drop_x" and "drop_y"

Harbormaster completed remote builds in B18131: Diff 43783.Oct 22 2021, 4:12 PM

Fix offset of the item's name when it has an image

Harbormaster completed remote builds in B18137: Diff 43790.Oct 22 2021, 5:18 PM

Fix scale applied twice

Harbormaster completed remote builds in B18142: Diff 43798.Oct 22 2021, 8:49 PM

There's a change in behavior. The drawing disappears when the cursor is between two regions.

Fix icon disappearing

Harbormaster completed remote builds in B18144: Diff 43800.Oct 22 2021, 9:49 PM

Fix drop of objects without boundbox (Camera, lamps)

It was calculating the matrix based on the dimensions of the last used object.
The box is not drawn in these cases, but grid drawing and snapping is still supported.

Harbormaster completed remote builds in B18145: Diff 43801.Oct 22 2021, 10:04 PM

Do not change order of drawings
Align objects without BoundBox with the snap plane.
Fix Crash on drag collections (region was NULL in some cases)
Silence Warning

Harbormaster completed remote builds in B18152: Diff 43817.Oct 23 2021, 5:53 AM

This seems very close to done. The issues I mentioned earlier are fixed.

A few smaller things:

Think we should disable this if the object will be linked, since the location it will get will be unset (as in, set to the transform of the object in the source file). This can be done by checking if wmDragAsset.import_type is FILE_ASSET_IMPORT_LINK.
Maybe boundbox_hint should be called just boundbox? I wanted to emphasize that this is just a hint, not proper object data that you can rely on being available. Since there is no description for the property, it seems good to make that clear.
Since this is the axis-aligned bounding box, we could just store the dimensions directly. That brings added memory per object asset down form 96 to 12 Bytes. I don't see a reason to store the full bounding-box data.
Maybe ED_assets_pre_save() should be turned into a regular pre-save callback. Don't mind really, you can keep it as-is for now.

source/blender/editors/interface/interface_handlers.c
11704–11706 ↗	(On Diff #43817)	Looks like a general fix? When can this happen, I guess while dragging over area borders?
source/blender/editors/space_view3d/space_view3d.c
708–737	This should be moved into it's own function that gives this operation a name. Could also be done in the operator itself, then we don't have to add the `matrix` operator property, but I think this here is the better place.
source/blender/windowmanager/WM_types.h
1071–1073	Document what these are. E.g. that they are executed when a dropbox is activate/deactivated or while it is active, and how this relates to entering/leaving an editor in practice.

This revision now requires changes to proceed.Oct 23 2021, 6:05 PM

Remove unrelated change (region == NULL)
Move all the logic of obmat_final to the invoke function of the operator
Store only "dimensions" in the asset (in place of "boundbox_hint" and "matrix_basis")
Add description for new wmDropBox callback members

I'm not sure I understand the points:

we should disable this if the object will be linked...

ED_assets_pre_save() should be turned into a regular pre-save callback.

Harbormaster completed remote builds in B18154: Diff 43833.Oct 24 2021, 12:06 AM

Fix warnings and compiler error on Unix Makefiles

Harbormaster completed remote builds in B18158: Diff 43837.Oct 24 2021, 2:31 AM

Issue: Dropping lamps, cameras, etc. with this isn't working well. I think this should just not use the snapping, and use the old ED_view3d_cursor3d_position() behavior.

Clarification on my earlier comment:

Think we should disable this if the object will be linked, since the location it will get will be unset (as in, set to the transform of the object in the source file). This can be done by checking if wmDragAsset.import_type is FILE_ASSET_IMPORT_LINK.

When you select an asset library that is not the Current File one, you can choose to link objects, rather than append them:

This is the regular ID data-block linking, so you can't modify any data of the object, because it will be reset when reloading the file. This includes the object's location.
So when wmDragAsset.import_type is FILE_ASSET_IMPORT_LINK, we should disable the bounding-box snapping entirely. It shouldn't even touch the object location then, but that is already an issue in master that I can fix separately.

Maybe ED_assets_pre_save() should be turned into a regular pre-save callback. Don't mind really, you can keep it as-is for now.

I meant to type it could be turned into a regular pre-save callback, not should. So yeah, no need to change anything, just saying we could do this.

Julian Eisel (Severin) requested changes to this revision.Oct 25 2021, 11:02 AM

This revision now requires changes to proceed.Oct 25 2021, 11:02 AM

Julian Eisel (Severin) added inline comments.Oct 25 2021, 11:28 AM

source/blender/blenkernel/intern/object.c
1203	Can use `ARRAY_SIZE()`.
source/blender/editors/include/ED_asset.h
19–21	This change should be reverted.
source/blender/editors/include/UI_tree_view.hh
253	There will be a conflict when merging master here, I removed two arguments.
source/blender/editors/object/object_add.c
3485–3525	We should either do all bounding-box snapping stuff in the operator, or in the dropbox callbacks I think. Now it's split across both places. I think where it was before was fine, I just asked you to move it into a function. The operator can use the `matrix` if set, otherwise, use the `drop_x`/`drop_y` properties.
3554–3560	If the matrix is not set it should fallback to the old method.
source/blender/editors/space_view3d/space_view3d.c
519–538	This can be removed now I think?

Julian Eisel (Severin) mentioned this in D12935: Drop object assets and associated objects at the cursor location.Oct 25 2021, 11:58 AM

Rebase on master (resolve conflicts)
Use ARRAY_SIZE
Rever change in ED_asset.h
Remove unused function

Harbormaster completed remote builds in B18186: Diff 43887.Oct 25 2021, 1:49 PM

Fix compiler error

Harbormaster completed remote builds in B18187: Diff 43888.Oct 25 2021, 1:50 PM

Bring drop_x and drop_y back

Harbormaster completed remote builds in B18188: Diff 43889.Oct 25 2021, 2:40 PM

Some small points only. Sorry for only bringing them up now, didn't pay much attention to these parts of the patch until now. No need to re-review once these are addressed.

source/blender/windowmanager/WM_types.h
1043	Think this should be named `active_dropbox`. And I'd update the comment to say that this is stored to detect when the active dropbox changes (and thus state change callbacks need to be called).
source/blender/windowmanager/intern/wm_dragdrop.c
706–707	We use `_fn` for callbacks only, it's weird to see a regular function call to this. So I'd suggest having a private `wm_drag_draw_item_name()` and a public wrapper for it `WM_drag_draw_item_name_fn()`.
711–718	This isn't used as callback at all, so shouldn't use the `_fn` suffix.
752–755	Same here as above, the `WM_drag_draw_default_fn()` call below seems weird. So suggest `wm_drag_draw_default()` and a public `WM_drag_draw_default_fn()` wrapper.

This revision is now accepted and ready to land.Oct 25 2021, 3:37 PM

include mentioned points

Harbormaster completed remote builds in B18198: Diff 43899.Oct 25 2021, 4:10 PM

Germano Cavalcante (mano-wii) marked 3 inline comments as done.Oct 25 2021, 4:16 PM

Germano Cavalcante (mano-wii) added inline comments.

source/blender/editors/object/object_add.c
3485–3525	Now it's split across both places. Is not true. This drop matrix is only calculated here. (I accidentally left a function unused in the other file but it's already removed). I can add the parameters `drop_x`/`drop_y` along with the matrix if preferable.
3554–3560	I don't think a fallback is applicable here. Originally drop_x and drop_y were only added in the invoke function (optionally the user could set these values). When not added, the `object_add_drop_xy_get` returned false and also skipped the matrix calculation. This happens because we don't have the cursor position in the exec function.

Ops ignore previous comment (It is outdated)!

We still need to disable this for linking, but I can do that part myself once this is in master.

Closed by commit rBa84f1c02d251: Assets: Snapping with visual feedback while dragging (authored by Germano Cavalcante (mano-wii)). · Explain WhyOct 25 2021, 4:57 PM

This revision was automatically updated to reflect the committed changes.

Germano Cavalcante (mano-wii) added a commit: rBa84f1c02d251: Assets: Snapping with visual feedback while dragging.

Why should this not work for linking? Collection instances?

Paul Larson (GeorgiaPacific) added a subscriber: Paul Larson (GeorgiaPacific).Oct 25 2021, 9:50 PM

Note about the issues below. Following comment is on patch-review procedure level.

Next time you change something as critical and core-related as IDTypeInfo, please tag Core and add me or another core module owner as reviewer.

That such a patch could be accepted and landed without Core approval is not acceptable.

CC @Dalai Felinto (dfelinto) .

source/blender/blenkernel/intern/object.c
1272	I) Please add data members before any callbacks functions. This might currently only have a callback, but conceptually this is an `IDTypeInfo` data extension, not a callback function. II) Always add explicit initialization of any new member in `IDTypeInfo` to all IDTypeInfo structures. III) Such changes to core functionality should be isolated in their own commit, and separated from much higher-level changes, usually.

Assets: Snapping with visual feedback while dragging
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 43744

source/blender/blenkernel/BKE_asset.h

source/blender/blenkernel/BKE_idtype.h

source/blender/blenkernel/BKE_main.h

source/blender/blenkernel/intern/asset.cc

source/blender/blenkernel/intern/object.c

source/blender/blenloader/intern/readfile.c

source/blender/editors/asset/ED_asset_mark_clear.h

source/blender/editors/asset/intern/asset_mark_clear.cc

source/blender/editors/include/ED_asset.h

source/blender/editors/include/ED_object.h

source/blender/editors/include/ED_view3d.h

source/blender/editors/include/UI_interface.h

source/blender/editors/include/UI_tree_view.hh

source/blender/editors/interface/interface.c

source/blender/editors/interface/interface_dropboxes.cc

source/blender/editors/interface/interface_template_asset_view.cc

source/blender/editors/interface/tree_view.cc

source/blender/editors/object/object_add.c

source/blender/editors/object/object_relations.c

source/blender/editors/space_file/asset_catalog_tree_view.cc

source/blender/editors/space_file/file_draw.c

source/blender/editors/space_outliner/outliner_dragdrop.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/editors/space_view3d/view3d_cursor_snap.c

source/blender/gpu/GPU_immediate_util.h

source/blender/gpu/intern/gpu_immediate_util.c

source/blender/makesdna/DNA_asset_types.h

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/WM_types.h

source/blender/windowmanager/intern/wm_dragdrop.c

source/blender/windowmanager/intern/wm_draw.c

source/blender/windowmanager/intern/wm_files.c

source/blender/windowmanager/wm_event_system.h

Assets: Snapping with visual feedback while draggingClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 43744

source/blender/blenkernel/BKE_asset.h

source/blender/blenkernel/BKE_idtype.h

source/blender/blenkernel/BKE_main.h

source/blender/blenkernel/intern/asset.cc

source/blender/blenkernel/intern/object.c

source/blender/blenloader/intern/readfile.c

source/blender/editors/asset/ED_asset_mark_clear.h

source/blender/editors/asset/intern/asset_mark_clear.cc

source/blender/editors/include/ED_asset.h

source/blender/editors/include/ED_object.h

source/blender/editors/include/ED_view3d.h

source/blender/editors/include/UI_interface.h

source/blender/editors/include/UI_tree_view.hh

source/blender/editors/interface/interface.c

source/blender/editors/interface/interface_dropboxes.cc

source/blender/editors/interface/interface_template_asset_view.cc

source/blender/editors/interface/tree_view.cc

source/blender/editors/object/object_add.c

source/blender/editors/object/object_relations.c

source/blender/editors/space_file/asset_catalog_tree_view.cc

source/blender/editors/space_file/file_draw.c

source/blender/editors/space_outliner/outliner_dragdrop.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/editors/space_view3d/view3d_cursor_snap.c

source/blender/gpu/GPU_immediate_util.h

source/blender/gpu/intern/gpu_immediate_util.c

source/blender/makesdna/DNA_asset_types.h

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/WM_types.h

source/blender/windowmanager/intern/wm_dragdrop.c

source/blender/windowmanager/intern/wm_draw.c

source/blender/windowmanager/intern/wm_files.c

source/blender/windowmanager/wm_event_system.h

Assets: Snapping with visual feedback while dragging
ClosedPublic
Actions

Revision Contents
Changeset List