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 1 of 2 #1502

Merged
merged 24 commits into from Dec 9, 2018

Conversation

Projects
None yet
5 participants
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Nov 18, 2018

Depends on: EasyRPG/liblcf#282

This PR refactors the Game_Character class heirarchy to use the RPG::SaveMapEventBase class heirarchy from liblcf.

The goals are:

  • Correct loading of fields from LSD
  • Correct saving of fields to LSD
  • Correct behavior when saving / loading between RPG_RT <-> Player.
  • Binary compatibility with RPG_RT even when data change appears not change behavior.
  • Devirtualize parts of Game_Character where not needed
  • Simplify and remove duplication of logic and state
  • Resolve #1499

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:savemapevent branch from a7b0a34 to 484adf3 Nov 18, 2018

int Game_Character::GetOpacity() const {
return opacity;
return (8 - GetTransparency()) * 32 - 1;

This comment has been minimized.

@fmatthew5876

fmatthew5876 Nov 19, 2018

Author Contributor

@CherryDT Do you agree with this calculation?

Transparency is valued from 0 to 7, with 7 being almost completely transparent. This value can't go one more step to 8 to set fully transparent.

This converts [0,7] -> [255, 31]

This comment has been minimized.

@CherryDT

CherryDT Feb 23, 2019

Almost. It is a bit weirder. It is 255 - GetTransparency() * 30. Yes, 30 and not 32.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:savemapevent branch from a4eb89f to 60349a5 Nov 19, 2018

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:savemapevent branch from 10cdb56 to bb6d129 Nov 19, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 19, 2018

Unless I find more issues to fix, I think this PR is nearly done. It's a lot of commits, so once the liblcf dependencies are in it could be broken up if needed.

Here are some save chunks I'm still unsure about:

  • map_id - For some reason RPG_RT always sets map_id = 0 for all map events. The map_id for the player and vehicles is set correctly. I'm hesitant to do this as someone could call GetMapId() on a game_event in the code expecting the right Id and getting surprised.
  • processed - This chunk is used somehow by RPG_RT to resolve movement, particularly when you have 2 events that want to move and event A wants to take event B's spot. It looks like some kind of way to not double compute event movement. How do we handle this situation in Player? At some point we should maybe also use this flag to ensure our saved games don't break in mysterious ways between RPG_RT and Player.
  • route_through - Move commands that set/unset through also set this flag. I set it for binary compatibility on LSD but I don't use it. Why does this exist? What edge case do we need to differentiate between through and route_through? If there is such a case, fixing in it Player would likely fix some esoteric event code bug in some game.
  • stop_count, max_stop_count, anim_count,remaining_step - These are now carried between RPG_RT and Player via save games. Are we frame accurate with these?
  • frame_count - We still don't write the frame count. I don't know if our frame counter is the same as RPG_RT. Does it reset to 0 when starting a new game?
@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 20, 2018

For processed, I decided to just write the flag so it looks like RPG_RT.

My observation is that processed will always be 1 in RPG_RT saves (except for vehicles you haven't seen yet). My assumption is that this is because you can never save at a time when events are resolving their moves and haven't finished processing.

Is that assumption correct?

What is SaveScreen::pan_x and SaveScreen::pan_y? We don't use them at all in Player. They look to be populated with 1/16 pixel screen coordinates in RPG_RT.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:savemapevent branch from e6150bc to 77ad7a6 Nov 21, 2018

@carstene1ns

This comment has been minimized.

Copy link
Member

carstene1ns commented Nov 29, 2018

jenkins: you know what to do, test this please 😍

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 30, 2018

@Ghabry and @carstene1ns

Would you like me to break this one up into separate PRs?

@carstene1ns

This comment has been minimized.

Copy link
Member

carstene1ns commented Nov 30, 2018

game_character.h(1100): error C2589: '(': illegal token on right side of '::'

The weird build error happens, because windows header has own macros for min() and max().
Needs this stuff here.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:savemapevent branch from c284ac7 to fcd3949 Nov 30, 2018

Show resolved Hide resolved src/utils.h Outdated
@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 1, 2018

Would you like me to break this one up into separate PRs?

@fmatthew5876
No, I think it's fine. Because is mostly code simplification because of the new inheritance feature in liblcf I think we can handle this.
Will tell you when I'm hitting something that takes longer to review, than we can split there.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:savemapevent branch from fcd3949 to ea20e63 Dec 2, 2018

Show resolved Hide resolved src/game_character.cpp Outdated
@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 3, 2018

I'm requesting a split after the commit "Remove Game_Vehicle::driving" with the commit "Add compatibility hack for old Player save games" cherry-picked. (using commit msg instead of hashes here, less chaotic)
Afterwards the corner-case "this is how RPG_RT does it" stuff begins which needs more looking and testing.

Additional 3 commits also cherry-pickable imo from more down the commit log (if possible conflict free):

  • Remove unused Game_Character::animation_id
  • Use SaveMapEventBase::anim_frame …
  • Use SaveMapEventBase::anim_paused

Only stuff I found: "Encapsulte" in one commit message.
And there are multiple doc comments missing the @param field.

Which brings this PR to 19 commits + 1 pick + 3 optional picks

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 3, 2018

Well, maybe should also give some 👍 . Nice refactor, I found this huge code duplication with virtual inheritance ugly since I added it. xD

And great to see more LSD compat problems solved.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:savemapevent branch 3 times, most recently from 1d3db32 to e9fdac1 Dec 3, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 3, 2018

I've rebased this and removed the commits after the requested break point. I've cherry-picked all the ones asked for, including Utils::Clamp() as that's needed to make this compile on windows.

fmatthew5876 added some commits Nov 19, 2018

Fix animation_type
* Use SaveMapEventBase::animation_type
* Correctly saves and loads the chunk

* WARNING: Loading old Player save games will cause all
  events to appear to be walking in place due to old liblcf bug
Expose begin_jump_x and y
Not used yet as our jump algorithm doesn't work
by recalculating itself from the beginning position
each time.
Refactor BushDepth
Move bush depth layer check up into Game_Character
Refactor Transparency/Opacity
* Use SaveMapEventBase::transparency
* Fixes saving and loading.
Set and reset player flying flag in airship
* When flying in the airship, RPG_RT sets the flying flag
  for both the player and the vehicle.
* Fixes a bug where if you loaded a save game from RPT_RT in
  the airship, the player would have the flying flag set. This
  breaks bush depth and anything else later that might depend on flying.
Use SaveMapEventBase::anim_frame
* Supports saving and loading anim_frame

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:savemapevent branch from e9fdac1 to 7871921 Dec 4, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 4, 2018

Added the @param doc comments.

@Ghabry
Copy link
Member

Ghabry left a comment

This whole refactor should be fine, but will check it once again later to ensure I didn't miss anything.

@Ghabry

Ghabry approved these changes Dec 8, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 9, 2018

This one starts to become a blocker now. For future LSD work.

Show resolved Hide resolved src/player.cpp Outdated

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:savemapevent branch from 7871921 to 48d6d9d Dec 9, 2018

@Ghabry Ghabry merged commit 1624189 into EasyRPG:master Dec 9, 2018

6 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details

@fmatthew5876 fmatthew5876 deleted the fmatthew5876:savemapevent branch Dec 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.