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

Vehicle bgm fix #1620

Merged
merged 3 commits into from Feb 7, 2019

Conversation

Projects
None yet
4 participants
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Feb 4, 2019

Fixes #1619

Note: I use the aboard flag here because that's what tells us the player is actually inside the vehicle. You could have an event that teleports you while boarding and then the bgm effect would not happen.

I tested this parallel event:

GetOnOffVehicle
Wait 0.0s (x3)
Teleport other map

The player teleports to the other map, the other map music plays, and then the player boards the ship on that map and the vehicle bgm never plays.

I'll fix that behavior in #1601

@fmatthew5876 fmatthew5876 changed the title Vehicle bgm Vehicle bgm fix Feb 4, 2019

@fdelapena fdelapena added this to the 0.6.0 milestone Feb 5, 2019

@carstene1ns

This comment has been minimized.

Copy link
Member

carstene1ns commented Feb 5, 2019

What about the second part of #1619? Seems memorized bgm should be cleared on map change.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:vehicle_bgm branch from 86603b6 to 769bd66 Feb 5, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Feb 5, 2019

What about the second part of #1619? Seems memorized bgm should be cleared on map change.

You're right, I overlooked that. We actually have SaveSystem::before_vehicle_bgm chunk designed exactly for this purpose, which Player doesn't use currently.

I tested these cases:

  • Get on, Get off -> plays map music
  • Change map music, Get on, Get off -> plays changed music
  • Get on, change map (no change music), Get off -> plays other map music
  • Change map music, Get on, change map (no change music), Get off -> plays other map music
  • Get on, change music, get off -> plays map music -> get on again -> plays vehicle music
  • Get on, change music, change map (no change music), get off, plays other map music
@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Feb 5, 2019

And another one, similar to the escape/teleport/save edge case fixed previous.

fmatthew5876 added some commits Feb 4, 2019

Fix vehicle music behavior
Correctly use SaveSystem::before_vehicle_bgm
Fix bgm edge case for map children of the root
If map bgm type is "parent" and it is also a direct child
of the root map, it behaves as if it's bgm type was "event"

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:vehicle_bgm branch from 3bf010c to 220c9ad Feb 5, 2019

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Feb 5, 2019

Looks good to me but I don't fully understand the difference between the aboard flag and "InVehicle()".

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Feb 5, 2019

Looks good to me but I don't fully understand the difference between the aboard flag and "InVehicle()".

The difference matters when boarding the ship. As soon as you start boarding, the player vehicle chunk is set to the ship or boat. At this time, the player is still visible and the music behavior is the same if you teleport. Only once the boarding actually finishes does the aboard flag get set and then the vehicle music behavior takes effect.

@carstene1ns

This comment has been minimized.

Copy link
Member

carstene1ns commented Feb 7, 2019

I guess the other occurrences of InVehicle() might not care about this corner case. Maybe can be refactored/simplified.

@carstene1ns carstene1ns merged commit 5a476be into EasyRPG:master Feb 7, 2019

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 Feb 7, 2019

I guess the other occurrences of InVehicle() might not care about this corner case. Maybe can be refactored/simplified.

There are times when you only need to know if the vehicle is "in use" and other times when it is "boarded". After #1601 we should do a review of this and simplify the API to make this distinction obvious. I expect the majority of cases required boarded.

@fmatthew5876 fmatthew5876 deleted the fmatthew5876:vehicle_bgm branch Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment