Implement auto battle by mateofio · Pull Request #2331 · EasyRPG/Player

mateofio · 2020-09-12T04:55:52Z

This implements RPG_RT compatible auto battle. I've designed auto battle to support different auto battle algorithms. The end goal will be Player ships a few algos that you can choose from. Game developers customizing player could also add their own to the AutoBattle::CreateAlgorithm factory function.

This PR provides 3 algorithms:

RPG_RT - RPG_RT compatible including bugs - this is the default
ATTACK - RPG_RT, only physical attacks, bug fixes included
RPG_RT+ - RPG_RT with bug fixes - Also open to further improvements

For now I just added a --autobattle-algo command line parameter to select the algo, later this can probably be part of Scene_Settings etc.. Or maybe even a custom save chunk.

TODO

DynRPG Addresses

Address	Class	Name	Comment
004BA518	Actor	SelectAutoBattleAction	Virtual function which runs the whole auto battle algorithm, everything is here
004B9EF0	Actor	GetAutoBattleSkillRank	Computes best ranking of skill against all possible targets
004BA384	Actor	GetAutoBattleAttackRank	Computes best rank of normal attack against all possible targets
004BA0B8	Actor	GetAutoBattleSkillEffectRank	Computes the rank of a skill against a single target
004BA3E8	Actor	GetAutoBattleAttackEffectRank	Computes the rank of a normal attack against a single target

RPG_RT bugs ❗

Normal attack ranking does not consider attack all. It should sum the ranks of all enemies
Normal attack ranking does not consider dual attack
Normal attack ranking doesn't accurately consider dual wield for 2k3
Normal attack ranked does not consider sp cost of weapons
Skill ranking uses variance, normal attack does not
Revive skill ranking does not check for the reverse_state flag, and so a skill which inflicts death on an ally would be highly ranked and probably used on a dead ally.

NovemberJoy · 2020-09-13T10:21:28Z

In its current state, the timing for auto-battle in 2k3 is off by enough to grant a second, player-controlled turn to AI characters after their standard turn. However, the issue was strangely intermittent - in one case, it happened with two out of three characters, with the third never getting an extra turn. After further testing, it appears to be dependent on stats or more likely animation timing.

Ghabry

src/autobattle.cpp

Ghabry · 2020-09-14T23:16:46Z

src/autobattle.cpp

+}
+
+void AlgorithmBase::SetAutoBattleAction(Game_Actor& source) {
+	vSetAutoBattleAction(source);


Is this some convention to use v?
I have a similiar "call forwarder to virtual" in my unfinished Filesystem code.

There I use XXXXX and for virtual XXXXImpl.

Opinion?

This is the non-virtual idiom NVI
http://www.gotw.ca/publications/mill18.htm

If I were a more disciplined NVI folllower, I would do this for all virtual functions. I like the v prefix, but I don't really care that much what naming scheme is used.

This is also a good way to fix the clang-tidy warnings on GetBaseAtk() etc..

src/autobattle.h

src/autobattle.cpp

Ghabry · 2020-09-15T00:01:16Z

More Todos:
The autobattle options must be added to the manpage and to the "--help" output and needs config file support.

For later (editor, pls):
Imo this belongs in LDB for a default game value and needs a new event command "Select Battle Algorithm" which is stored in LSD.

mateofio · 2020-09-15T01:06:40Z

I created Game_ConfigPlayer for this and other related future items, and wired it up. Added the manpages etc.. too.

I don't think we need anything else for this iteration. Now this just has to be tested.

mateofio · 2020-09-15T01:35:59Z

I checked all the RPG_RT versions and found something really strange. Every version of the engine uses double division for maxhp, maxsp, etc.. math.

except 2ke. For some reason I can't grasp, it looks like 2ke developers actually decided to go into the auto battle code and switch to using integer division.

Other than that there are no differences.

Given the amount of randomness here and the pain required to support both integer and double (templates), I'm inclined to leave it with double division.

Ghabry · 2020-09-15T01:41:16Z

Maybe @CherryDT as 2ke developer can give some insight here?
Could be also some strange compiler behaviour maybe.

CherryDT · 2020-09-15T10:16:29Z

Weird indeed, I didn't change anything. How is the behavior in the Japanese Value!+ version 1.52? This is the version that I changed to become 1.60. Unlike 2k3e, I actually changed the code and compiled from source - it should be the same compiler (Delphi 5) but maybe originally some different compiler flags were used or something (although I'd expect those settings to be stored in the project file)... If it changed only between 1.52 and 1.60 then it's a bit of a mystery...

