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

Fix Saving and Loading of panning chunks #1498

Merged
merged 5 commits into from Dec 9, 2018

Conversation

Projects
None yet
5 participants
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Nov 18, 2018

Don't merge until #1502 is merged

Fixes saving and loading games in the following conditions. Saves were tested to be compatible between RPG_RT and Player for these behaviors.

  • When the screen is panned.
  • While the screen is panning.
    • Loading correctly continues the panning. This is broken in RPG_RT.
  • While screen is locked

I also tested these scenarios

  • Screen behavior when event unlocks when screen is in default centered position.
  • Screen behavior when event unlocks when screen is panned.
  • Teleporting to same map does not change any panning state
    • If pan lock is enabled, the screen will still change position when you teleport to the same map.
  • Teleporting to different map resets pan position and speed, but not pan lock.
  • Panning move screen event commands still work while screen is locked.
    • Including Loading while pan is occurring.
  • Multiple consecutive no wait panning move command overwrite each other. All but the final one become no-ops.
  • Verified return to origin panning still works.
    • Including all 4 directions of diagonal panning

This patch uses RPG_RT centered panning units inside Game_Map and converts to 0 based for use externally. This was done to minimize the amount of change on panning code.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:pan branch 7 times, most recently from 540eead to fe7ab68 Nov 18, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 18, 2018

@CherryDT did I miss any edge case?

@everyone
This stuff is tricky. Could use a second pair of hands to retest the cases mentioned and any more you can think of that I didn't mention.

@CherryDT

This comment has been minimized.

Copy link

CherryDT commented Nov 18, 2018

Unfortunately I don't have time for this at the moment, maybe later

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 18, 2018

My request for testing was more for carstenens, Ghabry, or whomever reviews the code.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:pan branch from fe7ab68 to 4688022 Nov 18, 2018

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 19, 2018

one game to test is this one as it had multiple pan issues before: #1169 (comment)

Also Razas had an issue in the intro, but I'm not sure if this was full verison only, going to test this by my own.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 19, 2018

I tested some HH3 intro scenes and they work with this branch.

Another test case is you can produce diagonal panning using a no-wait scroll followed by a wait scroll. For example Up (no-wait), and Right (wait). I checked and this case works in Player.

@fdelapena
Copy link
Contributor

fdelapena left a comment

default panning constant expressions might be written as * 256 to be more human readable, but looks good to me.

@@ -151,6 +150,7 @@ void Game_Map::Setup(int _id) {
events.emplace_back(location.map_id, ev);
}

// pan_state does not reset when you change maps.

This comment has been minimized.

@Ghabry

Ghabry Nov 26, 2018

Member

This could fix a bug in Ara Fell (have no savegame around for this) but the pan after loading a new map was completely off, which requires you to navigate blind to the next exit to fix it.

@Ghabry
Copy link
Member

Ghabry left a comment

This PR breaks indeed the test case in #1169 (comment)

Go to the right until you reach the "mouse ate the tile" msgbox, then go down until you can't go down anymore and then hold right. Wait for the Pan to finish, the "faceset" is gone. This is because the screen is not aligned to the 16-pixel grid anymore

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 26, 2018

also agree on the note of fdela to use 256 * X to make the calc more clear

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:pan branch from 4688022 to 376ef6b Nov 28, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 28, 2018

I fixed the bug in that test case. You were right in that its related to the map position not being a multiple of 256.

I also fixed the constants to be multiples of SCREEN_TILE_WIDTH

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:pan branch from 376ef6b to 3030e98 Nov 28, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 29, 2018

I did some testing of panning with looping maps. Everything appears to work.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 1, 2018

okay, will also do another quick test tomorrow from my side if I can still break it, havn't tested the Razas intro yet ^^

@carstene1ns carstene1ns requested a review from Ghabry Dec 4, 2018

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 5, 2018

This also reintroduces the "Pan too far" in Razas:
Unfortunately there are not any details available how the Panning is special, wasn't ever analyzed. Was fixed as part of #1197

Game download: https://razas.itch.io/razas

Save13.zip

Load the save and on the next map in that cutscene there is pan downwards which goes too far until it reaches the map border.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 6, 2018

For a more concise test, just place your party starting position at map "69: Inexplored Lands 3" at (24,15) and start a new game.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:pan branch from 3030e98 to 0f30b42 Dec 6, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 6, 2018

Rebased the last change. Now it fixes both Razas and Personal Project.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 6, 2018

@fmatthew5876 uhm for me Razas is still broken o.O

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 7, 2018

Well you're right, it is still broken. Somehow I really messed up here.

Looking into it again..

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:pan branch from 0f30b42 to 71f6e08 Dec 7, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 7, 2018

You guys are going to hate me for this.

Previously I was converting panning from (9,7) internally to (0,0) externally in order to minimize code changes. In order to fix these panning problems and get panning implemented the right way, I've had to completely refactor how player scrolling works from scratch.

Panning has a special behavior I didn't notice before. Lets say you are 2 tiles from the right edge of the map, and then you do a panning command to move to the right by 5. What actually happens is RPG_RT will only pan 2 tiles and then stop. Pan current will be +2 and pan finish will be +5. As soon as you move left enough, the pan will continue until it hits the edge again or gets to +5.

Before we tracked how much we actually panned in a local variable (actual_pan_x/y) and kept incrementing pan_current each frame. This is wrong, pan_current is stops when it hits the edge of the map. We don't need a local variable to track this.

This behavior can be confirmed by saving your game and looking at the chunks. You'll see pan_current = +2 * 256 and pan_finish = +5 * 256.

Finally, we had invented 2 member variables last_remaining_move and last_remaining_jump. There are several more like this in the Game_Character hierarchy and many are removed by #1502. These variables are not saved in LSD, and thus writing algorithms this way will produce inaccuracies when saving and loading.

The panning refactor removes these 2 local variables. Once #1502 is in and SaveMapEvent::remaining_steps is used, we should have frame perfect position and panning in save games.

I apologize in advance for the code review this is going to need...

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:pan branch from 71f6e08 to ab37d0f Dec 7, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 7, 2018

I did some testing of panorama. It does not appear broken by this PR.

Please merge #1502 before merging this.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 7, 2018

Thanks, instead of reviewing code I suggest playing games instead, therefore this will take a while to find regressions this could cause ^^'

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 8, 2018

This gets my approval now, works with that fancy test, Razas and my new map loop rendering code.
I just keep the X to prevent an accidental merge before 1502

fmatthew5876 added some commits Nov 15, 2018

Fix panning x and y to match RPG_RT
* Rescale to use RPG_RT values
* Supports saving and loading x and y values
  between RPG_RT and Player
* Correctly continues non-wait panning when loading
  saved game (RPG_RT bug fixed in player).
Refactor pan_state
- Move IsPanLocked() checks internally.
- Fixes bug where saved games with pan lock and panning don't load
  screen position correctly.
- pan lock does not reset when changing maps (match RPG_RT)
- panning does not change at all when teleporting to same map.
- panning commands work while screen is locked
- pan lock is correctly loaded from save games
Completely refactor panning
* System uses panning in its natural (9,7) units
* Panning is updated after player position
* Correct edge of map behavior
* Correct behavior on looping maps.
* Remove local variables that aren't saved/loaded

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:pan branch from ab37d0f to b81ee26 Dec 9, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 9, 2018

rebased to master

@Ghabry

Ghabry approved these changes Dec 9, 2018

@Ghabry Ghabry merged commit a85ea61 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:pan branch Dec 11, 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.