Skip to content
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

Refactor Battle Algorithm effect and hit rate calculations #2320

Merged
merged 31 commits into from
Sep 14, 2020

Conversation

fmatthew5876
Copy link
Contributor

@fmatthew5876 fmatthew5876 commented Sep 5, 2020

This PR moves battle algo calculation logic into it's own namespace as a set of standalone free functions with minimal dependencies.

Goals:

  • Migrate battle algorithm logic into separate free functions that can be called from multiple contexts such as: battle, menu, events, auto battle, and unit tests
  • Unit tests around all edge cases in battle algos
  • Allow implementation of auto battle in a follow up PR
  • Restructure the code to be able to have confidence and converge on algo correctness
  • Support weapon modes, so we can implement correct dual wield in 2k3
  • Eliminate Main_Data::game_data.actors Remove global save data #1954

This also implements 2k3 negative attributes using logic taken from RE. It implements calculating a negative effect, but does not yet pass it through the battle system.

TODO:

  • Write unit tests for all edge cases
  • Verify each unit test by testing in RPG_RT
  • Implement weapon modes for 2k3 dual wield
  • Verify differences between actor / monster normal attack in RPG_RT including row
  • Recheck for differences in menu vs battle skills
  • Verify behavior in 2k3 dynrpg
  • Verify behavior in 2ke
  • Verify behavior in 2k legacy
  • Verify behavior in 2k3 legacy

DynRPG Addresses:

address class function Comments
004B71AC Actor GetBaseAtk level + mod + all equipment
004B7408 Actor GetBaseDef level + mod + all equipment
004B74B8 Actor GetBaseInt level + mod + all equipment
004B7568 Actor GetBaseAgi level + mod + all equipment
004BFB28 Battler GetAtk base + battle_mod + states
004BFBF4 Battler GetDef base + battle_mod + states
004BFCC0 Battler GetInt base + battle_mod + states
004BFD5C Battler GetAgi base + battle_mod + states
004B760C Actor GetBaseAgiForUsedWeapon level + mod + used weapon + all armor + battle mod (not states ❗ )
004B7250 Actor GetBaseAtkForUsedWeapon level + mod + used weapon + all armor + battle mode + additional clamp(1, 9999) ❗
004B9834 SceneBattle ApplyNormalAttack Always used by monster, sometimes used by actor - has states inflict bug for actor attacks
0049B048 SceneBattle ApplyNormalWeaponAttack Used only for actors attack
004C0B2C Battler CalcNormalAttackDamage Always used by monster attack, sometimes used by actor attack. For actors, always uses combined atk and agi from both weapons. Used for actor auto battle ranking.
004B9834 Actor CalcNormalWeaponAttackDamage Used only for actors attack - has back attack row bug not present in other version
0049C644 SceneBattle ApplySkillEffect
004A20D8 SceneMenu ApplySkillEffect
004C0DBC Battler ComputeSkillPower Used for actor auto battle ranking
0049B7E4 SceneBattle ProcessActionSelfDestruct

List of RPG_RT bugs discovered ❗

  • When the battle condition is "back" all actor attacks against enemies are treated as if they were in the back row.
  • When actor attack uses ApplyNormalAttack path, state infliction percentages are taken as the max rate of both weapons, instead of the just the weapon which is performing the attack.
  • When actor uses a normal attack with a weapon, the actor's AGI used to calculate the hit probability ignores states which can double or half AGI.
  • In 2k3, RPG_RT will still check if failure_message = 3 and run physical evasion checks. There is no way to set this flags anymore in 2k3 editor. What about skills ported from 2k?
  • In physical skill evasion checks, 2k3 will not check for states with avoid_attacks or row.
  • When calculating the rank for skill use in auto battle, RPG_RT highly values skills which can revive dead allies. However RPG_RT does not check invert_state flag, and so it would prefer to use a skill which inflicts death on the party.
  • 2k3 protects against divide by zero in the normal attack hit rate agi calculation. 2k does not. However in both cases, the agi value used is already clamped to >= 1..

