Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix editor area/actor deselection bugs #21318

Merged

Conversation

drogoganor
Copy link
Contributor

@drogoganor drogoganor commented Jan 27, 2024

There are two state issues with selection and deselection in the editor after the new changes.

  • After dragging out an area selection, right clicking to deselect the area leaves the area selection panel visible.
  • If you drag out an area selection, then click an actor, then click cancel, the area selection panel is still shown.

This PR fixes these issues. ActorEditLogic needed some logic to fire in a handler for DefaultBrush.SelectionChanged instead of Tick().

Also added a missing copyright header.

Also fixes #21314

@drogoganor drogoganor force-pushed the bug/editor-selection-right-click-clear-bug branch from ef4ce40 to f96297e Compare January 27, 2024 05:49
@drogoganor drogoganor changed the title Fix editor area selection panel still visible after right-clicking to deselect Fix editor area/actor deselection bugs Jan 27, 2024
@drogoganor drogoganor force-pushed the bug/editor-selection-right-click-clear-bug branch 2 times, most recently from 270f9e9 to 1f482c3 Compare January 27, 2024 08:28
@Porenutak
Copy link
Contributor

a bit off topic, but it would be intuitive if selected area can be canceled also with Escape key. Same for Copy/paste brush

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo does not seem to remove actor selection visual

OpenRA.Mods.Common/Widgets/Logic/Editor/ActorEditLogic.cs Outdated Show resolved Hide resolved
@drogoganor drogoganor force-pushed the bug/editor-selection-right-click-clear-bug branch from 63d9a57 to e2f7a07 Compare January 30, 2024 02:47
@PunkPun
Copy link
Member

PunkPun commented Jan 30, 2024

the history tab seems a bit bugged. If you go to a select action then you are put into the selection panel, instead of being able to move through the rest on history. I dunno what the best way to fix that is.

And also all subsequent actions are deleted as the "Cleared Selection" action is added (or is going back to history tab inserting the action?)

@drogoganor
Copy link
Contributor Author

drogoganor commented Jan 30, 2024

I'll check out that History tab bug. Sorry Pun I thought it was in better shape.
EDIT: That history tab issue is fixed in the most recent commit now.

There's a couple of other code design issues I still want to address too:

  • The ActorEditLogic has a CurrentActor property. I think this should actually come from the EditorDefaultBrush.Selection.Actor now.
  • Additionally, there's a Selected property on the EditorActorPreview. I'm wondering if this flag should be set to true/false in the EditorDefaultBrush now instead.

@PunkPun
Copy link
Member

PunkPun commented Jan 30, 2024

yeah, probably they should be controlled by the default brush

@drogoganor
Copy link
Contributor Author

I'll continue working on this but just letting you know I'm happy if you want to back out the editor selection PR until it's fully ready to go.

@drogoganor drogoganor force-pushed the bug/editor-selection-right-click-clear-bug branch from 184107b to 1844a35 Compare February 4, 2024 08:19
@drogoganor
Copy link
Contributor Author

I did the intended code refactor items I mentioned in a previous comment:

  • Remove SelectedActorProperty from ActorEditLogic (source from Selection.Actor from EditorDefaultBrush instead)
  • Set the EditorActorPreview.Selected property to true/false in the new EditorDefaultBrush.Selection setter instead.

I also applied your idea to invoke the SelectionChanged event from the EditorDefaultBrush.Selection setter instead, which is tidier and worked as intended.

Two annoyances still remain:

  • Clicking a selection changed event in the History tab changes the tab to the (not shown) selection tab - as you mentioned, we could simply make the selection tab a visible tab instead. We have the room for it now. The tab could be disabled when there's nothing selected.
  • When you delete a selected actor, it inserts two history events: unselect the actor, then delete it. It would be good if these were merged into one.

@drogoganor drogoganor force-pushed the bug/editor-selection-right-click-clear-bug branch 4 times, most recently from 7d11c40 to 153fb1b Compare February 4, 2024 13:34
@drogoganor
Copy link
Contributor Author

drogoganor commented Feb 4, 2024

image
Select tab button has been (re)introduced. It's disabled if there's no current selection.

