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: Add negative attribute modifiers and allow state infliction by skills outside of battles #2329

Merged
merged 9 commits into from Sep 23, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 10, 2020

This PR implements the negative attribute modifiers used in RPG Maker 2003 which allows for damage absorbing (e.g. dragons absorbing fire-type skills) and the possibility for skills to inflict states outside of battles (this makes the "Regenerationstrank" in "Vampires Dawn II" work outside of battles). By the way, negative attribute modifiers even allow for "funny" things like taking damage through healing skills (but you cannot get killed this way, 1 HP remain - tested with RPG_RT). This is probably my "most insane" PR so I expect many changes needed. Feedback is very appreciated!

Fix #1391

@fmatthew5876
Copy link
Contributor

I had a thought about this, let me know what you think.

I find this whole idea of GetAffectedHp() < 0 meaning hp is not affected confusing. At a minimum we should wrap this in a function call and stop doing < 0 inline everywhere.

My idea:

What if we had boolean flags for hp, sp, atk, def, spi, and agi to say whether or not the skill affects them, and then just use + or - signed numbers to indicate healing vs damage for each one. Then there is no more healing or IsPositive() flag. Such a design also allows custom battle systems to extend and add skills which can affect each parameter individually.

I've always had an idea to try this approach, but haven't yet, so I don't know for sure if it'll end up better or worse.

