Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#678 closed enhancement (fixed)

[PATCH] [Atlas] Improve unit selection options

Reported by: Kieran P Owned by: historic_bruno
Priority: Should Have Milestone: Alpha 9
Component: Atlas editor Keywords:
Cc: Patch:

Description (last modified by Kieran P)

Copy over unit selection methods from simulation to the editor.

  • Double-clicking selects all similar units
  • Band-boxing selection of units
  • SHIFT+Click adds to already selected group
  • CTRL+Click removes from already selected group

Attachments (1)

atlas_selection-01182012.patch (48.8 KB ) - added by historic_bruno 12 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by Kieran P, 13 years ago

Description: modified (diff)

comment:2 by Kieran P, 13 years ago

Milestone: Alpha 4Alpha 5

comment:3 by Kieran P, 13 years ago

Milestone: Alpha 5Alpha 6
Priority: Nice to HaveIf Time Permits

comment:4 by Kieran P, 13 years ago

Milestone: Alpha 6Backlog

comment:5 by michael, 12 years ago

This would be insanely useful.

comment:6 by historic_bruno, 12 years ago

Owner: set to historic_bruno
Status: newassigned

comment:7 by historic_bruno, 12 years ago

Milestone: BacklogAlpha 9

So what I've got is as follows:

  • Double-click: select all entities on-screen with the same template
  • Band-boxing: selects all entities within the box
  • Shift+select: add to selected group
  • Ctrl+select: remove from selected group
  • Left-drag selection: moves all entities in the selection, maintaining relative positions and taking care of floating entities (e.g. boats)
  • Right-drag selection: rotates selection only if one entity is selected, otherwise does nothing

Multiple selection support:

  • Movement/translation: works
  • Deletion: works
  • Rotation: doesn't work (ignores command)
  • Set player ID: doesn't work (needs a lot of reworking)
  • Anything else?

These methods currently ignore player ownership, which I think is the most intuitive behavior for a map editor. Otherwise it's almost identical to in-game selections. Maybe eventually we can have some selection options for more fine grained controls, if people want them. Other than that, you have to play with it because it's not only useful but fun ;)

comment:8 by Pureon, 12 years ago

Would it be possible to add copy and paste (Ctrl-C + Ctrl-V) to multiple selections (and single entity ones). Not a priority, but it would be useful for a few things.

in reply to:  8 comment:9 by historic_bruno, 12 years ago

Replying to Pureon:

Would it be possible to add copy and paste (Ctrl-C + Ctrl-V) to multiple selections (and single entity ones). Not a priority, but it would be useful for a few things.

It's certainly possible but I've not looked into it yet. See #96.

in reply to:  5 comment:10 by historic_bruno, 12 years ago

Replying to michael:

This would be insanely useful.

Here's a question. In Atlas we can select both entities that are selectable in-game (they have the IID_Selectable component) and actors that are not. Consider a map like Belgian Bog that has a lot of decorative actors. If bandbox selection is used, should it select all actors, including decorative ones?

This is a question separate from the underlying technical issues that have to be solved, for instance, there's no selection ring (yet) for a decorative object. That can be fixed, if it's useful.

comment:11 by michael, 12 years ago

I think band box should default to entities, but if no entities in the box it'll select actors. If you band box around some entities and actors, it only selects the entities. If that's the case and you want to select the actors too, you can use ctrl-click (or ctrl-double click) to add the actors to the selection.

in reply to:  11 comment:12 by historic_bruno, 12 years ago

Replying to michael:

I think band box should default to entities, but if no entities in the box it'll select actors. If you band box around some entities and actors, it only selects the entities. If that's the case and you want to select the actors too, you can use ctrl-click (or ctrl-double click) to add the actors to the selection.

That conflicts a bit with how I envisioned Ctrl, it's kind of a universal "remove from selection" modifier (to match in-game selections). Might I suggest using Alt instead? Alt = A for actors :D

comment:13 by historic_bruno, 12 years ago

