[Editor] Change TransformationGizmo Update to be synchronous and change entity duplication behavior#3186
[Editor] Change TransformationGizmo Update to be synchronous and change entity duplication behavior#3186Basewq wants to merge 1 commit into
Conversation
…tity duplication from gizmo to EditorGameEntityTransformService.
| public override IEnumerable<Type> Dependencies | ||
| { | ||
| get | ||
| { | ||
| yield return typeof(IEditorGameEntitySelectionService); | ||
| yield return typeof(EditorGameModelSelectionService); | ||
| } | ||
| } |
There was a problem hiding this comment.
I know this is not the only place this is done like this but, why keep doing it like this? I mean, if the types are known at compile time (i.e., not dynamically determined), this is equivalent, even better if the JIT can optimize it more in the future:
There was a problem hiding this comment.
I can change this if wanted and people don't mind having some in this way and others in the yield return code.
I think there is a subtle difference if they do include base.Dependencies, ie. [.. base.Dependencies, typeof(A), typeof(B)] since it's making an expanded array though the likelihood of an extremely large array through chaining is highly unlikely in practice.
There was a problem hiding this comment.
Yes, I am fine to use new syntax from now on (for compactness & readability).
We can fix the others in separate PR later.
(but fine to merge as is and do everything later too, it's not a deal breaker)
|
Does it still work with multi-selection of entities that are not under the same sub-tree? |
Yes, it uses existing code that gets the root level entities from the selection(s) then copies all of them. For the time being, I've decided to put this on WIP status, as I am switching to another WIP thing, which may or may not use this (or parts of this). I think the overall idea is still valid: However I feel someone might come up with a cleaner implementation than here. |
|
🤖 Draft PR — automatic CI is skipped to save runner minutes.
|
|
Some parts may return, eg. transform gizmo cancellation event args. |
4 participants
PR Details
This serves as a bit of prep work leading to #3183.
The purpose of this PR is to remove the async part of
TransformationGizmo.Update.The only two places where async is used is:
W/E/Rkeys)Ctrl-> Drag gizmo to duplicate entities (note: an existing quirk is you must focus in the scene beforeCtrlkey down is detected, which is not fixed in this PR).My changes and reasoning:
No longer await UI invoke. This is an example of
game -> editor -> game, which relies on theeditor -> gamereturn to set update the transform gizmo mode (refer to Related Issue section for a bit more context).It now just treats it as a fire-and-forget request to the editor, which will 'eventually' update the mode back to the game thread.
I'm not really sure if there is a cleaner solution, other than either 'predicting' the mode change (ie. update directly as well), or block the gizmo until the editor 'confirms' the request.
Moved the entity duplication logic out of the transform gizmo and into EditorGameEntityTransformService.
This makes the transform gizmo reusable for other things which may or may not have 'duplication' logic (eg. spline tool).
Another change, which might be more debatable, is now the initial duplication is only done on the game side (temp cloned entities), then at the end of the transform the request is sent to the editor.
Doing it this way also means there is only one undo/redo transaction, whereas the previous behavior had two transactions (1st transaction on the drag start to generate entities in place, then a 2nd transaction for the position update), ie. you had to undo twice to actually remove the duplicated objects.
One other quirk is when duplicating a large number of entities (eg. use 3rd Person Platformer template and duplicate the entire 'Platform' folder via
Ctrl+ drag), the original behavior has a large initial freeze since it clones at the start.This change makes the large freeze occur at the end of the transform (along with a progress bar popup if longer than 500ms), however this does introduce a different quirk where the preview entities disappear then the real objects are added afterwards.
(New functionality) There is also now the ability to hit Escape key to cancel the transform when you're in the middle of transforming (no transaction goes the editor).
Related Issue
Prep work for #3183
Regarding further context for change 1:
There is a bit of concern with the current architecture of the 'game services', which may need additional thoughts on.
The issue is the back and forth marshaling between 'WPF UI thread' (ie. where the asset/quantum data live) and the 'game thread' (ie. live scene editor).
There are some 'game services' that contain both the game scene data and the 'view model'/UI thread data (ie. implement
IEditorGameServiceandIEditorGameViewModelServiceas a single class), and can end up doing things like'game thread' -> await InvokeAsync to 'UI thread' -> InvokeAsync to 'game thread', which is exactly what the gizmo code was doing.Regarding further context for change 2:
The future goal is to change classes that inherit
EditorGameMouseServiceBaseto only have sync update methods, so they can ultimately be bundled through some 'edit mode' class and updated in some specified order (and optionally enabled/disabled depending on the mode), rather than the free-for-all approach that's happening now (they will still have the option to register an independent task via ScriptSystem if they need it).Types of changes
Not sure if doc change is required or not,
TranslationGizmo/ScaleGizmo/RotationGizmoare public, however these gizmos are part ofStride.Assets.Presentation, which the general user cannot accidentally stumble into due to the lack of proper plugin integration.Checklist