Details

Reviewers

Julian Eisel (Severin)
Sybren A. Stüvel (sybren)

Maniphest Tasks

T92111: Respect parent delta transforms of dragged in objects

Summary

When dropping asset objects, place them under the mouse-cursor
along with any other objects they link in.

Note that there are quite a few changes/refactor that would be worth doing to make this work better, initially I looked into passing ID's from link/append to the caller so they could be manipulated - but this ended up having to access window->eventstate.x/y to position the object (which isn't so elegant). See P2519.

This patch aims to support this functionality in the least disruptive way (to simplify review and to see if the approach is worth pursuing further).

Further Work

Use two separate drop handlers instead of using OBJECT_OT_add_named only to place existing objects. One drop handler would deal with ID's (the outliner) the other would deal with assets. This way OBJECT_OT_add_named need not have the only_place option added.

OBJECT_OT_add_named "duplicate" option can be removed.

Notes

Operating on the resulting selection after appending has the potential downside that newly created data can't be selected or edited (is hidden for example). Committed rB3a2b7f35f469: LibLink: ensure objects are selectable when "Select" is enabled so all newly appended objects are visible & selectable, nevertheless, this is a weakness in this patch and we might consider supporting returning multiple ID's so they can be handled reliably.

T92111 notes that the repositioning should be done in IDTypeInfo.make_local().

I'm not sure how this would work as positioning objects may be assigning the position of multiple root-level parent objects, where only the parents should be moved. Which object is/isn't a root depends on knowing which objects are being transformed - which seems to high level for IDTypeInfo.make_local().

Properly knowing the object location may depend on evaluating the depsgraph (the object could be animated - or use constraints). This is currently done in this patch so the final (appended) location of the object is known before that is used as a reference to move it to it's new location.

We could consider this unimportant and use lower level object placement (e.g. see P2519 which assumed Object.obmat are up to date). But I think it's better to avoid any discrepancy between the position of the object in it's source file - and it's position once linked/appended.

Snap logic is used to translate the objects, to avoid duplicating code (committed separately rB2bcf93bbbeb9: View3D: expose snap selection as a utility funciton).

Diff Detail

Repository

rB Blender

Branch

TEMP-ASSET-LINK (branched from master)

Build Status

Buildable 18246
Build 18246: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.Oct 20 2021, 11:34 AM

Campbell Barton (campbellbarton) created this revision.

Harbormaster completed remote builds in B18030: Diff 43619.Oct 20 2021, 11:34 AM

Campbell Barton (campbellbarton) added a task: T92111: Respect parent delta transforms of dragged in objects.Oct 20 2021, 11:41 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Oct 20 2021, 12:01 PM

Campbell Barton (campbellbarton) added reviewers: Julian Eisel (Severin), Sybren A. Stüvel (sybren).

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Oct 20 2021, 12:03 PM

Regarding using IDTypeInfo.make_local(), https://developer.blender.org/T92111 is... extremely shallow in its description.

My suggestion was that link/append code could get and extra parameter representing the target transform (as a matrix e.g., so that in future you can also tweak rotation and scale of appended data). Could also be used for collections in a different way, etc.

This suggestion has however some very difficult aspects regarding dependencies between objects (parenting, constraints, etc.), which would require a lot of work to implement reliably at such low-level code.

I find your solution (using high-level ED code like snapping selected objects) much better for the time being at least, but this indeed needs to be handled by the operator, not the link/append code.

Campbell Barton (campbellbarton) added projects: Asset Browser, BF Blender.Oct 20 2021, 12:27 PM

Default only_place to off, don't save this setting.

Harbormaster completed remote builds in B18033: Diff 43624.Oct 20 2021, 12:38 PM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Oct 20 2021, 12:45 PM

Don't set duplicate (no longer needed).

Harbormaster completed remote builds in B18034: Diff 43625.Oct 20 2021, 12:47 PM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Oct 21 2021, 8:05 AM

Correct return value from wm_drag_asset_id_import

Harbormaster completed remote builds in B18069: Diff 43679.Oct 21 2021, 8:09 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Oct 21 2021, 9:21 AM

Use a separate drop handler for object assets (instead of having placement-only options for OBJECT_OT_add_named).
Remove OBJECT_OT_add_named "duplicate" option.

Harbormaster completed remote builds in B18070: Diff 43680.Oct 21 2021, 9:23 AM

Rebase on master.

Harbormaster completed remote builds in B18071: Diff 43681.Oct 21 2021, 9:28 AM

Smaller changes requested, or at least suggested ;)

