o3de icon indicating copy to clipboard operation
o3de copied to clipboard

Fixes and reworks the way drag and drop functions (when dragging FROM asset browser)

Open nick-l-o3de opened this issue 1 year ago • 3 comments

What does this PR do?

A slight rework to the way drag and drop functions:

Drag and drop from the Assset Browser now works when dropped on the Outliner (It creates entities that have the highlighted entity as a parent).

Drag an drop is now consistent when dragging into the 3d viewport compared to when dragging to the outliner (becuase its the same functions).

Dragging to the viewport / outliner. If you selected a "container" asset that has lots of products (like FBX) it will use a heuristic (which gems can hint priorities to) to select which component to attach to the new entity. it will attempt to pick the component that most represents the asset, preferring exact name matches - and failing that, alphabetically. If you selected a specific product asset, it will spawn an entity with that asset in a component.

If gems want to highly customize the behavior, they can either supply a priority for their kind of asset type in the above heuristic, or, they can supply their own drag and drop handler to take over and do something special. For example, special case handling was added for procprefabs and prefabs by adding a higher priority handler for drag and drop. This handler: If you drag a container (like FBX) which produces procprefabs, will automatically instantiate the procprefab (re-creating the heirarchy) and detaching it.
If you drag a prefab it will instantiate the prefab If you drag a procprefab directly, it will instantiate the procprefab.

If you drag to the entity inspector, it will still act as before. This lets you (for example) script canvases directly into the inspector to add it to an existing entity, or drag it into the viewport to make a new entity.

Drag and drop bugs fixed, including duplicate drops.

Please add links to any issues, RFCs or other items that are relevant to this PR. Addresses #9885 Addresses #10627

How was this PR tested?

Automated tests are included in this PR. Manual testing (dragging various things) was also done.

Note that as part of this change, the objectstream is no longer used to serialize/deserialize asset entries.

I also found unused functionality (removed) such as IsGemFolder. There was also functionality in the AssetHandler to specify other asset types to be incompatible - but tracing the prior logic it turns out this call was not actually ever being handled correctly. (and was only used by Atom) The handler was removed.

technical breakdown of what changed

  • Objectstream no longer used to serialize/deserialize dragged asset browser entries
  • Const-cast removed from mimedata functions involving asset browser entries
  • Mimedata manipulation functions for asset browser entries moved to a utils class
  • Automated tests added to the utils class.
  • Removed the "Source drop bus" as it was only used by 1 listener and was insufficient for expressing the functionality needed - for example, it was unable to accept the drop and prevent duplicate drops by other listeners
  • Replaced the PrefabSaveLoadHandlers use of Source Drop Bus with a higher-than-normal priority drop handler that actually responds correctly to prefabs being dropped.
  • Added a new kind of drag and drop bus and context to handle item-view drags, which have different API than window drags.
  • Made it so that the entity outliner used that new bus
  • Removed duplicate code from the entity outliner. It was duplicating AzAssetBrowserEventHandler functionality
  • AzAssetBrowserEventHandler cleanup and unification of all kinds of drops - for list views as well as widget views.
  • AzAssetBrowserEventHandler no longer tries to do anything to source files, this is for other handlers to intercept.
  • AzAssetBrowerEventHandler now tries to find the "best" asset when a source is dropped.
  • Added a overridable API to the Asset Type Info Bus that lets gems say that their asset is a better match than another kind of asset (using a prioritysystem)
  • Used this new bus API to make it so that actors are "better" to spawn a component for than meshes (so if you drop a FBX with an actor in it, you'll likely get the Actor Component instead of mesh component).
  • Added tests for AzAssetBrowserEventHandler.

Signed-off-by: lawsonamzn [email protected]

nick-l-o3de avatar Jul 28 '22 21:07 nick-l-o3de

I took a look at this again and one thing I need to change is basically I need to make the Asset Browser Entries consistent. They're a tree of entries, each with pointer to parent, and pointers to children, with polymorphic base AssetBrowserEntry*.

An example heirarchy that represents "D:/MyProject/Textures/icon.tif" from the source files, which outputs @products@/textures/icon.textureasset" into the cache:

RootAssetBrowserEntry   
     FolderAssetBrowserEntry    D:\  
         FolderAssetBrowserEntry      MyProject    <-- watched scan folder
            FolderAssetBrowserEntry      Textures
                  SourceAssetBrowserEntry        icon.tif
                        ProductAssetBrowserEntry     (cache)/textures/icon.textureasset

In the code at the moment, each of these browser entries have the following fields "full path" "name" "relative path" "display name" -- shown in the UI in tree and list view mode "display path" -- shown in the UI in list view mode

The problem is that "Full path", "name", and "relative path" are all inconsistent between the various types.

What I expect: Full path - the full absolute path to the thing. The textures folder should have "D:\MyProject\Textures". Products should be the full path including the cache folder. You should be able to 'fopen' the full path. Relative path - depends on context - for it to be useful , for folders and sources it should probably be the relative path to the scan folder. For products it should be the relative path to the cache. This should essentially be the name for it that you'd use to talk to the asset system APIs. So for the source file that should be "Textures/icon.tif" and for the product it should be "textures/icon.textureasset"

Name - the file name only.

But the actual case is that depending on where its situated in the tree and what type it is, it has partial data for the above, or the wrong data. For example, products have their parent's full path as their own full path (so the texture pretends its the source), and the relative path is blank.

As part of cleaning this up I'll go ahead and fix this so that they are all 100% consistent. Then code that works with these like was brought up in comments above doesn't have to go thru the catalog system to look up things like name or relative path or full path. They can just ask the node they're handed. It looks like theres a bunch of places in the code which could have just asked for the relative or full path for an item they're handed but because relative and full path were completely inconsistent or missing the code goes on a roundabout way to query it from some other system (such as querying it via assetid when the information should be right there).

nick-l-o3de avatar Aug 05 '22 15:08 nick-l-o3de

Changes suggested by PR made. All tests succeed (except known broken Test Impact Framework tests) from ci_build.py locally. In addition, manual testing done. You can drag an ddrop to viewport, to slots, and you can also drag and drop to, say, notepad, vscode, etc.

nick-l-o3de avatar Aug 08 '22 18:08 nick-l-o3de

Note that I have not yet merged from mainline. once reviewers are okay with these changes I intend to rebase on latest dev. However I prefer to avoid doing that until reviewers have seen the changes and can choose the "since last I looked at them" option. (Which breaks if you rebase)

nick-l-o3de avatar Aug 08 '22 18:08 nick-l-o3de

I updated to handle the PR comments I could. Retested manually but also by running the tests. Found some additional cases where GetFullPath is called and they all expect a real path, so I'm now auto-resolving it as part of GetFullPath()

nick-l-o3de avatar Aug 11 '22 19:08 nick-l-o3de

I believe thats a shipit from each commenter. So I'll make one more update - the rebased one. Then try to build.

nick-l-o3de avatar Aug 11 '22 22:08 nick-l-o3de

the rebase had no conflicts with anything.

nick-l-o3de avatar Aug 11 '22 22:08 nick-l-o3de

so now CI/AR is failing due to unrelated errors in tests this does not affect. Will try again on monday

nick-l-o3de avatar Aug 12 '22 20:08 nick-l-o3de