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 2 / 2 #1785

Merged
merged 23 commits into from Jun 9, 2019

Conversation

@fmatthew5876
Copy link
Contributor

commented May 29, 2019

Depends on #1726

Fix: #1725
Fix: #1677
Fix: #1748
Fix: #1770
Fix: #821
Fix: #1490

@Ghabry Ghabry added this to the 0.6.1 milestone May 29, 2019

@Ghabry Ghabry added the Battle label May 29, 2019

@fmatthew5876 fmatthew5876 referenced this pull request May 29, 2019
6 of 6 tasks complete
@Ghabry

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

Good test:
Wadanohara http://vgperson.com/games/wadanohara.htm

Save04.zip

The 2nd char in the party has a skill called flock which inflicts up to 9 states.

Here is a save earlier in the dialog which works in RPG_RT:

Save02.zip

@Ghabry

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Review note: The first commit here is "Fixes for physically healed condition"

@@ -740,8 +740,6 @@ bool Scene_Battle_Rpg2k3::ProcessBattleAction(Game_BattleAlgorithm::AlgorithmBas
action->IsPositive() ? Font::ColorHeal : Font::ColorDefault,
std::to_string(action->GetAffectedHp()));
}

target->BattlePhysicalStateHeal(action->GetPhysicalDamageRate());

This comment has been minimized.

Copy link
@Ghabry

Ghabry Jun 5, 2019

Member

Refactor BattlePhysicalStateHeal

"Remove call from battle 2k3 that did nothing"

Did this break now autostatehealing in 2k3 or where is this handled?

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Jun 6, 2019

Author Contributor

This method returns a vector of conditions to heal from a physical attack. I have no idea why it's here in 2k3 battle, as it just computes this vector and does nothing with it.

Physical states healing is all setup properly in Game_BattleAlgorithm.

Auto state healing is a different concern. I haven't checked where that is in 2k3, but this change doesn't affect that either way.

src/state.cpp Show resolved Hide resolved
src/state.cpp Outdated Show resolved Hide resolved
@@ -78,13 +78,13 @@ static inline int ToHitPhysical(Game_Battler *source, Game_Battler *target, int
return to_hit;
}