I discovered a few things after testing my patch:

  • Decals are not selectable since #914 was fixed, because they don't have selection boxes. So I've added a "fallback" to the old axis-aligned boxes for decals and other actors without selection boxes, until #1032 is fixed. Now decals can be selected and moved in Atlas.
  • At least one actor still causes problems: props/flora/water_lillies.xml. It has a selection box, but is apparently flat (2D) causing the selection algorithm to fail. The result is you might unintentionally select lillies, it's most noticeable on maps like Belgian Bog. This is not specific to my patch, so I've opened a ticket for it (#1121).
  • Actors in general are very annoying because it's not clear when you might select them by accident. I've decided to make this more explicit by requiring the Alt key to be held down to select actors in any circumstances. The other modifier keys (Ctrl, Shift) work as described above and can be used in combination.

I added selection rings to all actors, by implementing a new EditorOnly flag, that tells the engine not to select this entity in-game. I also added player colour to Atlas selection rings, because sometimes it's not clear who is the owner (when the model has no player colour).

by historic_bruno, 12 years ago

comment:14 by historic_bruno, 12 years ago

Keywords: review added
Summary: [Atlas] Improve unit selection options[PATCH] [Atlas] Improve unit selection options

Summary of fixes in this patch:

  • Multiple entity selection in Atlas:
    • Individual selection
    • Bandbox selection
    • Similar (matching template) selection
    • Add to/remove from existing selection
  • Move multiple selected entities in Atlas.
  • Add selection rings to all actors, use <EditorOnly/> flag for decorative objects, decals, etc.
  • Show player colours in Atlas selection rings (default to white).
  • Allow selecting entities regardless of ownership.
  • Fix selection of decals using old-style AABBs.
  • Use EntitySelection helper functions in place of UnitManager.
  • Allow double click events to be captured by Canvas.
  • Add DrawOverlays to Atlas views, to handle Atlas-specific rendering code.
  • Don't pass mouse move events from Transform tool, to prevent conflicting selection rings during game testing.

I'm kind of a noob when it comes to OpenGL stuff, feel free to suggest better methods of rendering the bandbox :)

comment:15 by historic_bruno, 12 years ago

Priority: If Time PermitsShould Have

comment:16 by vts, 12 years ago

Looks great as far as I can tell! Works very well, and significantly improves the usability of Atlas in my opinion.

I'm not nearly as good at reviewing patches as other people around here, but here are some things I noticed and some minor personal preferences:

tools/atlas/GameInterface/View.cpp

  • ViewGame::DrawOverlays needs an orthographic projection to be set up for the bandbox to show up. As discussed on IRC, some changes were made to the GUI rendering code since this patch was first submitted that were apparently being relied on.

tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp

  • The ps/Overlay.h include doesn't seem to be necessary.

tools/atlas/GameInterface/Shareable.h

  • As discussed on IRC, the changes to this file seem to be crossovers from some other patch and can be ignored :)

simulations2/helpers/Selection.cpp

  • In EntitySelection::PickEntitiesAtPoint and EntitySelection::PickEntitiesInRect, I'd prefer for the parameter name editorSelectable to be more explicit about whether it means editor-only selectables are included or excluded from the selections made. It's documented in the header, but I think it would improve readability if it would be called e.g. allowEditorSelectables.
  • For consistency, in EntitySelection::PickEntitiesAtPoint and EntitySelection::PickEntitiesInRect it might be nice to use INVALID_PLAYER instead of a literal -1, and to use player_id_t instead of int for the type of the owner argument (and also change the calling code to use INVALID_PLAYER instead of a literal -1).
  • Since PickEntitiesInRect and PickSimilarEntities have documented behaviour for the owner argument being -1, I thought it might be nice for PickEntitiesAtPoint to also mention that the selection will be limited to entities that CCmpRangeManager::GetLosVisibility says are visible for that player. (Since Atlas registers -1/INVALID_PLAYER to reveal the entire map, thus also enabling behaviour similar to the other functions).

ps/GameSetup/GameSetup.cpp

  • I'm not very familiar with the game setup code, and thought the following bit of code in the Render function was confusing:
    if (g_GameLoop && g_GameLoop->view)
    	g_GameLoop->view->DrawOverlays();
    

The Render method itself is part of the main game's Frame() loop, so it seems to call out to the game loop while it's already in the game loop. I learned afterwards that g_GameLoop is entirely specific to Atlas, serving to support Atlas' own, separate game loop (RunEngine in tools/atlas/GameInterface/GameLoop.cpp I believe). For this reason, I'd suggest for g_GameLoop to be renamed to g_AtlasGameLoop.

The latter uses the current Atlas View instance's Render method, of which ViewGame::Render actually appears to be reusing the main game loop's Render function for understandable purposes. This is presumably also the reason, then, why Atlas-gameloop rendering functionality was added to the main game's rendering function (so as to not have to duplicate its entire setup).

