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 Battle2k message timings + state fixes 1 / 2 #1726

Merged
merged 17 commits into from Jun 5, 2019

Conversation

@fmatthew5876
Copy link
Contributor

commented Mar 24, 2019

This is yet another 2k battle system refactor.

In order to emulate all of the waits RPG_RT does in #1677 I've had to add more states and substates.

I've also taken this opportunity to do some cleanup. In particular I've removed GetResultMessages() from Game_BattleAlgorithm and nearly all of the 2k message processing logic into SceneBattleRpg2k.

I also fixed a self-destruct bug and #1725 using a hack that hopefully can be dismantled later.

Related: #1725
Related: #1677
Related: #1748
Related: #1770
Related: #821
Related: #1490
(they are all closed with #1785)

Tested:

  • Test all message timing cases in #1677
  • Test all weapon state cases in #1677
  • Test all skill state cases in #1677
  • Test all item state cases in #1677
  • Test all self-destruct state cases in #1677
  • Test the last state for off by 1 errors.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:battle_time branch from bd5aa3d to a70d306 Mar 24, 2019

@fdelapena fdelapena added this to the 0.6.1 milestone Mar 24, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:battle_time branch 5 times, most recently from 6ee60c4 to 352ed88 Mar 24, 2019

@fmatthew5876 fmatthew5876 changed the title Refactor Battle2k message timings Refactor Battle2k message timings + state fixes Mar 26, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:battle_time branch 5 times, most recently from e66ba7c to 47bdd2a Mar 26, 2019

@fmatthew5876 fmatthew5876 marked this pull request as ready for review Mar 26, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

This is ready now, I've run through all the test cases and they all pass.

  • There is a bug where enemies with no actions will not print messages for their states. I believe they won't heal states either. If this takes a while to merge I might fix it here, or punt it until a later PR.
@Ghabry

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Can you explain why GetResultMessages and GetResultSe were removed and GetFailureMessage and GetFailureSe were added.

How is success handled now?

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

The GetResult methods had too much 2k specific logic in them so I moved them out.

GetResultSE no longer makes sense as failure and damage sounds happen in different places.

In the damage and death commits I already had to start ripping stuff out of GetResultMessages(). So it already became partially implemented.

Success is computed in Execute(), so it shouldn't change.

Essentially, GetFailureMessage() is like getting a property of the action. Whereas GetResultMessages() is like, "do a bunch of 2k battle system message logic on this action and return it"

We could move out all the message logic from battle algorithm, but there are many others outside of failure and I haven't done that. I'm not sure we need to, as long as they stay like properties and not battle system logic.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:battle_time branch from 2c5013f to f9960f0 May 1, 2019

@Ghabry

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Fix broken states healed by SelfDestruct logic

What was broken there?

I don't really understand the cloning.
After Cloning the Actor it will still modify the same liblcf data as the original actor.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Fix broken states healed by SelfDestruct logic

What was broken there?

Self Destruct is supposed to heal states like confuse, just like a normal attack.

I don't really understand the cloning.
After Cloning the Actor it will still modify the same liblcf data as the original actor.

Ouch, that's a pretty bad oversight on my part. I will need to re-evaluate this one.

It's supposed to make a copy of everything, so that we just use AddState() to answer of the question of whether we can add the state or not.

The reason for this is that the logic involved is very complicated, mostly because of the whole thing about states being cancelled by priorities. The idea was to isolate it in one place.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

This now passes all test cases of #1770

def = Utils::Clamp(def, 1, 999);
spi = Utils::Clamp(spi, 1, 999);
agi = Utils::Clamp(agi, 1, 999);

This comment has been minimized.

Copy link
@Ghabry

Ghabry May 29, 2019

Member

Refactor scene_equip

But this change doesn't handle the case where a state is inflicted which halfes the SP, or does it?

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 May 29, 2019

Author Contributor

Half SP?

If you mean the half SP cost flag on equipment, that doesn't change the equip numbers.

If you mean half / double stats from states, this is RPG_RT compatible. The equip window doesn't show the effect of those states in the arrows, they only appear after you change equipment.

This comment has been minimized.

Copy link
@Ghabry

Ghabry May 29, 2019

Member

Yes I mean the half/double skill stuff.
The problem is that this is a RPG2k3 feature, so my educated guess is: They forgot to update the scene code. ^^'

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 May 29, 2019

Author Contributor

I could see a use for this. Imagine a cursed equipment with apparently good stats which ends up halving all your stats after you equip it and can't remove it!

@Ghabry

This comment has been minimized.

Copy link
Member

commented May 29, 2019

I ask for a split after "Fixes for physically healed conditions" because the state code exploded into 20 new commits.

fmatthew5876 added some commits Mar 9, 2019

Introduce BattleActionSubState concept
Use it for battle 2k escape logic
Refactor BattleActionState_ConditionsHeal
* Add Begin state to flash and wait 4
* Rename ConditionsHeal -> Conditions
* Remove IsFirstAttack() check, as the state machine never
  returns to Begin anyway
Fix Wait timings for Execute state
* Add wait 4 in execute
* Adds Critical state
Add Game_BattleAlgorithm::IsKilledByDamage()
Hacky, but we need it for proper message timing in 2k battles
Battle2k: Refactor Failure handling
* No target? : Apply->Finished
* Add a new failure state: Apply->Failure->Finished
* Handle failure SFX and message separately
Battle2k: Fix Death message timing
Also clean up Finished state
Battle2k: Refactor battle action results stage
* Wait 4 before each action
* Fix timing for actions
Battle2k: Refactor results messages processing
* Remove Game_BattleAlgorithm::GetResultsMessages()
* Put all the logic into ProcessActionResults
* Fix RPG_RT wait bug with death state handling
Fixes for physically healed conditions
Conditions healed by attacks can be re-infliced again by
the same weapon. Change the logic to allow this to happen
and rename `healed_conditions` to `phys_healed_conditions`
to make it more obvious what its for.
@Ghabry

This comment has been minimized.

Copy link
Member

commented May 29, 2019

thx!

@fmatthew5876 fmatthew5876 changed the title Refactor Battle2k message timings + state fixes Refactor Battle2k message timings + state fixes 1 / 2 May 29, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

FYI I haven't really tested this split here. Not sure if there would be regressions while we're in between this and #1785

@Ghabry

Ghabry approved these changes Jun 2, 2019

Copy link
Member

left a comment

Everything seems to work, couldn't find any regressions (also tested a bit in 2k3)

@carstene1ns carstene1ns merged commit 7735033 into EasyRPG:master Jun 5, 2019

7 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Wii (SDL1) Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.