[Controller] feat: no more melee auto-chasing

[bmcszk]

Primary Action Button changes

A proposition of change described in: #6436

These changes prevent player's character from chasing targets with melee weapons when using a controller.

When Primary Action button is pressed, the following checks are being done from top to bottom:

• Townsfolk target - talk
• Monster target:
  • talking monster - talk but only on close range
  • range - attack monster
  • melee (close target) - attack monster
  • melee (distant target) - attack monster's location; the same as stand ground attack
• Player target (hostile):
  • range - attack player
  • melee (close target) - attack player
  • melee (distant target) - attack player's location; the same as stand ground attack
• No monster/player target:
  • interact with object if any
  • attack a location the player is facing; the same as stand ground attack

The result is smooth combat using melee weapon and controller when movement is controlled only by stick and Primary Action fights the closest enemy.

Implementation details

I am using preexisting function

GetMinDistance(position)

to determine if target is close or distant.

Bugfix

I found a bug when testing PvP with controllers.
The bug prevented targeting another player in melee mode.

newDdistance = GetDistance(player.position.future, distance);
where distance is a maxDistance for a distance calculation so should not be 0 as initial value. I've changed the initial value from 0 to INT_MAX.

Bugfix is in a separate PR:
#6494

Tests

The tests where done on two PCs using controller and mouse/keyboard. Tested:

• melee combat
• ranged combat
• spell casting
• talking to monsters
• PvP using melee
• PvP using ranged

[StephenCWills]

I can still get my character to chase down enemies if I press-and-hold the primary action button while moving the analog stick in a direction.

I can no longer interact with Snotspill without the aforementioned workaround.

It feels a bit odd to me to pass the AxisDirection into the IsStandingGround() function. Standing ground is a deliberate action on behalf of the player whereas standing still is a passive state of being resulting from a lack of deliberate action. I think this fundamental difference is the reason why this change doesn't work quite right, because the code block you've activated is based on the assumption that the player has made a deliberate action to disable movement. Instead, all you should need to do is modify the logic in Interact() where the game handles player interaction with monsters, and you shouldn't need to check whether the player is standing still at all.

[bmcszk]

I can still get my character to chase down enemies if I press-and-hold the primary action button while moving the analog stick in a direction.

I can no longer interact with Snotspill without the aforementioned workaround.

It feels a bit odd to me to pass the AxisDirection into the IsStandingGround() function. Standing ground is a deliberate action on behalf of the player whereas standing still is a passive state of being resulting from a lack of deliberate action. I think this fundamental difference is the reason why this change doesn't work quite right, because the code block you've activated is based on the assumption that the player has made a deliberate action to disable movement. Instead, all you should need to do is modify the logic in Interact() where the game handles player interaction with monsters, and you shouldn't need to check whether the player is standing still at all.

You are right, the previous commit wasn't good enough. I've changed the approach and played with Interact() function a bit. It works, tested:

• playing with controller melee and range
• playing with mouse melee and range
• interaction with Snotspill (this one moves player to target but it's a minor thing)

Not yet tested:

• other platforms
• PvP

[StephenCWills]

Nice work. Your changes here are better than I envisioned. Unfortunately, I can see one issue that will still need to be addressed, though I'm not sure if it needs to be done in this PR.

NetSendCmdLoc(MyPlayerId, true, myPlayer.UsesRangedWeapon() ? CMD_RATTACKXY : CMD_SATTACKXY, position);
LastMouseButtonAction = MouseActionType::Attack;

This breaks the "repeat action" logic in track.cpp because the logic that handles MouseActionType::Attack depends on cursPosition and gamepad deliberately invalidates cursPosition. This was already a problem with the "stand ground" feature on gamepad, but we chose not to address it right away since it was reported right as we were approaching the 1.5.0 release. However, this PR doesn't just break the functionality when using stand ground so we'd have to consider that a regression.

I was thinking about how to handle this problem yesterday, and I think it would probably be best to introduce CMD_SATTACKID and then somehow determine in track.cpp which of the three attack messages would be most appropriate to repeat. Perhaps it would be good enough to check ControlMode for that. It kinda depends on whether @AJenbo thinks this PR is good as a first step or if he'd like to keep the original move+attack functionality as an option.

[bmcszk]

@StephenCWills thanks,
The PR is simply my subjective proposition of change. I have no idea how this change would work on other devices. This impacts the virtual controller for smartphone and tablets, isn't it? I wonder if it improves gameplay there or makes it worse?

[StephenCWills]

This impacts the virtual controller for smartphone and tablets, isn't it?

Yes, it should affect virtual gamepad in the same way it affects gamepad.

I wonder if it improves gameplay there or makes it worse?

The answer to that is subjective, but there's a good chance your experience with virtual gamepad would be similar to your experience with gamepad.

[bmcszk]

@StephenCWills

This breaks the "repeat action" logic

I think I have a workaround in my latest commit. I've tried to use CMD_RATTACKXY : CMD_SATTACKXY as few as possible and it works pretty good.

[bmcszk]

Hey @StephenCWills,
I've done more tests including PvP so I believe the PR is ready.
I've put a proper PR description.
Now it's up to maintainers if they like it or not.