source/blender/editors/object/object_add.c
3618	This mention clearly this transforms the entire selection, so that delta transforms with parents, boolean-objects, etc. are preserved.
source/blender/editors/space_view3d/space_view3d.c
561–568	The distinction here isn't if something is an asset or not, but if something is an external asset that has to be imported first, or a local ID, which may well be marked as asset too. In hindsight, I find `WM_DRAG_ASSET` to be named badly, and reusing `WM_DRAG_ID` for local ID asset makes things even more confusing. Instead there should be `WM_DRAG_ASSET_EXTERNAL` and `WM_DRAG_ASSET_LOCAL_ID` or similar, but that's a separate cleanup.
source/blender/windowmanager/intern/wm_dragdrop.c
476–482	Could we keep this object specific logic in `view3d_ob_drop_copy_asset()`? `wm_drag_asset_id_import()` could become a public function that we can pass `FILE_AUTOSELECT` to.
519	Would add a comment explaining why we deal with selection here.

This revision now requires changes to proceed.Oct 21 2021, 1:25 PM

Andy Cuccaro (AndyCuccaro) added a subscriber: Andy Cuccaro (AndyCuccaro).Oct 21 2021, 2:19 PM

Update based on review

Harbormaster completed remote builds in B18112: Diff 43759.Oct 22 2021, 11:54 AM

Campbell Barton (campbellbarton) marked 4 inline comments as done.Oct 22 2021, 11:56 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/windowmanager/intern/wm_dragdrop.c
519	Comment added to: `view3d_ob_drop_copy_non_external_asset`

In essence this looks good. Still needs some fine tuning to avoid possible confusion I think. The current WM_DRAG_ASSET/WM_DRAG_ID design is already confusing, but I want to address that too (separately).

source/blender/editors/object/object_add.c
3620	What's that hyphen doing there, looks wrong? :) I'd also mention child-parent and boolean modifier relationships as examples, so people have a concrete idea what this is talking about.
3623	`s/do/does`?
source/blender/editors/space_view3d/space_view3d.c
568	Would suggest `view3d_ob_drop_poll_local_id()`, since not every ID is an asset. This will also be the version executed when dragging from the Outliner, which of course isn't asset specific at all. Just note in a comment that this local ID may also be an asset.
570	Together with the suggested naming change, I'd change this to check `drag->type != WM_DRAG_ID`.
692–694	`view3d_ob_drop_copy_local_id()`?
source/blender/windowmanager/intern/wm_dragdrop.c
516	No strong opinion, but what I suggested was making `wm_drag_asset_id_import()` public and call that directly from `view3d_ob_drop_copy_external_asset()`. I find that nicer than an `_ex()` function.

Campbell Barton (campbellbarton) marked 4 inline comments as done.Oct 25 2021, 3:53 AM

Updated based on review

Harbormaster completed remote builds in B18177: Diff 43869.Oct 25 2021, 3:53 AM

Campbell Barton (campbellbarton) added inline comments.Oct 25 2021, 4:01 AM

source/blender/editors/space_view3d/space_view3d.c
568	Renamed to `local_id` to avoid too many iterations on this patch, I'm concerned this would be confused with local-id / linked-id terminology which is used throughout the code-base. While `non_external_asset` reads awkwardly, at least it's clear whats happening.

Remove non-existant function from header

Harbormaster completed remote builds in B18178: Diff 43870.Oct 25 2021, 4:03 AM

Looks good!
There's just the issue that D12912: Assets: Snapping with visual feedback while dragging also touches the object dropping operator, so we have to consider these changes here as well. We can check details in the chat.

source/blender/editors/object/object_add.c
3631	With D12912: Assets: Snapping with visual feedback while dragging, the name `transform_to_mouse` doesn't make much sense anymore, since it would transform to a given matrix. Maybe rename it to `drop_transform`?
source/blender/editors/space_view3d/space_view3d.c
568	While `non_external_asset` reads awkwardly, at least it's clear whats happening. I'd argue against that. `non_external_asset` to me means an asset that isn't external, so local. However this is also used for IDs that are not marked as assets. But anyway, I think the way it is now it's clear.

This revision is now accepted and ready to land.Oct 25 2021, 11:58 AM

Rebase on master, use snap matrix

Harbormaster completed remote builds in B18246: Diff 43992.Oct 26 2021, 7:04 PM

Campbell Barton (campbellbarton) mentioned this in rBa062d86230a0: Drop object assets and associated objects at the cursor location.Oct 26 2021, 7:20 PM

Committed rBa062d86230a07caf022b105f395a6b47dc604040, closing.

Drop object assets and associated objects at the cursor location
ClosedPublic
Actions

Details

Further Work

Notes

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 43992

source/blender/editors/include/ED_object.h

source/blender/editors/object/object_add.c

source/blender/editors/object/object_intern.h

source/blender/editors/object/object_ops.c

source/blender/editors/object/object_utils.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/intern/wm_dragdrop.c

Drop object assets and associated objects at the cursor locationClosedPublicActions

Details

Further Work

Notes

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 43992

source/blender/editors/include/ED_object.h

source/blender/editors/object/object_add.c

source/blender/editors/object/object_intern.h

source/blender/editors/object/object_ops.c

source/blender/editors/object/object_utils.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/intern/wm_dragdrop.c

Drop object assets and associated objects at the cursor location
ClosedPublic
Actions

Revision Contents
Changeset List