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

SaveGame Fixes #277

Merged
merged 13 commits into from Nov 5, 2018

Conversation

Projects
None yet
5 participants
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Nov 2, 2018

Depends on #268

This PR is attempting to fix how LSD files are written from Player. A lot of this includes proper sizing of arrays and clearing out chunks which have the same value as in the LDB / LMU.

The way I'm testing this is:

  1. Create a very simple test game
  2. Start New Game and Save in RPG_RT.exe
  3. Start New Game and Save in Player
  4. Binary compare the resulting lsd files and track down differences.
  5. Start adding things to test game, and repeat the process.

Most of this work will end up in liblcf, but some changes are needed in Player too. The corresponding Player PR is EasyRPG/Player#1470

More work is needed here, but already I've come across some serious bugs in how we use save files and some unknown fields.

@CherryDT

What are these fields?

  • SaveSystem::screen 0x1
  • SavePartyLocation::stop_count 0x34

One very interesting problem is Save::pictures. For a new game, this array needs to be resized to exactly the number of a picture that this particular version of RPG_RT.exe supports.

Number of pictures changed quite a lot. I don't know all of the different versions and what their picture counts were.

Does anyone know:

  • What versions of rm2k and rm2k3 had picture count changes?
  • Can we accurately detect all of those versions?

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:lsd_defaults branch 3 times, most recently from f63131b to 077bcab Nov 2, 2018

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 2, 2018

Not Cherry but I can guess most :)

screen is the ID of the screen where you are currently. Doesn't matter at all for loading afaik, therefore would simply write "Map" there, see: http://rpg-maker.cherrytree.at/dynrpg/namespace_r_p_g.html#a2385fd16f4ad88a4166fc9d04255a88c

stop_count is the "stop_count" of Main_Data::game_player

anim_count is the animation frame of Main_Data::game_player

panorama_data does not export any savable chunks and is unused.

frame_count: Probably frames since startup? Don't think it's important to get this right.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 2, 2018

Is the picture count really relevant? RPG_RT doesn't really care what you put there, also there are patches which adjust the amount of pictures, is not really possible to guess the versions.
Imo it is good enough when RPG_RT can load the save correctly instead of adding complexity for corner cases just because of "RPG_RT does it this way" (for all other LCF formats I agree with this, but LSD?)