mateofio · 2020-09-15T13:42:22Z

I got an RPG_RT binary from here https://tkool.jp/products/trial.html which I believe is 1.5.2 JP?

I checked that one and it is using double math.

CherryDT · 2020-09-15T14:22:47Z

I went to check what's going on there, and there is no change in Git for the relevant file. So if anything was changed, it must have happened before I got the code.

I wonder if it's indeed a subtle compiler difference or something like that because the calculation (if that's the one you meant, that is) looks like this (in pseudocode):

x = min(p, maxHp - hp) / maxHp;

x is a double, but p, maxHp and hp are integers. Now I wonder if maybe the compiler version I used does this using integer division because all terms in the formula are integers, while the compiler version used by the original devs used double division because the result of the calculation is assigned to a double variable? Not sure.

But actually that doesn't sound right either, because I checked the help file in my Delphi version and it says that / should always do double division (as opposed to div which does integer division):

🤷‍♂️

mateofio · 2020-09-15T14:44:51Z

I wonder if it's indeed a subtle compiler difference or something like that because the calculation (if that's the one you meant, that is) looks like this (in pseudocode):
x = min(p, maxHp - hp) / maxHp;
x is a double, but p, maxHp and hp are integers. Now I wonder if maybe the compiler version I used does this using integer division because all terms in the formula are integers, while the compiler version used by the original devs used double division because the result of the calculation is assigned to a double variable? Not sure.

Yes that is the division I'm talking about, that and the cost / maxSp that happens for skills.

I guess this one will remain a mystery..

mateofio · 2020-09-15T14:57:42Z

Sorry but it looks like this was a red herring. I was looking at the wrong disassembly. It indeed appears that double division is being used by all version, including 2ke.

Sorry about that

ghost · 2020-09-16T16:11:12Z

I have checked out your auto battle implementation and I like it so far! But I noticed some weird behavior in RPG Maker 2003 battles: In auto battle mode, after an actor executed his automatic selected action, the game switches to manual battle mode and even grants the actor an extra turn (automatic and manual action in the same turn despite ATB being zero after the automatic action).

mateofio · 2020-09-20T22:58:01Z

Thanks, I fixed the auto battle 2k3 bug you mentioned.

src/autobattle.h

Ghabry · 2020-09-21T13:45:24Z

Jenkins: test this please

mateofio · 2020-09-30T01:45:44Z

I've added a start for some unit tests. This is as far as I will go on this PR.

Ghabry · 2020-09-30T14:09:48Z

src/scene_battle.cpp

-	Output::Debug("Starting battle {} ({})", troop_id, troop->name);
+	autobattle_algo = AutoBattle::CreateAlgorithm(Player::player_config.autobattle_algo.Get());
+
+	Output::Debug("Starting battle {} ({}) : autobattle={}", troop_id, troop->name, autobattle_algo->GetName());


isn't it a bit verbose to print the constant autobattle algo everytime a battle starts?

btw is the algorithm name case sensitive or can I also specify "attack" on the command line?

My original design was printing this once on startup in it's own logfile line. I was thinking of future extensibility where you could change the algo at runtime, but since we don't have that yet this is a bit overkill.

The algo name is case sensitive, I can fix that

Added the case insensitive name compare. I left the log message in as it seems to me it'll be better for debugging when the algo is changeable in scene settings etc..

That being said if you feel strongly I'll remove it.

well keep it but please get rid of the whitespace before the ":". (Also imo a comma or a semi-colon would fit better here.)

