ActorMoveBrush for Editor - #21255
Conversation
PunkPun
left a comment
PunkPun
left a comment
There was a problem hiding this comment.
Crash
Exception of type `System.NullReferenceException`: Object reference not set to an instance of an object.
at OpenRA.Mods.Common.Widgets.EditorActorMoveBrush.HandleMouseInput(MouseInput mi) in /OpenRA/OpenRA.Mods.Common/EditorBrushes/EditorActorMoveBrush.cs:line 49
at OpenRA.Mods.Common.Widgets.EditorViewportControllerWidget.HandleMouseInput(MouseInput mi) in /OpenRA/OpenRA.Mods.Common/Widgets/EditorViewportControllerWidget.cs:line 94
Generally we do atomic and self-contained commits, that is each commit is a feature or change that doesn't break bleed. We do make exceptions to that rule sometimes when splitting a feature makes reviewing much simpler.
When it comes to this feature I feel like it would be much more intuitive to just allow moving actors while having them selected. I feel a new brush isn't necessary as it really only interacts with actors anyway.
Even if we move actors while they are selected we would still need a new brush that handles the mouse inputs etc. |
|
actor edit brush should be handling the inputs |
|
We don't have a brush for editing actors. |
|
From workflow perspective new brush just for move is bad idea. default brush should handle this just fine. If you really wanna use modifier, not button. |
|
if #20226 were to be merged then drag selection will conflict with actor moving, a lot of tools separate out move cursor specifically for this purpose. But I still believe that actor edit cursor should be capable of moving actors |
c015c36 to
88bd693
Compare
|
|
||
| public void MoveActor(EditorActorPreview preview, CPos location) | ||
| { | ||
| editorLayer.Remove(preview); |
There was a problem hiding this comment.
I think removing is wrong. We should instead add an update function to EditorActorLayer
There was a problem hiding this comment.
I'm giving this a go but it is proving to be a bit difficult. It makes a lot of sense to remove and recreate because lots of items like the footprint, LocationInit, and RuntimeNeighbourInit need to be removed and recreated, too. A dedicated Update function essentially recreates a lot of stuff that the Remove/Add functions are already doing and only really increases future maintenance burden.
However, I am recovering from Covid as I write this and might not be in my right state of mind.
There was a problem hiding this comment.
We also remove and add actors to the world instead of updating them, so this could be fine.
| new Rectangle(Bounds.X, Bounds.Y, Bounds.Width, Bounds.Height), Color.White); | ||
|
|
||
| // TODO: updating all actors on the map is not very efficient. | ||
| onCellEntryChanged = _ => GeneratePreviews(); |
There was a problem hiding this comment.
this should also trigger a visual update. When map height changes so should the actors position. Easiest way to test is placing cliffs and having terrain overlay turned on
There was a problem hiding this comment.
Oh yes, I see - also pasting terrain of a different height with the actor filter turned off
352c05d to
8fc0ad1
Compare
|
That should all be fixed up now - actor previews and selection box will be updated when the terrain changes. |
PunkPun
left a comment
PunkPun
left a comment
There was a problem hiding this comment.
LGTM, just one last bug
When you are dragging around an unselected actor you can still delete it, however if you had never stopped dragging the actor gets recreated. Once you're done dragging, if you move trough action history the map editor will crash
8fc0ad1 to
1ccf805
Compare
|
Cool - I disallowed the right click to delete actor if shift is held. |
|
|
||
| // Delete actor. | ||
| if (underCursor != null && underCursor != Selection.Actor) | ||
| if (underCursor != null && underCursor != Selection.Actor && !mi.Modifiers.HasModifier(Modifiers.Shift)) |
There was a problem hiding this comment.
this doesn't fix the bug. Holding shift is only necessary to start dragging, but once you start it can be released
1ccf805 to
fb36e99
Compare
6 participants
This PR adds a new brush that can move singular actors by drag and drop. It includes Ctrl + M hotkey as well as translation strings. It is expected to wait for #20226 to be merged first and this adjusted to changes, currently my goal is to get feedback whether this is the right approach. Example of tool in action:
Fixes #5241
2023-12-14.11-23-57.mp4