void RPG::SaveSystem::UnFixup() {
const RPG::System& system = Data::system;

//TODO: Should be a C++14 polymorphic lambda

This comment has been minimized.

@Ghabry

Ghabry Nov 2, 2018

Member

what is a polymorphic lambda?

This comment has been minimized.

@fmatthew5876

fmatthew5876 Nov 2, 2018

Author Contributor

A lambda where you can say auto. These 2 lambdas are identical except for the type of the parameter.

Show resolved Hide resolved src/rpg_fixup.cpp Outdated
Show resolved Hide resolved generator/csv/fields.csv Outdated
@CherryDT

This comment has been minimized.

Copy link

CherryDT commented Nov 2, 2018

panorama_data does save the position since 1.12... (before, the panorama position would reset when loading a game, even when it was normally moving with map, which I considered a bug as it would mess up panoramas which are used as carefully adjusted map backgrounds)

frame_count is number of times the main loop ran, note that there are occasions (such as screen transitions and attacks in battle) during which the screen is updated without exiting the main loop. I used this number as approximate play time counter in some occasions (through patches, I mean). Not sure right now what it's used for internally

@CherryDT

This comment has been minimized.

Copy link

CherryDT commented Nov 2, 2018

stop_count is counted towards max_stop_count until the next move is possible, this is used to wait during the "wait" move event, for movement frequency <8 and such. anim_count is for timing the stepping animation. These apply not only to the party but also the events and vehicles (the chunk id is below 100 so it's still in the shared range of SavePartyLocation, SaveEvent and SaveVehicleLocation)

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 2, 2018

Looks like we don't support SaveInventory::steps at all in Player. Will make a separate PR for that.

For frame_count, does it just count up to infinity? I imagine this number can get very large fast. Does it overflow and wrap at some point?

How does panorama_data work? It's only one integer but the background can scroll in the X and Y directions. Is it defaulted to 0?

@CherryDT

This comment has been minimized.

Copy link

CherryDT commented Nov 2, 2018

frame_count just counts up indefinitely. (Being a signed 32-bit integer, it would go haywire after 414 days of continuous playing... so not really important to consider.) It is used only for two things really: One is for drawing animated map tiles (their animation frame is based on it - probably you can ignore that because except for the very first map it's pretty unpredictable anyway what value the frame count will have, so it is unlikely that a game depends on that graphically), and for the animation of the character graphics and the animated arrows in the shop when buying equipment (pretty sure there is no benefit whatsoever in caring about that ^^).

No, panorama_data is its own structure, should probably be called SavePanorama, with X and Y in chunks 1 and 2. (Previously, it was an empty structure.) Yes, default is 0.

@fmatthew5876 fmatthew5876 referenced this pull request Nov 2, 2018

Merged

SaveGame Fixes #1470

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 2, 2018

This panorama_data feature doesn't appear to be implemented in rm2k steam. I created a 20x15 map and set the background to scroll horizontally with speed 8. When I save the game, RPG_RT always writes an empty chunk at 0x70. When I load the game, the background is reset to it's default position.

What this feature removed from some later versions of rm2k?

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:lsd_defaults branch 2 times, most recently from 8f91246 to b16d9f3 Nov 2, 2018

@CherryDT

This comment has been minimized.

Copy link

CherryDT commented Nov 2, 2018

It was implemented in RM2k3E v1.12, not RM2kE.

SaveSystem::screen: already answered Ghabry
SavePartyLocation::stop_count: I answered it above

Pictures: It shouldn't matter how many pictures exist in there as long as there aren't too many (although I'm not sure about that either - needs a test whether it crashes when the LSD has too many pictures) - should be fine to grow the picture array dynamically, like the switch/variables array work. And you cannot detect the amount of supported pictures (without analysing RPG_RT.exe, that is).

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:lsd_defaults branch 2 times, most recently from 0eabb96 to 1f8e1e0 Nov 4, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 4, 2018

The overall LSD work is ongoing, but this set of commits up to this comment are ready to go if approved ( same for player PR).

@Ghabry Ghabry added this to the 0.5.5 (or 0.6.0) -> we will see milestone Nov 4, 2018

@@ -26,6 +26,28 @@
namespace RPG {
class SaveSystem {
public:
enum ScreenRpgRt {

This comment has been minimized.

@fdelapena

fdelapena Nov 4, 2018

Contributor

Because they seem to be scene names, maybe scene is a better and shorter name instead of screen.

This comment has been minimized.

@Ghabry

Ghabry Nov 4, 2018

Member

Also find this RpgRt pretty ugly, but no idea how to name it. Screen oder Scene are an option. (and maybe add a comment that this is RPG_RT only).

This comment has been minimized.

@fmatthew5876

fmatthew5876 Nov 5, 2018

Author Contributor

I renamed it to just "Screen". I agree with Fdelapena that Scene is a better name than Screen, but Screen seems to be the terminology used by RPG_RT. Also since we call our thing a scene, using the word screen here helps avoid confusion.

This comment has been minimized.

@CherryDT

CherryDT Nov 5, 2018

Actually, the RPG_RT terminology is "scene" as well. (also in RGSS in later versions)

This comment has been minimized.

@fmatthew5876

fmatthew5876 Nov 5, 2018

Author Contributor

Well I guess we need to bikeshed this thing then. Here are some options:

  • SceneRPG_RT / scene_rpgrt
  • LegacyScene / legacy_scene
  • Scene / scene
  • Scene / scene - and change Player code / enums to use this

Votes anyone?

This comment has been minimized.

@Ghabry

Ghabry Nov 5, 2018

Member

We use Scene because RGSS calls it Scene ;).

Scene / scene

My favourite. When you document this as "Only used by RPG_RT, not by EasyRPG Player" (which you already do) I think this is not confusing and fine.

and change Player code / enums to use this

Another possibility but I'm against this because we have more scenes than RPG_RT which would mean adding custom scene entries to liblcf... not a fan of this.

Show resolved Hide resolved generator/csv/fields.csv Outdated

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:lsd_defaults branch 3 times, most recently from 86c778a to 3b1d092 Nov 5, 2018

@@ -705,7 +705,7 @@ SaveTitle,face3_name,f,String,0x19,,0,0,char[]: face filename
SaveTitle,face3_id,f,Int32,0x1A,0,0,0,int: face id
SaveTitle,face4_name,f,String,0x1B,,0,0,char[]: face filename
SaveTitle,face4_id,f,Int32,0x1C,0,0,0,int: face id
SaveSystem,screen,f,Int32,0x01,1,0,0,
SaveSystem,screen,f,Enum<SaveSystem_Screen>,0x01,0,1,0,The current Screen (Scene) type for RPT_RT. Legacy field only used by RPG_RT and not by EasyRPG Player.

This comment has been minimized.

@Ghabry

Ghabry Nov 5, 2018

Member

RPT_RT again btw

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:lsd_defaults branch from 3b1d092 to 6fac566 Nov 5, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 5, 2018

Comments addressed.

@Ghabry

Ghabry approved these changes Nov 5, 2018

Copy link
Member

Ghabry left a comment

(remember before merging that this depends on a Player PR)

graphics_name.clear();
}

#if 0

This comment has been minimized.

@Ghabry

Ghabry Nov 5, 2018

Member

The whole if 0 block can be removed

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:lsd_defaults branch from 6fac566 to 35adfb9 Nov 5, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 5, 2018

Removed the if 0. Great catch btw.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:lsd_defaults branch from 35adfb9 to 7a56ac7 Nov 5, 2018

@Ghabry Ghabry merged commit 5b52d85 into EasyRPG:master Nov 5, 2018

4 checks passed

GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows Build finished.
Details
web Build finished.
Details
@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 5, 2018

nice solution with this "UnFixup" btw, my initital idea for this was to get completely rid of "Fixup" and handle this in Player Getters, but I prefer yours.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 5, 2018

Still need to call unfixup for event info. That requires knowing the active page for the event at the liblcf level. Haven't figured out the best solution there yet.

@fmatthew5876 fmatthew5876 deleted the fmatthew5876:lsd_defaults branch Nov 7, 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.