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

More panorama fixes #1554

Merged
merged 13 commits into from Dec 22, 2018

Conversation

Projects
None yet
5 participants
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Dec 11, 2018

I found more issues with panorama.

  • Our panorama pan_x / pan_y were not compatible with rm2k3e. The means saving and loading games between them will be wrong. We were treating them as scrolling offsets, but they are actually the entire panorama image offset.
  • We weren't handling Looping and scrolling maps right
  • SaveScreen::pan_y was inverted (not used in panorama but similar code so added here)
  • Save data not loaded correctly in all cases
  • Scroll jumps when you move slowly and is sometimes 1 pixel off in frames.
  • Retest #1550 with this PR.
  • Fixes: #1534

So I've had to refactor this yet again!

I've retested this by trying different combinations of maps, saving the game in various spots, and comparing the pan_x and pan_y of Player vs RPG_RT.

Testing performed:

  • Map with no looping, no scroll - Panorama is 2x size and moves with relative position on map
  • Map with looping, no scroll - Panorama is screen size and doesn't move
  • Map with looping, scroll - Panorama is screen size and moves with player
  • Map with no loop, scroll - Panorama is screen size and moves with player
  • Map with loop, scroll, auto scroll - moves with player and moves by itself

I'm going to need to ask you guys to help test this one. Try some different size maps and panorama pictures. I don't want to keep finding bugs and refactoring this over and over again.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch from 35192d0 to 5723d59 Dec 11, 2018

@fmatthew5876 fmatthew5876 changed the title Panorama returns More panorama fixes Dec 11, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 11, 2018

Because the pan_x and pan_y values are the total panorama offset and not just scrolling offset, loading save games becomes problematic.

We have to do the reset logic in Game_Map::Parallax::Initialize(). The problem is this function is called asynchronously. I added a global variable which is set in SetupFromSave() and then retrieved in Initialize() but this solution is disgusting.

Anyone have a better idea how to do this?

@fmatthew5876 fmatthew5876 changed the title More panorama fixes More panorama fixes - Do not Merge Dec 11, 2018

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch from 37903b4 to 52a94d8 Dec 11, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 11, 2018

With these changes, the Yume Nikki bug is back. It's something more serious related to cross the edge of looping maps. When you cross the boundary, Game_Map::ScrollRight(-28128) is called. The horizontal size of the map is 28160 1/16 pixels wide.

The last panorama fix just hid the bug, it seems to affect other things too.

I'm still looking into it.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 12, 2018

I've fixed the Yume Nikki bug for real this time

I don't like the solutions here. In particular the savegame global and modulus stuff added to Player::UpdateScroll().

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch from 111b0fc to 0449b7c Dec 12, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 12, 2018

I fixed the panorama algo. It had some micro level frame by frame issues. I've verified it by using my frame by frame save generation script.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch from 0449b7c to 4fa3830 Dec 12, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 12, 2018

I've tested the new algo with the following cases:

640x480 Image

  • 41x31 Map - 1 tile more than pic
  • 40x30 Map - Same size as pic
  • 39x29 Map - 1 tile less than pic
  • 30x15 Map - Smaller than pic
  • 20x15 Map - Default map size, pic doesn't move

This is ready for review now.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch from 4fa3830 to c7f069d Dec 12, 2018

@fmatthew5876 fmatthew5876 changed the title More panorama fixes - Do not Merge More panorama fixes Dec 14, 2018

Show resolved Hide resolved src/game_map.cpp Outdated

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch from c7f069d to 1a3e356 Dec 15, 2018

@carstene1ns

This comment has been minimized.

Copy link
Member

carstene1ns commented Dec 15, 2018

Commit 5bdbeaa is already part of #1555.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch from 1a3e356 to 5249897 Dec 17, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 17, 2018

Now that #1522 is in, I've added support for resetting the panorama when SetVehicleLocation() or similar function which ends up calling Game_Player::MoveTo() is used.

With this, panorama should really be done now. I'd like to get this one out to the beta testers so we can catch any regressions before next release.

Please do one more code review to make sure I got all my X's and Y's straight.

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

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 19, 2018

FIXME: load_panorama_from_save

Can I have some further context? Which async-handler is invoked and causes issues? Note that on desktop the async handlers are synchronous (they always fire directly).

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 19, 2018

FIXME: load_panorama_from_save

Can I have some further context? Which async-handler is invoked and causes issues? Note that on desktop the async handlers are synchronous (they always fire directly).

The core problem is that from when we need to communicate the fact that it's a save game from the invocation of the async handler to when this Initialize() gets called.

To make matters worse, it looks like the async handler is triggered within an update call. I hate this global, but I don't know a better way.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 19, 2018

fmatthew5876 added some commits Dec 11, 2018

