Issues with desktop right-click to move

[xlenstra]

On desktop, you can rightclick after selecting a unit to quickly move or attack with it. However, there are a few bugs that can happen when doing this. Most of these are, because not all necessary checks are made when using this.

• [x] Spectators can move any unit they want by doing this
• [ ] Carriers can preform ranged attacks by doing this
• [ ] Attacking with an air unit and then rightclicking on any tile will cause an "I can't go there!" exception and remove the air unit from play
• [ ] You can move units while it is not your turn in multiplayer
• [ ] Probably more, but those are the ones I know from the top of my head

[SomeTroglodyte]

We have right-click?? 👀 😮

[Azzurite]

This is basically a code duplication issue. "right-click attack" and "normal attack" should use the exact same code to perform the attack instead of two separate implementations. That should fix all these issues.

[yairm210]

@Azzurite I think this can be closed as "resolved"?

[Azzurite]

I think this can be closed as "resolved"?

Can it? There are 4 check marks (3 with specific issues) not checked, and I don't know the status of these. Do you?

[SomeTroglodyte]

"Carriers can preform ranged attacks by doing this"

Untrue, not reproducible, but:

"Carriers can perform ranged attacks by doing this" - Yes, still bugged. Sorry for the typo joke.

Their attack is melee by now, but the implementation of the "Cannot attack" unique was flawed from the start. Implemented in the UI (BattleTable line 62), not logic. Yes, seems to me the only place that unique has an effect for human players is in the decision who to display in the UI battle pane. Which means Azzurite is right, the entire battle system needs something between thorough analysis with some restructuring, to redesign - I'm too cowardly and not quite fluent enough in the system to do that properly.

That said, the proper test for the unique would feel at home in MapUnit.canAttack() but probably better goes into BattleHelper.containsAttackableEnemy since CanOnlyAttackUnits and CanOnlyAttackTiles are there as well. Keep the bad one, commented, so the battle table does not appear (otherwise we'd see the strength relations + damage prediction with a grayed Attack button - maybe some would prefer that?)

Anyone listening can verify that patch approach?

"Attacking with an air unit and then rightclicking on any tile will..."

Not a problem anymore, before or after attacking, a air unit right-click on a non-attackable, non-move-to-able tile will end up the same, in that first exception handler in moveUnitToTargetTile. I'd say suppress the Log it it's UnreachableDestinationException as that situation is easy to produce and should count as normal - e.g. Ship onto coastal land, Warrion onto coast before embarcation, they all land there.

You can move units while it is not your turn in multiplayer

I don't do multiplayer, and find it too hard to stage a testing situation on a single box. Can anyone else check this?

[github-actions[bot]]

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.

[SeventhM]

Concerns raised in OP seems to be addressed. Closing as solved