this->agility = std::max<int>(0, std::min<int>(effect, GetTarget()->GetAgi() - (GetTarget()->GetBaseAgi() + 1) / 2));
if (skill.affect_attack && Utils::PercentChance(to_hit)) {
if (!this->healing) {
this->attack = std::max<int>(0, std::min<int>(effect, GetTarget()->GetAtk() - (GetTarget()->GetBaseAtk() + 1) / 2));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're refactoring and testing here, this confusing min/max junk could be replaced with Utils::Clamp()

Copy link
Author

Choose a reason for hiding this comment

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

Did the replacements now.

@ghost
Copy link
Author

ghost commented Sep 11, 2020

Your idea sounds interesting. Being able to affect each parameter individually would be a nice addition for #2219. But I would like to hear what the others think about it.

@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Sep 11, 2020

Regardless, I think the overall approach you took here probably makes sense, Any further enhancement can be a later PR.

Please also test carefully the 2k battle system. It's message based state machine heavily relies on the various battle algorithm flags so any small change in behavior could break it.

@ghost
Copy link
Author

ghost commented Sep 12, 2020

Did tests with the games "Vampires Dawn" (RPG Maker 2000), "Vampires Dawn II" (RPG Maker 2003) and some custom test cases and did not encounter any regressions so far.

@fdelapena fdelapena added this to the 0.6.3 milestone Sep 12, 2020
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Sep 13, 2020

I saw this in the RE code, couldn't believe it, but then tested and confirmed it's true...

There is a massive RPG_RT bug with negative attributes. ❗

Negative attribute HP healing is doubled in it's effect!

To see this, Create a skill that does exactly 50 damage, with fire element. Create an actor with -100% fire attribute.

Now start a battle with actor at 1 hp, and have an enemy use the fire skill on the actor. 50 hp will be displayed in green pop up numbers, but actor HP will actually increase by 100!

The reason for this is because in RPG_SceneBattle::ApplyHpDamageEffect, first they call RPG_Actor::ApplyHpDamage() unconditionally with a negative value. The function is meant to handle hp damage, but because the amount is negative, it actually recovers HP. Then it starts to spawn white popup numbers with a negative value.

Then they immediately check if the damage is negative and call RPG_Actor::ApplyHpRecovery() with the value flipped. This heals the actor again, and spawns green numbers with the effect amount, immediately overwriting the previous white numbers.

The correct behavior, they should have only called ApplyHpDamage() if the damage amount is >= 0

Here is the function with this bug ...

Address Class Name
0049CFE8 RPG_SceneBattle ApplyHpDamageEffect

Also confirmed, this bug still exists in latest stream version of 2k3e

@ghost
Copy link
Author

ghost commented Sep 14, 2020

The "double strength for negative effects" bug is emulated now.

@ghost
Copy link
Author

ghost commented Sep 14, 2020

@fmatthew5876 To avoid conflicts with your PR (#2320) I made a separate branch which implements the negative attribute modifiers on top of #2320. This task is already completed and my tests worked so far. So we can wait until #2320 is merged then I will force-push my new changes into this PR.

@fmatthew5876 fmatthew5876 added the Awaiting Rebase Pull requests with conflicting files due to former merge label Sep 14, 2020
@fmatthew5876
Copy link
Contributor

You can rebase your PR now onto master, #2320 was merged. Once that's done I'll test and review this guy.

@ghost
Copy link
Author

ghost commented Sep 15, 2020

Did the rebase now, you can test and review it now.

@Ghabry Ghabry removed the Awaiting Rebase Pull requests with conflicting files due to former merge label Sep 15, 2020
this->healing = true;
// RPG_RT bug: Negative effects have double strength
effect = -effect * 2;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if it's not used in Normal, we should for consistency set the negative_effect flag here in case something later wants to rely on it.

Also this *2 to emulate the bug is not correct. The double affect happens only for HP, but not other stats. So if you want to emulate the bug you'll need to double the hp effect but not others.

Also the double bug doesn't show up in the animated damage numbers, but I'm not sure if we want to go that far ...

Copy link
Author

Choose a reason for hiding this comment

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

I don't think we should emulate the animated damage number bug for better user experience in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I agree. We'll probably get "bug reports" about the effect being double, but we'll then just have to point out it's actually RPG_RT compatible!

Copy link
Author

Choose a reason for hiding this comment

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

The double affect bug is now applied on HP only.

@@ -1312,7 +1349,7 @@ void Game_BattleAlgorithm::Skill::Apply() {
AlgorithmBase::Apply();

for (auto& sa: shift_attributes) {
GetTarget()->ShiftAttributeRate(sa, healing ? 1 : -1);
GetTarget()->ShiftAttributeRate(sa, (healing ^ negative_effect) ? 1 : -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one part where the code gets confusing, when I first read this I thought you were reversing the attribute shift but you're not because both of these flags get set at the same time.

This inline xor everywhere is super ugly. Can you please wrap this in an inline helper function with a useful name so we can know whats going on.

Btw for this stuff you can push new commits instead of rebasing if thats easier.

Copy link
Author

Choose a reason for hiding this comment

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

Moved the XOR-Operations into own functions.

this->hp = Algo::AdjustDamageForDefend(effect, *GetTarget());

// RPG_RT bug: Negative effects affect HP double
if (this->negative_effect) {
this->hp *= 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

One more edge case, the double effect bug does not happen for absorb hp. Only normal hp damage..

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

if (!this->negative_effect) {
this->sp = Utils::Clamp(GetTarget()->GetMaxSp() - GetTarget()->GetSp() + sp_cost, 0, effect);
} else {
this->sp = effect;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you still need to reduce the SP recovery by the sp_cost here?

Copy link
Author

Choose a reason for hiding this comment

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

Well, the code as is works for me. The else branch is taking SP damage from the healing skill because of the negative effect and in my tests it worked correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

No you're right. This was a mistake on my part

// FIXME: This logic always called from menu, so we set false
// to allow_battle_states
AddState(lcf::Data::states[i].ID, false);
AddState(lcf::Data::states[i].ID, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how we missed this last time. You're right that this case allows you to inflict battle only states, even from the menu.

@fmatthew5876
Copy link
Contributor

This looks good to me. In particular I really like how Skill::Execute has changed from 2 overly long branches for positive and negative, to now flatter code which has a separate branch for each effect. It's more readable.

Once you address the few minor nits remaining, I can approve this.

if (Player::IsRPG2k3()) {
this->hp = effect;
} else {
this->hp = Utils::Clamp(GetTarget()->GetMaxHp() - GetTarget()->GetHp(), 0, effect);
Copy link
Contributor

Choose a reason for hiding this comment

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

All these Clamps are written wrong, it should be (effect, 0, maxvalue)

Copy link
Author

@ghost ghost Sep 17, 2020

Choose a reason for hiding this comment

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

This is probably a dumb question but what value I'm supposed to use for maxvalue? Is it MaxDamageValue() or the maximum positive integer value? Moreover the maximum value for the clamp was effect according to the old code.

Copy link
Contributor

Choose a reason for hiding this comment

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

The maxvalue depends on the context. You just need to flip the order of the arguments here. While what you wrote technically works, it doesn't read well.

The function signature is Clamp(value, min, max). You wrote Clamp(max, min value)

So here it would be:

Clamp(effect, 0, GetTarget()->GetMaxHp() - GetTarget()->GetHp())

Copy link
Author

Choose a reason for hiding this comment

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

The order of the arguments have been corrected now.

@ghost
Copy link
Author

ghost commented Sep 17, 2020

I have added a commit which addresses two special cases which I must have overseen:

  • If the source tries to absorb HP from a target and this fails due to negative damage, the amount healed of the target is no longer restricted by the targets current HP.
  • If the target recovers SP due to negative damage, the amount recovered is no longer restricted by the targets current SP.

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.

Looks good to me now, thanks!

@ghost ghost mentioned this pull request Sep 20, 2020
2 tasks
@fmatthew5876
Copy link
Contributor

This one needs a rebase again

@fmatthew5876 fmatthew5876 added the Awaiting Rebase Pull requests with conflicting files due to former merge label Sep 23, 2020
@Ghabry
Copy link
Member

Ghabry commented Sep 23, 2020

sorry, this will be the next one that gets in :)

rueter37 added 9 commits September 23, 2020 15:34
In RPG Maker 2003 battles support for negative attribute modifiers
has been enabled, which allows damage absorbing.
Negative attribute rates are now supported outside of battles too.
!IsPositive() checks have been added on the IsAbsorb() checks to make
sure absorbing works only if the target has been damaged.
Some double code has been removed and the min/max has been
replaced Utils::Clamp now.
The RPG_RT "double strength for negative effects" bug is now
emulated.
The XOR-Operations are moved into the functions IsNegativeSkill() and
IsPositiveSkill(). These functions check if the skill is negative or
positive before applying the attribute modifiers. Moreover the double
negative effect bug is applied on HP only now.
On absorbing the RPG_RT double negative effect is not applied.
This commit fixes two bugs I came across recently:
- HP healing from a absorbed absorb due to negative effects gets no
  longer restricted by the current HP.
- SP healing due to negative effects gets no longer restricted by the
  current SP.
The order of the parameters used in the Clamp() functions have been
fixed now.
@ghost
Copy link
Author

ghost commented Sep 23, 2020

Did the rebase now.

@fmatthew5876 fmatthew5876 removed the Awaiting Rebase Pull requests with conflicting files due to former merge label Sep 23, 2020
@Ghabry Ghabry merged commit c333ce2 into EasyRPG:master Sep 23, 2020
@Ghabry
Copy link
Member

Ghabry commented Sep 23, 2020

In general I let @fmatthew5876 handle any PRs that are algorithm related. I don't really want to dive into this

@ghost ghost deleted the negative-effect branch September 23, 2020 17:11
@Ghabry Ghabry linked an issue Sep 23, 2020 that may be closed by this pull request
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.

[RM2kE Bug] Element with E rate negative % bug
3 participants