-
Notifications
You must be signed in to change notification settings - Fork 719
AFK team mate: better ship handling + tests + bugfix #2203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
92c4324
Better ship handling for AFK player
VariableVince 870a35a
New src for retreating transport
VariableVince 0f867eb
Small fix
VariableVince ac128e0
Merge branch 'v26' into warship-afk-handling
VariableVince d479c3e
Warships never attack when friendly is disconnected
VariableVince 1b44273
Fix leftover troop removal on attack and add tests
VariableVince 3c0baa5
Fix condition
VariableVince f583e5b
Merge branch 'v26' into warship-afk-handling
VariableVince fac44c8
Merge branch 'v26' into warship-afk-handling
VariableVince c84e532
Originalowner = attacker
VariableVince 705fce6
Move src check to retreating block
VariableVince a85b6da
add param to isFriendly to prevent afk warship attack
VariableVince 9e7d9ef
Prettier
VariableVince b844bea
origowner outside constr
VariableVince d26886b
Merge branch 'v26' into warship-afk-handling
VariableVince 8b4f35b
Fix origowner
VariableVince dd13a89
Use bestTransportShipSpawn instead of canBuild
VariableVince 3c0d967
Fix no troop loss
VariableVince 32bbdf2
Wording
VariableVince d3382a7
Merge branch 'v26' into warship-afk-handling
VariableVince 04d9c76
No troop remove at init but keep bugfix and test
VariableVince cbcd3db
Prettier
VariableVince 8abebe6
Merge branch 'v26' into warship-afk-handling
VariableVince a91c671
Pascal Case and more ticks
VariableVince fe854c0
Merge branch 'warship-afk-handling' of https://github.com/openfrontio…
VariableVince a7974d9
Merge branch 'v26' into warship-afk-handling
evanpelle File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have no troop loss when attacking afk teammate, in DefaultConfig.attackLogic. This looks like it could allow players to gain additional troops, since they will get their troops back after the attack completes.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The attacker has less troops left after the attack currently, than expected without troop loss. This is what i discovered when making test "Player can attack disconnected team mate without troop loss" in Disconnected.test.ts, which is part of this PR. It tests this by calculating the normal troop growth, then attacking, and afterwards checking if troops have increased by the normal troop growth. The player should have the number of troops afterwards that he would have had when the attack had not taken place. But each test run the player had less troops than that after the attack. If the start troops that are removed in the AttackExecution init were added back after the attack, that should not have happened.
The test only started passing after applying these changes in the AttackExecition init, ie. remove no troops at the start of the attack on the disconnected teammate.
So like you I was not expecting troop loss either based on AttackLoss in DefaultConfig. But AttackExecution removes the start troops (attack ratio %) in its init. Even for attacks on disconnected team mates. And those start troops apparently are not returned after the attack ends. Otherwise the test "Player can attack disconnected team mate without troop loss" would have already passed before the fix.
But it could be i am missing something of course. Could you maybe point me to the code where troops are added back? Or is the test flawed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... looking at the code is seems the refund happens only after the attack completes, and in that test it only executes 2 ticks, so idk if the attack completes?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes i missed that, thanks for looking into it again! Since i still find it better if there's no visual troop loss either, i kept with it but fixed how it works.
It now sets the existing removeTroops variable to True, still resulting in startTroops not being removed from owner troops using existing code.
Next is basically a bug fix: when the attack ends it checks if removeTroops was set to True in the retreat() function, and if so it subtracts the startTroops from the attack troops. So the startTroops aren't added back to the owner troops when they were never removed from owner troops in the first place. Variable removeTroops variable could have been set to False for another reason someday so this fix was actually needed anyway.
Added comments to explain, maybe a bit verbose.
Changed test "Player can attack disconnected team mate without troop loss" so it fully conquers the disconnected player before checking if there was no troop loss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking about it in the Dev server I decided to remove the 'do not remove troops from owner at attackexecution init' part from the PR. Left in the bug fix in retreat(), which was needed already anyway in the case anyone would set existing variable removeTroops to false. Also left in the test "Player can attack disconnected team mate without troop loss" in Disconnected.test.ts.
(You don't have to read this, but before deciding on removing the 'do not remove troops from owner at attackexecution init' part from this PR, I did consider and try:
Setting startTroops to 0 so attack.troops() would be 0. But this leads to devide-by-zero calculations in AttackLoss in DefaultConfig. It would be too much for this PR to change this. I propose we do this in another PR. The attack should, imo, feel more like a swift annexation than like a normal attack (but it should still be an attack just an easy one, not some automatic annexation, an attack adds tension to the game because you need to be quick about it before the enemy attacks the AFK teammate). AttackLoss could get a specific part for annexing an AFK teammate, that can handle attack.troops 0 and make it feel annexation-like. Once we can set startTroops/attack.troops to 0, so there is no initial removal of owner troops and no troops in the attack, we can have renderTroops in Utils.ts return an empty string for "0" so EventDisplay just displays "AFK Teammate" and not "0 AFK Teammate" so there's no confusion about an attack with 0 troops.
Add a 'hideTroopsInUI' flag to attack.troops in AttackUpdate. So even when attack.troops would be >0, it would just send attack.troops 0 to the UI like EventsDisplay. We could then have renderTroops in Utils.ts return an empty string for "0" so EventDisplay just displays "AFK Teammate" and not "0 AFK Teammate" so there would be no confusion about an attack with 0 troops. However, just hiding it in the UI would cause confusion in itself because the attack would still feel slow like now, you get no feedback as to why it is slow. Only if the attack becomes swift annexation-like (see above) would it make sense to not display troops in the UI.)