@fmatthew5876 fmatthew5876 marked this pull request as draft September 5, 2020 06:59
@fmatthew5876 fmatthew5876 changed the title Refactor Attributes Refactor Attributes + Normal Attack Sep 5, 2020
@fmatthew5876 fmatthew5876 changed the title Refactor Attributes + Normal Attack Refactor Battle Algorithm calculations Sep 5, 2020
@fmatthew5876 fmatthew5876 changed the title Refactor Battle Algorithm calculations Refactor Battle Algorithm effect and hit rate calculations Sep 5, 2020
@fmatthew5876 fmatthew5876 force-pushed the attribute branch 2 times, most recently from 1d0137e to 5d64f75 Compare September 6, 2020 04:31
@fmatthew5876 fmatthew5876 force-pushed the attribute branch 3 times, most recently from 13998a3 to 99b47ad Compare September 8, 2020 03:14
@fmatthew5876 fmatthew5876 marked this pull request as ready for review September 8, 2020 03:15
@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Sep 8, 2020

This is now ready for review and testing.

I've written a comprehensive unit test suite for everything here.

This PR needs to be tested, in particular:

  • Reproduce each tests/algo.cpp unit test case in RPG_RT and verify the numbers match.
  • Do some basic testing of games
  • Verify unit test coverage is adequate

If any of you guys have time, I could really use some help testing this stuff. Once we get this locked down, our core battle algorithm numbers will be correct and stable forever.

@ghost
Copy link

ghost commented Sep 9, 2020

I would like to try out your PR. But I have a probably stupid question: How to build the tests? Building and running the Player itself works but the tests are not getting built. Maybe I'm missing something obvious but I'm a bit lost here. Any help would be appreciated!

@Ghabry
Copy link
Member

Ghabry commented Sep 9, 2020