Ghabry · 2020-10-05T09:01:25Z

Sorry I wanted to review this and comparing with RPG_RT on the weekend but I'm away from my dev-station until wednesday so this has to wait a bit longer.

src/autobattle.cpp

Ghabry

some remarks, but imo the algorithms are correct

Ghabry · 2020-10-08T19:34:22Z

resources/unix/bash-completion/easyrpg-player

      #_filedir '@(lmu|emu)'
      return
      ;;
+    # load map files


load map files?

Ghabry · 2020-10-08T19:35:25Z

src/autobattle.cpp

+
+		auto rank = static_cast<double>(max_effect) / static_cast<double>(tgt_max_hp);
+		if (src_max_sp > 0) {
+			const double cost = source.CalculateSkillCost(skill.ID);


For CalculateSkillCost RPG_RT appears to ignore the "Half SP" (or I missed it). Maybe also emulate this bug?

Nice catch, you're right this is another RPG_RT bug

Ghabry · 2020-10-08T19:35:48Z

src/autobattle.cpp

+	}
+	if (src_max_sp > 0) {
+		const double cost = source.CalculateSkillCost(skill.ID);
+		// Note: RPG_RT 2ke only uses integer division for cost / src_max_sp here.


same two comments apply here

Ghabry · 2020-10-08T19:36:22Z

src/autobattle.cpp

+	const double tgt_hp = target.GetHp();
+
+	if (target.GetHp() > 0) {
+		// Can the skill can heal the target?


remove 2nd can

Ghabry · 2020-10-08T19:39:50Z

src/player.cpp

+                           Possible options:
+                            RPG_RT     - The default RPG_RT compatible algo, including RPG_RT bugs
+                            RPG_RT+    - The default RPG_RT compatible algo, with bug fixes
+                            ATTACK     - RPG_RT+ but only physical attacks, no skills


The lines are too long (>80) but this is a problem in many parts of the help output

Would like to punt until we have a wholistic answer to this one

Ghabry · 2020-10-08T19:41:22Z

src/scene_battle.cpp

-	Output::Debug("Starting battle {} ({})", troop_id, troop->name);
+	autobattle_algo = AutoBattle::CreateAlgorithm(Player::player_config.autobattle_algo.Get());
+
+	Output::Debug("Starting battle {} ({}) : autobattle={}", troop_id, troop->name, autobattle_algo->GetName());


well keep it but please get rid of the whitespace before the ":". (Also imo a comma or a semi-colon would fit better here.)

mateofio · 2020-10-09T00:08:08Z

Addressed review comments. I also found and fixed and off by 1 bug in CalculateSpCost().

The question of the algo logging will come up again in #2381 as I log the enemyai algo too. Let's figure it out for good when we get to that PR.

Adds: 3 algorithms * RPG_RT - bug compatible with RPG_RT * ATTACK - only physical attacks + bug fixes * RPG_RT+ - RPG_RT algo + bug fixes

When halfSpCost() modifier is used, we add 1 to flat sp cost but not percent sp cost.

RPG_RT ignores half sp cost modifier

ghost added the Battle label Sep 12, 2020

ghost added this to the 0.6.3 milestone Sep 12, 2020

Ghabry added the Has PR Dependencies This PR depends on another PR label Sep 12, 2020

mateofio force-pushed the autobattle branch from 700654f to 6536569 Compare September 14, 2020 23:03

mateofio removed the Has PR Dependencies This PR depends on another PR label Sep 14, 2020

Ghabry reviewed Sep 14, 2020

View reviewed changes

mateofio force-pushed the autobattle branch 2 times, most recently from 069dc29 to b1572eb Compare September 15, 2020 00:23

Ghabry mentioned this pull request Sep 15, 2020

New Event Commands (request them here!) #2338

Open

mateofio force-pushed the autobattle branch from b1572eb to f606d6d Compare September 15, 2020 01:05

mateofio force-pushed the autobattle branch from f606d6d to 8903fae Compare September 15, 2020 01:45

mateofio force-pushed the autobattle branch from 8903fae to f866a06 Compare September 20, 2020 22:57

ghost approved these changes Sep 20, 2020

View reviewed changes

src/autobattle.h Outdated Show resolved Hide resolved

mateofio force-pushed the autobattle branch 2 times, most recently from 733534a to 39a51a5 Compare September 21, 2020 13:11

mateofio force-pushed the autobattle branch 2 times, most recently from 82a3529 to 2ff9527 Compare September 23, 2020 13:17

mateofio force-pushed the autobattle branch 2 times, most recently from 2ca3656 to 90e47a4 Compare September 30, 2020 01:45

Ghabry reviewed Sep 30, 2020

View reviewed changes

ghost added the Awaiting Rebase Pull requests with conflicting files due to former merge label Oct 5, 2020

mateofio force-pushed the autobattle branch from 90e47a4 to 7ff2107 Compare October 5, 2020 13:43

mateofio mentioned this pull request Oct 5, 2020

Refactor Enemy AI #2381

Merged

5 tasks

ghost removed the Awaiting Rebase Pull requests with conflicting files due to former merge label Oct 7, 2020

Ghabry reviewed Oct 8, 2020

View reviewed changes

src/autobattle.cpp Outdated Show resolved Hide resolved

Ghabry requested changes Oct 8, 2020

View reviewed changes

mateofio added 6 commits October 8, 2020 20:24

Implement AutoBattle

e6b7b5e

Adds: 3 algorithms * RPG_RT - bug compatible with RPG_RT * ATTACK - only physical attacks + bug fixes * RPG_RT+ - RPG_RT algo + bug fixes

Add auto battle support to 2k, 2k3, and cmdline

61d1bec

Add autobattle normal attack unit test

0ebd89e

Add StringView overload for Utils::StrICmp

6d53d92

Make autobattle algo selection case insensitive

54cbb06

Fix some compiler warnings

16a8c80

mateofio force-pushed the autobattle branch from 3c3f919 to 5dea2b3 Compare October 9, 2020 00:24

mateofio added 3 commits October 8, 2020 20:27

Autobattle: Address minor review comments

3705a09

Fix off by 1 bug in CalculateSpCost()

6d56a92

When halfSpCost() modifier is used, we add 1 to flat sp cost but not percent sp cost.

AutoBattle: emulate RPG_RT sp cost bug

780949c

RPG_RT ignores half sp cost modifier

mateofio force-pushed the autobattle branch from 5dea2b3 to 780949c Compare October 9, 2020 00:28

Ghabry approved these changes Oct 9, 2020

View reviewed changes

Ghabry merged commit 798e098 into EasyRPG:master Oct 9, 2020

Conversation

mateofio commented Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

DynRPG Addresses

RPG_RT bugs ❗

Uh oh!

NovemberJoy commented Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ghabry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ghabry commented Sep 15, 2020

Uh oh!

mateofio commented Sep 15, 2020

Uh oh!

mateofio commented Sep 15, 2020

Uh oh!

Ghabry commented Sep 15, 2020

Uh oh!

CherryDT commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mateofio commented Sep 15, 2020

Uh oh!

CherryDT commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mateofio commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mateofio commented Sep 15, 2020

Uh oh!

ghost commented Sep 16, 2020

Uh oh!

mateofio commented Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Ghabry commented Sep 21, 2020

Uh oh!

mateofio commented Sep 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ghabry commented Oct 5, 2020

Uh oh!

Uh oh!

Ghabry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mateofio commented Sep 12, 2020 •

edited

Loading

NovemberJoy commented Sep 13, 2020 •

edited

Loading

CherryDT commented Sep 15, 2020 •

edited

Loading

CherryDT commented Sep 15, 2020 •

edited

Loading

mateofio commented Sep 15, 2020 •

edited

Loading

mateofio commented Sep 20, 2020 •

edited

Loading

mateofio commented Oct 9, 2020 •

edited

Loading