Added fixes for the last two items mentioned:

  • Clicking selection changed items in the history tab will no longer change to the Select tab. (Though this does produce an annoying issue - if you select an actor/area on the map with the history tab open, it won't go to the Select tab. Could be fixed with a parameter on the SelectionChanged event that determines whether the change was from undo/redo or from user action - however, the event is invoked from within the EditorDefaultBrush.Selection setter, so it will need some extra thought).
  • Deleting a selected actor will now only produce one history item instead of two.

Also reintroduced a fix for issue #21314 as per PunkPun's suggested code change.

I also added a couple of little fixes for the new area selection info panel (I might look at that in more detail later).

I also made the chrome for the editor icon collection use a distinct set instead of reusing the timer and infantry icons as suggested in Discord by Dzierzan:

editor:
    Inherits: ^Glyphs
    Regions:
        tiles: 35, 171, 16, 16
        overlays: 52, 171, 16, 16
        tools: 69, 171, 16, 16
        actors: 100, 171, 16, 16
        history: 130, 171, 16, 16

Having a ResourceLayer defined should be optional again as per michael.dgg.2's suggestion.

ArtSrc changes are here: OpenRA/ArtSrc#20

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like icons became off-centred

Screenshot 2024-02-05 at 12 37 07

when I select a resource, it blinks on the last spot where the add resource cursor was rendered. Same with tiles. I wonder if this is a recent regression.

I wonder how other cursors should work. Now when you change cursors you nolonger delete selection. The actor is still selected, and the area select still exists (you can copy paste), but just not rendered while another cursor is equipped. I feel like changing cursor should delete selection.

Perhaps the select tab should act as a button that sets cursor to the default cursor? That way it would still have a function while there's no selection available and would not need to be disabled

@drogoganor drogoganor force-pushed the bug/editor-selection-right-click-clear-bug branch from e7b438b to 525d3f5 Compare February 6, 2024 07:45
@drogoganor
Copy link
Contributor Author

drogoganor commented Feb 6, 2024

Thanks for the code review again Pun. I agree, changing cursors should clear the selection - I added a ClearSelection(); to the EditorDefaultBrush.Dispose() which does that.

I tested out the "flashing tile" issue on commit b58c1ea (before the selection changes) - it happens there, too. I guess it needs a tick to set the cursor position again.
Reproduction steps:

  • Select a tile brush, and hover it somewhere
  • Right-click to deselect the brush
  • Select an actor brush
  • Select a tile brush again

The select tab being enabled without a selection is a possibility - we could add an instructions label to the empty tab, to the effect of "Left-click to select an actor, or left-click and drag to select an area.". I guess the downside is that the tiles aren't shown as soon as you create a new map (likely the first thing the user will want to use).

Yes, it's a real pain that the icons seem off-center, too. I wonder if they can be offset in the chrome? The problem is that the buttons are 49 pixels wide.
EDIT: I'm an idiot - they have positions that are explicitly set! I will fix that.
EDIT: Button icons are centered again now.
cnc-2024-02-06T090004692Z

The area selection info panel needs a bit of work to fit correctly on the minimum supported resolution (1280x720 I believe), but I should probably leave that for another PR. I think the "Area Selection" header could have the dimensions and coordinate range instead, like "Area Selection: 13,47 - 25,55 (12x8)"
(Screenshot includes centered buttons)
ra-2024-02-06T085932707Z

@drogoganor drogoganor force-pushed the bug/editor-selection-right-click-clear-bug branch from 525d3f5 to a961adf Compare February 6, 2024 08:49
@drogoganor
Copy link
Contributor Author

drogoganor commented Feb 6, 2024

I also made some adjustments to the area selection panel info area so it will fit at 1280x720. The area and dimensions are now shown in the title:

ra-2024-02-06T093439269Z cnc-2024-02-06T093414885Z

I also correctly centered the title label.

@drogoganor drogoganor force-pushed the bug/editor-selection-right-click-clear-bug branch from 14f0866 to fb7355a Compare February 6, 2024 11:16
@drogoganor
Copy link
Contributor Author

drogoganor commented Feb 6, 2024

I've altered the area select behavior now so that the selection is kept when selecting a different brush. I know this is contrary to my earlier comment, but after playing around with it, I think it's better behavior.

Right clicking will not clear the selection. This matches actor selection behavior.
Right clicking an actor or overlay with an area selection will delete the actor or overlay.
Switching to the selection tab with another brush selected will no longer show a blank panel.

Undo/redo actions will no longer switch tabs. This required a change to IEditorAction.Do() to pass a isUndoRedoAction bool (Should this be named "isHistoryAction" instead?)

I'm much happier with how it all fits together now.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you copy-paste with no filters on, the undo action won't undo the removed actors

OpenRA.Mods.Common/EditorBrushes/EditorActorBrush.cs Outdated Show resolved Hide resolved
@drogoganor drogoganor force-pushed the bug/editor-selection-right-click-clear-bug branch from fb7355a to 1463118 Compare February 7, 2024 04:49
@drogoganor
Copy link
Contributor Author

drogoganor commented Feb 7, 2024

I reverted the change to the IEditorAction interface and introduced a separate event EditorDefaultBrush.UpdateSelectedTab, instead.
I also fixed the actor copy/paste not being gated by the filter correctly, ack.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@PunkPun PunkPun merged commit d630a6e into OpenRA:bleed Feb 7, 2024
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Feb 7, 2024

changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Actor names are not translated on the new Editor Selection UI.
3 participants