For running the tests you must use the check target (that's a convention invented by autotools, so many open source projects follow it):

  • When using CMake: cmake --build . --target check
  • When using autotools (configure) or the CMake "Makefile" generator: make check

@ghost
Copy link

ghost commented Sep 9, 2020

Perfect, thank you! Using --target check worked for me.

@fmatthew5876
Copy link
Contributor Author

Added a "fix" for the row "bug"

Also refactored a bit to improve the code and added to all the tests.

Confirmed in 2k legacy, 2k updated, and 2ke
@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Sep 11, 2020

I checked the battle algorithms from this PR against 2k legacy, 2k updated, 2ke, 2k3 legacy, and 2k3 dynrpg.

I discovered the battle adjusted stats can go up to 9999 in all versions of 2k. Other than that, everything appears the same except the legacy variance adjustment discovered by @rueter37 and already implemented here.

All that's left to do now is some in game testing of the unit test cases to verify everything.

@fmatthew5876 fmatthew5876 requested a review from a user September 11, 2020 05:00
@fmatthew5876
Copy link
Contributor Author

I verified the unit test cases in RPG_RT. I did normal attacks with the various row configurations and use a debugger to get the damage value before variant.

I tested 0 variance skills.

The numbers match up.

So this PR is done.

@Ghabry
Copy link
Member

Ghabry commented Sep 13, 2020

Minor: You forgot our shiny header in some new files. (does not apply to files in tests/ where we always omit it).

Won't be able to look through this before the next weekend. This is just tooooo much and git diffs is of no help because most of the changes are "take everything from battle algorithm and paste it with changes somewhere else". :(

I reaaaaaaaaaaaaaaally hope that there will be never any reason to change the Api ever again in the classes where you added 3000 lines of unit tests. When somebody touches them they have to modify at least 100 unit tests. (or give up out of frustration)

@fmatthew5876
Copy link
Contributor Author

I reaaaaaaaaaaaaaaally hope that there will be never any reason to change the Api ever again in the classes where you added 3000 lines of unit tests. When somebody touches them they have to modify at least 100 unit tests. (or give up out of frustration)

Well that's kind of the point. If you want to refactor the API, you have to make sure your code change passes all the tests. This gives us more confidence in refactors than any manual testing can.

That being said, I don't see too much refactoring in the future for these interfaces

Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

src/attribute.h Outdated Show resolved Hide resolved
src/attribute.cpp Outdated Show resolved Hide resolved
src/algo.cpp Outdated

bool IsRowAdjusted(const Game_Actor& actor, lcf::rpg::System::BattleCondition cond, bool offense) {
return (cond == lcf::rpg::System::BattleCondition_surround
|| (actor.GetBattleRow() == (1 - offense)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Algo library and port normal attack calculations to it

(1 - offense). That's a bool, how about actor.GetBattleRow() != offense? Or whatever the intention is (have unit tests ;)). Is hard to read.

int CalcSkillToHit(const Game_Battler& source, const Game_Battler& target, const lcf::rpg::Skill& skill) {
auto to_hit = skill.hit;

if (skill.failure_message != 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Algo library and port normal attack calculations to it

Could you flag this as a RPG2k3 bug? (the skill message can't be modified)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this doesn't apply anymore. Saw this in a later commit commented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added later, but it should go above here to make it easier to read.

src/algo.cpp Outdated
}

int CalcCriticalHitChance(const Game_Battler& source, const Game_Battler& target) {
// FIXME: Make this function return int 0 to 100.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Algo library and port normal attack calculations to it

Can you explain this fixme?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got rid of it later

src/game_actor.h Outdated
int GetHitChance() const override;
float GetCriticalHitChance() const override;
int GetHitChance(int weapon = kWeaponAll) const override;
float GetCriticalHitChance(int weapon = kWeaponAll) const override;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow computing stats as if single weapon equipped

Sorry, but for all of them the doc comment was not updated.

From GetBaseAtk to AttackIgnoresEvasion

Also in Game_battler.h and game_enemy.h

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

if (state.affect_attack) {
n = AffectParameter(state.affect_type, base_atk);
break;
static int AdjustParam(int base, int mod, int maxval, Span<const int16_t> states, bool lcf::rpg::State::*adj) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor GetAtk() etc..

What is this bool lcf::rpg::State::*adj? Looks like a Pointer to State::... what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a pointer to data member, points to State::affect_atack or State::affect_defense etc.. Essentially lets me write one function for all 4 parameters without needing templates.

for (auto i: states) {
const auto* state = lcf::ReaderUtil::GetElement(lcf::Data::states, i);
assert(state);
if (state->*adj) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor GetAtk() etc..

Or here. state has no field adj. I'm super confused.

@@ -32,26 +32,24 @@ Game_Actors::Game_Actors() {
Game_Actors::~Game_Actors() {
}

void Game_Actors::Fixup() {
void Game_Actors::SetSaveData(std::vector<lcf::rpg::SaveActor> save) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove Main_Data::game_data.actors

Intential copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, internally we move the SaveActor objects.

The new model for loading save data will be that you move it.

@@ -898,4 +906,8 @@ inline void Game_Battler::SetDirectionFlipped(bool flip) {
direction_flipped = flip;
}

inline int Game_Battler::GetBaseAttributeRate(int attribute_id) const {
return 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor Battler attributes

Does this have a use? It is overoaded for both actor and enemy.
Also needs a // C - default comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this function

}
auto& source = static_cast<const Game_Actor&>(source_battler);

std::array<const lcf::DBBitArray*, 2> attribute_sets;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clang tidy complaints that you access uninitialized memory. Though this looks safe (maybe initialize it to make it happy?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added initializer

data.resize(lcf::Data::actors.size());
for (size_t i = 1; i <= data.size(); i++)
GetActor(i)->Init();
Game_Actors::~Game_Actors() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is now empty. Can be defaulted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@Ghabry
Copy link
Member

Ghabry commented Sep 14, 2020

Your refactoring finally solved the "Invalid actor" warnings on shutdown printed into the logfile 👍 .

src/algo.h Outdated
#include <lcf/rpg/fwd.h>
#include <lcf/rpg/system.h>
#include <lcf/rpg/saveactor.h>
#include <game_battler.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<> -> ""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@Ghabry
Copy link
Member

Ghabry commented Sep 14, 2020

There are now virtual function calls in the constructors:
Afaik this is not allowed? But somehow it works (last time I did this in MSVC I got a "pure virtual called"?! Was this changed in a later standard?

Actor:

if (GetLevel() > 0) {
	SetHp(GetMaxHp()); <- this
	SetSp(GetMaxSp()); <- this
	SetExp(exp_list[GetLevel() - 1]);
}

Enemy

hp = GetMaxHp();
sp = GetMaxSp();

Game_Battler:

ResetBattle(); <- Maybe make non-virtual. Isn't overloaded.

@Ghabry
Copy link
Member

Ghabry commented Sep 14, 2020

Also non-standard it seems: (but I let you decide)

On GetBaseAtk etc. "Clang-Tidy: Default arguments on virtual or override methods are prohibited"

@Ghabry Ghabry merged commit 1953a7f into EasyRPG:master Sep 14, 2020
@fmatthew5876 fmatthew5876 deleted the attribute branch September 14, 2020 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants