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

Battle 2k3: Implement battle row damage and accuracy modifiers #2321

Merged
merged 1 commit into from Sep 10, 2020
Merged

Battle 2k3: Implement battle row damage and accuracy modifiers #2321

merged 1 commit into from Sep 10, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 5, 2020

This PR implements the damage and accuracy modifiers for normal attacks which depend on the battle condition and the row of the actors in RPG Maker 2003.

effect = 125 * effect / 100;
}
}
if (Game_Battle::GetBattleCondition() == lcf::rpg::System::BattleCondition_surround) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot the back/pincer back row case

Copy link
Author

@ghost ghost Sep 5, 2020

Choose a reason for hiding this comment

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

This is because the pincer battle condition doesn't do any changes at all. And in the back battle condition I noticed in my tests with RPG_RT that there are no positive damage modifiers applied. More details about the mentioned things in my comments in #680.

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed, you're right about pincer. But this is still missing back attack case.

Copy link
Author

Choose a reason for hiding this comment

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

Whoops, after doing some retests you are right about the back attack case. I will fix this in the next commit.

@ghost
Copy link
Author

ghost commented Sep 5, 2020

Added a commit which fixes the handling of back battle conditions.

@@ -1031,7 +1036,7 @@ bool Game_BattleAlgorithm::Normal::Execute() {
}
}
if (Game_Battle::GetBattleCondition() == lcf::rpg::System::BattleCondition_back) {
if (source->GetType() == Game_Battler::Type_Ally && static_cast<Game_Actor*>(source)->GetBattleRow() == Game_Actor::RowType::RowType_front || target->GetType() == Game_Battler::Type_Ally && static_cast<Game_Actor*>(target)->GetBattleRow() == Game_Actor::RowType::RowType_front) {
if (target->GetType() == Game_Battler::Type_Enemy || target->GetType() == Game_Battler::Type_Ally && static_cast<Game_Actor*>(target)->GetBattleRow() == Game_Actor::RowType::RowType_front) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This Type_Enemy thing looks like a bug? Also lets smash all this into one commit so it's easier to review

Copy link
Author

@ghost ghost Sep 5, 2020

Choose a reason for hiding this comment

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

The back attack condition is a special case. If the player attacks an enemy the damage is always reduced by 25% regardless of row. If the player is in the back row he gets the 25% damage bonus but it still means that the damage is reduced by 6.25%. That were the results from my tests with RPG_RT.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. I checked again.

As mentioned in the issue, there are 2 normal attack code paths. One appears to be taken for normal, initiative, and surround. The other for back and pincer.

The first code path only does row adjustements on the attacker / defender if it's an actor. The second one does not.

Enemies are always in the front row. So for back you always miss more often and hit for less. I need to verify this more, it looks like RPG_RT bugs because this behavior does not make a lot of sense.

Also we need to verify when actors attack other attacks via confuse status.

Copy link
Author

Choose a reason for hiding this comment

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

I have tested the confused status cases and in this cases the modifiers are applied as well. I only had to check in back attack condition if source and target is an ally because the 25% damage bonus is not applied if an ally attacks an other ally or himself due to confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also lets put parens around the second condition to make the logic more clear

target->GetType() == Game_Battler::Type_Enemy || (target->GetType() == Game_Battler::Type_Ally && static_cast<Game_Actor*>(target)->GetBattleRow() == Game_Actor::RowType::RowType_front)

Copy link
Author

Choose a reason for hiding this comment

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

Parenthesis have been added.

@ghost
Copy link
Author

ghost commented Sep 5, 2020

Did the squash as requested.

@ghost
Copy link
Author

ghost commented Sep 5, 2020

For handling the confused status a change in the back attack condition was necessary. I assume now you want everything in one commit so I amended the commit and force-pushed the changes.

@ghost
Copy link
Author

ghost commented Sep 5, 2020

Sorry, my bad. Had to force-push it again because I forgot the add the changed file before the commit amending.

The damage and accuracy modifiers for normal attack which depend on
the battle condition and the row of the actors have been implemented
now.
@ghost
Copy link
Author

ghost commented Sep 5, 2020

And once again a force push because I forgot the second pair of parenthesis... I really must stop that because permanent force pushes are considered bad practice.

Copy link
Contributor

@fmatthew5876 fmatthew5876 left a comment

Choose a reason for hiding this comment

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

This looks good to me now. I'm working on a comprehensive refactor of Normal Attack to support multiple weapons as well as auto battle in #2320

Once we have that, we should do comprehensive testing on the new code end to end to try to achieve perfection

@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Sep 7, 2020

Open question:

This back attack always having an accuracy and damage penalty regardless of row is definitely an RPG_RT bug in my opinion. Should we implement it to be compatible with any games maybe relying on this to tune difficulty? or fix it?

In SceneBattle::ApplyNormalAttack all 3 row logics (target evasion, source atk bonus, target def bonus) are surrounded by a query that checks whether the affected source or target battler is an actor.

In SceneBattle::ApplyWeaponNormalAttack the row logic is the same, but the actor checks are missing from all of them.

The IsActor() check is a virtual function call. My hypothesis is someone removed the actor checks (maybe for speed?) because the second function is only used when the source is already an actor. I think they removed this but accidentally forgot that the target can still be a monster.

Outside of that, the logic also doesn't make any sense.

@ghost
Copy link
Author

ghost commented Sep 7, 2020

I think we should go for the RPG_RT route regarding the back attack condition for compatibility sake. But your suggestion to fix this behavior would be a nice optional feature for #2219.

@fmatthew5876
Copy link
Contributor

Considering that some games may have tuned their difficulty mechanics to account for this behavior, I have to agree we should keep it, at least for now.

@Ghabry
Copy link
Member

Ghabry commented Sep 9, 2020

Is the RPG_RT compatible nonsense-back-attack implemented or not? I'm also in favor of implementing it for compat.

Is Back Attack the same as Surprise Attack (thats the official English term)? https://twitter.com/EasyRPG/status/1282987664636088320/photo/1

btw @rueter37 no need to apologize for force pushes. They are only bad style when more than one person works on the branch.

@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Sep 9, 2020

I don't like the 2k3e terminology, it's confusing. The names we use in lcf are perfect.

none/init = default
back = surprise - this PR implements the RPG_RT compatible bug
surround = preemptive (wtf is this name??)
pincers = pincers

@ghost
Copy link
Author

ghost commented Sep 9, 2020

I went for the RPG_RT compatible back attack condition.

@Ghabry Ghabry merged commit 57f6c63 into EasyRPG:master Sep 10, 2020
@fmatthew5876
Copy link
Contributor

I noticed here that the back attack bug should only be triggered when the source is an actor and the target is an enemy. If enemy attacks enemy in back attack, the bug should not occur.

This is already fixed in #2320 and is a super rare edge case, so we can wait for that to get it addressed.

@ghost ghost deleted the battlerow-modifiers branch September 10, 2020 18:51
@fmatthew5876 fmatthew5876 added this to the 0.6.3 milestone Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants