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 Game_Character classes + add move route test suite #2208

Merged
merged 96 commits into from
Sep 21, 2020

Conversation

fmatthew5876
Copy link
Contributor

@fmatthew5876 fmatthew5876 commented May 11, 2020

This is a large refactor for the Game_Character class heirarchy.

Depends on: EasyRPG/liblcf#372

Goals

  • Remove use of Main_Data:;game_data from characters and map Remove global save data #1954
  • Restructure Game_Character to match RPG_RT design more closely. This will fix bugs we don't even know about yet, make it easier to fix all of the current bug set, and make it very easy to fix bugs discovered later.
  • Fix remaining incompatibilities with RPG_RT w.r.t characters and move routes
  • Simplify code and remove global variables

Issues Fixed

Unit Test coverage:

Additional Incompatibilities discovered and fixed

  • CommandHeroTranspareny resets the through flag
  • Verify stop animation flag is ignored for spin
  • through flag correctly used
  • Several additional cases when menu_calling and encounter_calling are cleared
  • airship will trigger collision events but not touch events. And only on Game_Event's update sequence.
  • GetOnOffVehicle replaces diagonal directions with sprite_direction
  • Fix incorrect timing of spin with speed 2
  • Post-boarding sets phasing = hidden || slipThrough ??

Testing Plan

  • Add a unit test suite
  • Test a list of unique edge cases for this PR
  • Retest the issue list
  • A code reviewer should independently review this PR against RPG_RT in an RE tool.
  • Play a few games like a normal user would

Issue Retest List

Lcf Chunk Renames to follow

  • move_route.repeated -> done
  • sprite_direction -> facing
  • through vs route_through -> something?
  • sprite_transparent -> sprite_hidden

Dynrpg Info

Vtable and Structure layouts:
https://github.com/fmatthew5876/DynRPG/blob/master/Character.h

Address Class Symbol
004C3D60 Character VTABLE
004A89A0 Hero VTABLE
004B3BF4 Vehicle VTABLE
004AAC8C Event VTABLE
004A35D0 SceneMap Update()
004AAF50 SceneMap RefreshAllPages()
004B1938 Interpreter OnFinishEvent()
004AEBD4 Interpreter CommandTeleport()
004AEF30 Interpreter CommandSetEventLocation()
004AFBF0 Interpreter CommandSetMoveRoute()
004AF09C Interpreter CommandSwapEventLocation()
004AD618 Interpreter CommandChangeParty()
004AF610 Interpreter CommandPanScreen()

Notes to self

  • Vehicles pending move on load / map change
  • Visible flag only used by hero, we use it in other places
  • CommandHeroTranspareny resets the through flag
  • Embarking a vehicle resets the through - test this?
  • Teleport / vehicle location / event location commands - review direction and facing
  • move route on inactive event
  • Check command halt all movement
  • Collision code sets menu calling
  • Get on/off vehicle not blocked by boarding or unboarding?
  • Check change party

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented May 13, 2020

Address Class Name
004C1150 Battler ShouldMirrorBattleAnimations() - not used in this PR, but shows how battle aninimation mirroring depends on battler mirrored flag
??? SceneBattle Logic which makes the character turn only if they perform an action on a single enemy - I need to find this again
004977A4 SceneBattle InitBattle() - initializes the battle and calls all the below routines
004B4910 SceneBattle InitBattleActors() - does actor placement
004BE8E0 SceneBattle InitBattleMonsters() - filters encounter condition and does enemy placement
004B4B6C SceneBattle ResetPartyDirections() - Resets mirrored flag for all party at end of battle init sequence
004BED2C SceneBattle ResetMonsterDirections() - Resets mirrored flag for all monster at end of battle init sequence

This is rebased but the rebase was a little nasty. I need to retest again.

This was referenced May 14, 2020
@fmatthew5876 fmatthew5876 force-pushed the char_refactor branch 2 times, most recently from 5ab56ec to d95f748 Compare May 23, 2020 18:52
@fmatthew5876 fmatthew5876 force-pushed the char_refactor branch 2 times, most recently from 8b8fda7 to f902c68 Compare May 25, 2020 08:04
@fmatthew5876 fmatthew5876 changed the title Refactor Game_Character classes Refactor Game_Character classes + add move route test suite May 25, 2020
@fmatthew5876 fmatthew5876 force-pushed the char_refactor branch 2 times, most recently from 4566935 to 558696a Compare May 26, 2020 01:11
@Ghabry
Copy link
Member

Ghabry commented May 26, 2020

btw you can use REQUIRE with ==, >, <, ... instead of REQUIRE_EQ/_GT. It will still show the evaluated values when the test fails.

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented May 27, 2020

Interesting jump edge case:

Move Route: UpRight, Forward, Forward
Result: UpRight, UpRight, UpRight

Move Route: UpRight, Jump, Foward, EndJump, Forward
Result: UpRight, UpRight Jump, Up

So when a jump lands the UpRight direction gets replaced by Up

@fmatthew5876 fmatthew5876 force-pushed the char_refactor branch 4 times, most recently from ba5f6ab to f597451 Compare May 30, 2020 20:00
@fdelapena fdelapena added the Has PR Dependencies This PR depends on another PR label Jun 9, 2020
@fmatthew5876 fmatthew5876 removed the Has PR Dependencies This PR depends on another PR label Jun 14, 2020
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.

Let's get the counter close to 200

@Ghabry Ghabry removed the Needs feedback Waiting for the issue author to give further information. label Sep 21, 2020
@Ghabry Ghabry merged commit e1ea5a9 into EasyRPG:master Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment