Page MenuHomeWildfire Games

Walk to target rather than walk to point
Needs ReviewPublic

Authored by wraitii on Nov 27 2017, 12:57 AM.

Details

Reviewers
temple
Summary

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.

Test Plan

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

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

temple created this revision.Nov 27 2017, 12:57 AM

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.

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.

temple updated this revision to Diff 4409.Nov 27 2017, 4:07 PM

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).

bb added a subscriber: bb.Nov 28 2017, 5:58 PM
bb added inline comments.
binaries/data/mods/public/gui/session/unit_actions.js
97–98

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?)

wraitii accepted this revision.EditedDec 3 2017, 6:34 PM

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.

This revision is now accepted and ready to land.Dec 3 2017, 6:34 PM

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).

Done in rP20594.

binaries/data/mods/public/gui/session/unit_actions.js
97–98

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.

Thanks for the patch, will commit this after A23 is released. Sorry for the delay.

wraitii commandeered this revision.Apr 20 2019, 5:25 PM
wraitii edited reviewers, added: temple; removed: wraitii.
This revision now requires review to proceed.Apr 20 2019, 5:25 PM
wraitii updated this revision to Diff 7786.Apr 20 2019, 5:26 PM

Remove the "only if blocking pathfinding" because I don't see the point tbh.

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 will commit this soon.

Freagarach added a subscriber: Freagarach.EditedJun 2 2020, 6:23 PM

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.

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?

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.

Guess I should ping @borg- and @ValihrAnt here, what do you think about this?

@badosu, @Feldfeld, @genava55 any time to test this and provide us with feedback?

binaries/data/mods/public/simulation/helpers/Commands.js
165

You can do an else and ditch the return and braces.