The pathfinder prefers goals that are in reachable regions, so when we move to walk to a point inside the obstruction of a tree or building, it would be better to instead walk to the tree or building. Already we "walk to target" for commands like gather or repair with units that can't complete those actions. We should do it for the regular "walk" command too.
Details
- Reviewers
temple
We could add a move-to-target unit action and a walk-to-target command instead of combining them with move and walk. I don't know which way is better.
Currently we try to move as close as possible to the point we clicked, but now we'll move to the nearest point on the obstruction. So if you click on the far side of a building, the units won't move around the building anymore. You have to click on open ground to move them to the other side.
Because of this, I disabled DrawTargetMarker when we move to a building, because that seems misleading (we don't necessarily move close to where the marker's drawn).
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- D1074_walk_to_target
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 7226 Build 11777: Vulcan Build Jenkins Build 11776: arc lint + arc unit
Event Timeline
I do like the idea, but this:
Currently we try to move as close as possible to the point we clicked, but now we'll move to the nearest point on the obstruction. So if you click on the far side of a building, the units won't move around the building anymore. You have to click on open ground to move them to the other side.
Seems like it could be annoying with our tall buildings and tall trees tbh.
Well, it's already annoying:
- Click on a tall building and units will drop off resources there instead of moving to the far side of the building.
- Click at the top of the tree and infantry will start chopping it down instead of moving to the point beyond the tree.
There was some agreement at D674 to create a hotkey to force a move (to point) command, so that would address this concern.
Fixed foundations, also changed my mind on removing the target marker.
The whole obstruction of a gate is targeted, so units will move to the edge of it like other other buildings. Gates are buggy pathfinding-wise anyway, so I don't consider this a huge deal. With a move-to-point hotkey they could still move to the center of the gate if they really wanted (they can also past it then stop when the units are halfway through; currently this is what you have to do at enemy gates since clicking them does an attack command rather than move).
binaries/data/mods/public/gui/session/unit_actions.js | ||
---|---|---|
105–106 | why don't do the same here? also rallypoint (or was there a patch for that already?) and patrol (or does it do it already?) |
OK, but I'd like a "move-only" hotkey of some kind before this gets committed (or I'd rather just not commit this because of the annoyances described above).
Also yes this should be done for attack-walk too, but that can be done before committing imo.
Done in rP20594.
binaries/data/mods/public/gui/session/unit_actions.js | ||
---|---|---|
105–106 | Attack-move and patrol are trickier, you'd have to rewrite chunks of UnitAI to use a target rather than position, and I'm not sure it's worth it. (Although I think those need some work anyway, e.g. if a unit is dead when we're trying to attack it, in regular attacking we'll use the attack on a new unit, but in walking-and-fighting we won't so we waste the attack). It could be used in rally point, although usually there's some other action that takes precedence, e.g. gather or garrison, and units that can't do that action already walk to target instead. |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... binaries/data/mods/public/gui/session/unit_actions.js | 561| » » » switch·(tradingDetails.type) | | [NORMAL] ESLintBear (default-case): | | Expected a default case. binaries/data/mods/public/simulation/helpers/Commands.js | 908| » var·ids·=·[·id·for·(id·in·members)·]; | | [MAJOR] ESLintBear: | | Parsing error: Unexpected token for binaries/data/mods/public/simulation/helpers/Commands.js | 53| var·g_Commands·=·{ | | [NORMAL] JSHintBear: | | 'g_Commands' was used before it was defined. binaries/data/mods/public/simulation/helpers/Commands.js | 549| » » » » ····&&·player·!=·+cmd.owner) | | [NORMAL] JSHintBear: | | Misleading line break before '&&'; readers may interpret this as an expression boundary. binaries/data/mods/public/simulation/helpers/Commands.js | 737| » » » » var·cmpGUIInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface); | | [NORMAL] JSHintBear: | | 'cmpGUIInterface' is already defined. binaries/data/mods/public/simulation/helpers/Commands.js | 908| » var·ids·=·[·id·for·(id·in·members)·]; | | [NORMAL] JSHintBear: | | 'array comprehension' is only available in Mozilla JavaScript extensions (use moz option). binaries/data/mods/public/simulation/helpers/Commands.js | 908| » var·ids·=·[·id·for·(id·in·members)·]; | | [NORMAL] JSHintBear: | | Expected 'for' and instead saw 'id'. binaries/data/mods/public/simulation/helpers/Commands.js | 958| » » for·(var·i·=·0;·i·<·length;·++i) | | [NORMAL] JSHintBear: | | 'i' is already defined. binaries/data/mods/public/simulation/helpers/Commands.js | 971| » » var·count·=·0; | | [NORMAL] JSHintBear: | | 'count' is already defined. binaries/data/mods/public/simulation/helpers/Commands.js |1118| » » var·cmpGuiInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface); | | [NORMAL] JSHintBear: | | 'cmpGuiInterface' is already defined. binaries/data/mods/public/simulation/helpers/Commands.js |1370| » » var·piece·=·pieces[j]; | | [NORMAL] JSHintBear: | | 'piece' is already defined. binaries/data/mods/public/simulation/helpers/Commands.js |1453| » » var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI); | | [NORMAL] JSHintBear: | | 'cmpUnitAI' is already defined. binaries/data/mods/public/simulation/helpers/Commands.js |1491| » » » &&·cmpFormation.GetMemberCount()·==·formation.entities.length) | | [NORMAL] JSHintBear: | | Misleading line break before '&&'; readers may interpret this as an expression boundary. binaries/data/mods/public/simulation/helpers/Commands.js |1517| » » » » » var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI); | | [NORMAL] JSHintBear: | | 'cmpUnitAI' is already defined. binaries/data/mods/public/simulation/helpers/Commands.js |1550| » » » var·cmpFormation·=·Engine.QueryInterface(formationEnt,·IID_Formation); | | [NORMAL] JSHintBear: | | 'cmpFormation' is already defined. Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1242/display/redirect
I lack the time for a proper review and test, but won't this give unexpected behaviour when one thinks one clicks just beyond a building and the entities move to the building instead of closest to the point one clicked?
[EDIT]: I should have read more carefully the first time, it is already said above.
As explained above, one can use the "move" shortcut, or the attack-move shortcut.
I'm not sure this is such a big issue, and the behaviour is quite natural when you do intend to click on the building imo. We can always revert the change if players dislike it.