Still, to my mind introducing Atlas-specific rendering calls into the main game's rendering function feels like it's entangling separate concerns, although admittedly I'd also be hard-pressed to come up with a better alternative. I suspect any alternative would probably be quite involved and not worth the effort.

Finally, I feel it would benefit readability and make it easier for new members to get familiar with the codebase if the View, ViewGame and ViewActor classes in tools/atlas/GameInterface/View.cpp were to be prefixed with Atlas. The main game has its own view called CGameView, so I feel it would make sense for Atlas' views to be called (C)AtlasView. But primarily, it would help bring some clarity to the purpose of all the various classes with View in them. Currently there's View, CGameView, ViewGame, ViewActor, ActorViewer, and it can be unnecessarily hard to keep them separate if you're not already familiar with what they do.

in reply to:  16 comment:17 by historic_bruno, 12 years ago

Thanks for getting around to this :)

Replying to vts:

tools/atlas/GameInterface/View.cpp

  • ViewGame::DrawOverlays needs an orthographic projection to be set up for the bandbox to show up. As discussed on IRC, some changes were made to the GUI rendering code since this patch was first submitted that were apparently being relied on.

I'll do something like GetDefaultGuiMatrix, it won't work for GLES but that will be a TODO if someone cares enough :P

simulations2/helpers/Selection.cpp

  • In EntitySelection::PickEntitiesAtPoint and EntitySelection::PickEntitiesInRect, I'd prefer for the parameter name editorSelectable to be more explicit about whether it means editor-only selectables are included or excluded from the selections made. It's documented in the header, but I think it would improve readability if it would be called e.g. allowEditorSelectables.

Good idea, I've changed it.

  • For consistency, in EntitySelection::PickEntitiesAtPoint and EntitySelection::PickEntitiesInRect it might be nice to use INVALID_PLAYER instead of a literal -1, and to use player_id_t instead of int for the type of the owner argument .

Using player_id_t is more correct and I changed that for these functions, unfortunately there are not a few other places that also use int for player IDs, and I don't know what I might break by changing that, so I'll leave it as a separate task. (It's not a big deal these days as int should likely mean at least 32-bit integer and we don't support anywhere near that many players - but still it's important to make sure we're consistent and nothing breaks).

(and also change the calling code to use INVALID_PLAYER instead of a literal -1)

  • Since PickEntitiesInRect and PickSimilarEntities have documented behaviour for the owner argument being -1, I thought it might be nice for PickEntitiesAtPoint to also mention that the selection will be limited to entities that CCmpRangeManager::GetLosVisibility says are visible for that player. (Since Atlas registers -1/INVALID_PLAYER to reveal the entire map, thus also enabling behaviour similar to the other functions).

I didn't even know that value existed, fixed :) I have to look up what exactly player -1 means when calling GetLosVisibility, then I'll add it to the comment.

ps/GameSetup/GameSetup.cpp

Still, to my mind introducing Atlas-specific rendering calls into the main game's rendering function feels like it's entangling separate concerns, although admittedly I'd also be hard-pressed to come up with a better alternative. I suspect any alternative would probably be quite involved and not worth the effort.

Yeah, that was my initial concern as well, but since there's not a lot of special Atlas-only rendering yet, it's hard to justify redesigning the render loop. I don't know how to go about doing that either, so Philip and I discussed it and the current solution is the best we came up with, although it is a bit of a hack.

I agree though Atlas could definitely use some refactoring in places, but again I'll leave that as a separate task for now ;)

comment:18 by ben, 12 years ago

Resolution: fixed
Status: assignedclosed

In 11177:

Adds multiple entity selection to Atlas (including move/delete). Fixes #678.
Makes all actors selectable in Atlas and gives them selection rings (an EditorOnly flag is used in the template for Atlas-only selectables).
Adds player colors to Atlas selection rings.
Fixes decal selection by falling back to old-style AABBs. Refs #1032.
Replaces UnitManager selections with EntitySelection helpers.
Adds DrawOverlays to Atlas views, for Atlas-specific rendering.
Fixes bug where selection rings conflicted with Move/rotate tool in Atlas simulation test.

comment:19 by historic_bruno, 12 years ago

Keywords: review removed

comment:20 by vts, 12 years ago

In 11232:

Renamed some Atlas classes and g_GameLoop to better reflect their Atlas-related nature and to lower potential confusion with similar main-game-related functionality. Refs #678.

Note: See TracTickets for help on using tickets.