static void BattlePhysicalStateHeal(int physical_rate, std::vector<int16_t>& target_states, std::vector<Game_BattleAlgorithm::StateEffect>& states) {
static void BattlePhysicalStateHeal(int physical_rate, std::vector<int16_t>& target_states, const PermanentStates& ps, std::vector<Game_BattleAlgorithm::StateEffect>& states) {

This comment has been minimized.

Copy link
@Ghabry

Ghabry Jun 5, 2019

Member

Add PermanentStates concept

(Flagging the whole commit). Is there any sanity check for PermanentStates? What happens when a weapon inflicts a state that is out-of-bounds?
Looks filtered in next commit "Support equipment inflicting states "?

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Jun 6, 2019

Author Contributor

The warnings will show up when the equipment tries to add the states.

I added a new commit to prevent adding these invalid states to the PermanentStates object.

src/game_battler.cpp Outdated Show resolved Hide resolved

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:battle_time2 branch 2 times, most recently from 10fc820 to 77b4da6 Jun 6, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Rebased and addressed review comments

@Ghabry

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

@fmatthew5876
I noticed in RPG_RT that "Full Recovery" inflicts battle equipment states even on the map.
Player doesn't do this and I consider the RPG_RT behaviour a bug, your opinion?

@Ghabry

Ghabry approved these changes Jun 8, 2019

Copy link
Member

left a comment

What I tested:

  • (Battle) equip state infliction map/battle
  • Add/Remove/Full Recovery event commands
  • Skills that inflict states
  • Revive skill items

Seems to work.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

@fmatthew5876
I noticed in RPG_RT that "Full Recovery" inflicts battle equipment states even on the map.
Player doesn't do this and I consider the RPG_RT behaviour a bug, your opinion?

I guess we need to emualte this for completeness? If I get around to it, I'll add it in another PR. Don't want to delay this one for such an edge case.

@Ghabry

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Also happens when using the Inn.
Nice so I go to bed and wake up with Poison and Silence.
Not sure if that should be even emulated, to get rid of these bogus states you can just reequip the equipment in the equipment scene... Because they are battle states they are not reeinflicted :)

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

Well technically the effect should be benign, since they are battle states they aren't supposed to affect the map. For example, the editor won't let you adding stepping effects to a battle only poison effect. Once you start a battle, you'll get them regardless.

I'm in favor of emulating this. The feature in general is pretty broken, but maybe some game is relying on these edge case behaviors?

@Ghabry

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

=================================================================
==16125==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000064 (pc 0x560fc702240d bp 0x7ffeaa073dd0 sp 0x7ffeaa073a00 T0)
==16125==The signal is caused by a READ memory access.
==16125==Hint: address points to the zero page.
    #0 0x560fc702240c in Scene_Equip::UpdateStatusWindow() /home/gabriel/Programmierung/easyrpg/easyrpg-player/src/scene_equip.cpp
    #1 0x560fc7021804 in Scene_Equip::Update() /home/gabriel/Programmierung/easyrpg/easyrpg-player/src/scene_equip.cpp:62:2
    #2 0x560fc6ba0ec2 in Player::Update(bool) /home/gabriel/Programmierung/easyrpg/easyrpg-player/src/player.cpp:309:21
    #3 0x560fc6befd66 in Scene::MainFunction() /home/gabriel/Programmierung/easyrpg/easyrpg-player/src/scene.cpp:119:4
    #4 0x560fc6ba04b4 in Player::MainLoop() /home/gabriel/Programmierung/easyrpg/easyrpg-player/src/player.cpp:225:19
    #5 0x560fc6ba0222 in Player::Run() /home/gabriel/Programmierung/easyrpg/easyrpg-player/src/player.cpp:219:3
    #6 0x560fc6b9c996 in main /home/gabriel/Programmierung/easyrpg/easyrpg-player/src/main.cpp:28:2
    #7 0x7f71ec73ace2 in __libc_start_main (/usr/lib/libc.so.6+0x23ce2)
    #8 0x560fc6a8402d in _start (/tmp/easyrpg/cmake-debug/easyrpg-player+0x1d802d)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/gabriel/Programmierung/easyrpg/easyrpg-player/src/scene_equip.cpp in Scene_Equip::UpdateStatusWindow()
==16125==ABORTING

No idea if this is a regression caused by this PR, probably not.

Have one equippable item in the inventory. Scroll the cursor to the right (on the empty unqeuip slot) and it crashes when highlighted.

fmatthew5876 added some commits Mar 25, 2019

Refactor states handling
States are infliced and healed in the order they happen. In order
to faithfully replicate this, in Execute() we make a copy of our
target and then try to add and remove the states. We keep a
record of what we did, then do it again for real in Apply().
Finally, we replay this record once more to print the messages
in 2k battle.

* Implements the skill reverse_state_effect chunk from #821
* Implements physical skills healing states like normal attacks.

This solution is really bad, but so far there appears to be
nothing better without a major refactor of how we process
battle aglorithms.

fmatthew5876 added some commits Mar 31, 2019

Battle: Fix screen effects timing
Battler sprites shake position was off by 1 frame. To fix this,
we move the screen update to happen after the interpreter runs but
before we update battlers.

Fix #1735
Optimize HasState and GetInflictedStates
* Avoid allocating temp in HasState()
* Avoid excess virtual calls in GetInflictedStates()
Refactor BattlePhysicalStateHeal
* This is algo logic, so move into game_battlealgorithm
* Optimize a bit (no vector copy)
* Remove call from battle 2k3 that did nothing
Game_Battler: use State functions for state manipulation
Also remove virtuals where not needed
Remove Game_Battler::Clone()
Use State methods on a copy of states instead
Add PermanentStates concept
Allows for battlers to have states which can never be
removed.
Refactor scene_equip
Don't actually equip stuff when displaying numbers, just
compute the numbers and show them.
AddState - add allow_battle_states
* Some contexts don't allow adding battle states
* Equipment inflict states doesn't add battle states on map
* Adds equipment states when battle starts
RemoveState event commands remove battle equip states
RemoveState commands on the map will always remove
battle states, even if equipment inflicts such a state
Battle: fix skill healing vs equip states
Skills which heal a state that are prevented by
equipment don't miss. They just do nothing.
Fix for revive skill items
Items which trigger revive skills did not work
from the menu
Fixes for revive skills
* Percent style: Should be effect / 100, not /10
* Apply hp after heal state
@Ghabry

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

I think this is caused by this PR, is in new code and the problem is current_item->two_handed. current_item is NULL.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:battle_time2 branch from 77b4da6 to 05894cb Jun 8, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

Nice catch, added a fix and rebased

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

Added the full recover and inn equipment states stuff.

Apply battle equipment states after full recover and inn
Emulates RPG_RT behavior with equipment states.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:battle_time2 branch from ad770c2 to 320e647 Jun 8, 2019

@carstene1ns
Copy link
Member

left a comment

LGTM

@carstene1ns carstene1ns merged commit 1b6c0d1 into EasyRPG:master Jun 9, 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.