Don't pass looping wrapped offsets to Game_Map::Scroll
When the player crossed the boundary of a looping map,
Scroll would get passed a huge number. This worked
for Game_Map::Scroll() but broke SaveScreen and panorama.

This is the cause of the Yume Nikki jumping panorama
bug in #1550
Fix no loop / no scroll panorama algo
* Algorithm would jump at low speeds and have small frame inaccuracies.
Remove Player::Center()
* Map position_x/y already loaded from save, we don't need
  to call Center() from player.cpp to recompute them.
* Function is not needed, so just put logic in MoveTo()
Reset panorama when fixing map position
When player is moved by a command such as Transfer,
SetVehicleLocation, etc.. The panorama will reset.
Clamp no loop panorama scrolling
No loop scroll is capped at a maximum of map_pos * 2, or
512 per tile scroll
@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 22, 2018

The double reset behavior you're seeing happens because of the funky way we reload panoramas.

The first time we call ChangeBG(), the panorama offsets go to zero. Then when Initialize() is called later, they get reset based on map position.

The second time we call ChangeBG(), the panorama offsets go to zero. But this time the panorama name didn't change, so the reload logic in Update() is never called.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch 3 times, most recently from 77ec899 to 1954e5f Dec 22, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 22, 2018

We should now be fully compatible with RPG_RT, bugs and all.

The panorama offsets will not reset when

  • Change panorama on looping map
  • Change to a scrolling panorama
  • Change to same filename (RPG_RT compatible, can cause pops when
    scrolling)

We do reset when:

  • Loading game with non-scrolling panorama
    • Even if panorama was saved with different offsets
    • This logic is critical to correctly load non rm2k3 save games and not mess up the panorama.
  • Changing panorama to non-scrolling

In particular we now emulate the first gif. The reason the panorama doesn't reset in that case is because the filename doesn't change. We happen to copy this bug because our logic in SpriteSet_Map::Update() is dependent on filename.

Furthermore, if you do the movement in the first gif, save your game after switching to the top panorama. Then reload, the panorama will be loaded at the correct (0,0) offset, not the one you see on the screen. Since the offsets are correct after loading, the panorama won't pop in this case.

So the popping in the first gif looks like an RPG_RT bug, which for now we also emulate!

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 22, 2018

Here is another test, which works:

  1. Copy the scrolling 100x100 panorama event from (1,13) to (1, 23)
  2. Trigger the 1000x1000 panorama at the top of the map
  3. Walk down to (1,23)
  4. pan_y is now 8192
  5. Talk to the 100x100 event, the 100x100 panorama is displayed
  6. pan_y is still 8192 (which is greater than the max panning offset allowed by this image)
  7. Step Up 1 tile
  8. pan_y snaps to 1536, scrolling is smooth and correct.
@carstene1ns

This comment has been minimized.

Copy link
Member

carstene1ns commented Dec 22, 2018

👍

@Ghabry Ghabry removed the Needs feedback label Dec 22, 2018

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 22, 2018

Wow this is really impressive! Unfortunately the looping map case is behaving incorrectly :/.

New Panorama Test (basicly the old one but the pictures have now the size that is written in there filename...)
panoramatest.zip

On Looping Map 41x32: Different scrolling
pana

When the other side of the looping map becomes visible the panorama jumps:
panc

The jumping doesn't happen when the panorama mode is "H/V" in that case only the scrolling is wrong.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch from 1954e5f to aef71d2 Dec 22, 2018

fmatthew5876 added some commits Dec 22, 2018

Refactor reset panorama position logic
The panorama offsets will not reset when
* Change panorama on looping map
* Change to a scrolling panorama
* Change to same filename (RPG_RT compatible, can cause pops when
  scrolling)

We do reset when:
* Loading game with non-scrolling panorama
   * Even if panorama was saved with different offsets
* Changing panorama to non-scrolling

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:panorama_save branch from aef71d2 to c10e1aa Dec 22, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 22, 2018

Pushed a fix for looping maps.

I didn't see it misbehaving on looping maps with H/V scroll set...?

@Ghabry

Ghabry approved these changes Dec 22, 2018

Copy link
Member

Ghabry left a comment

The only thing I see is that for looping maps with H/V Panorama the offsets are a bit off in some cases. But this is now really nit-picky as everything else works.

@Ghabry Ghabry merged commit 0384e2e into EasyRPG:master Dec 22, 2018

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
@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Dec 22, 2018

Do you have a specific example when it's off? Maybe a small numerical tweak is all that's needed

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Dec 22, 2018

just use the bottom right events on the 41x32 map. YOu will see that the Panorama doesn't match the RPG_RT position.

@fmatthew5876 fmatthew5876 deleted the fmatthew5876:panorama_save branch Jan 27, 2019

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.