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 player update handling + scene calling #1714

Merged
merged 10 commits into from Apr 30, 2019

Conversation

@fmatthew5876
Copy link
Contributor

commented Mar 17, 2019

Depends on #1697

Fix: #1691
Fix: #1457

Fixes the gameover part of #1642. The message vs gameover stuff is not fixed yet, but that looks like a message bug to be fixed in another PR.

I also haven't studied too deeply the immediate "to title" behavior.

2 more game temp variables killed!

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:player_scene branch from c2a3977 to 21344d4 Mar 17, 2019

@fdelapena fdelapena added this to the 0.6.1 milestone Mar 17, 2019

@fdelapena fdelapena added the Refactor label Mar 17, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:player_scene branch 5 times, most recently from e2cf79f to e742ee4 Mar 17, 2019

This was referenced Mar 21, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:player_scene branch 3 times, most recently from eba88c3 to 1458eca Mar 28, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:player_scene branch from 1458eca to 050c96f Apr 7, 2019

if (Game_System::GetAllowMenu()
&& !Game_Message::visible
&& !Game_Map::GetInterpreter().IsRunning())
{

This comment has been minimized.

Copy link
@Ghabry

Ghabry Apr 8, 2019

Member

Refactor player update routine order

I'm not sure about the change for ESC-Menu calling.

I thought the menu can't be accessed when the route is overwritten or a message is pending to be shown but both these conditions were removed.

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Apr 8, 2019

Author Contributor

UpdateSelfMovement() is not called by Game_Character if IsMoveRouteOverwritten() == true.

For the message pending part, I'll need to check how that overlaps with visible.

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Apr 9, 2019

Author Contributor

It's confusing as we don't have a standard way to ask "Is there a message active?"

message_waiting goes from when the ShowMessage command was invoked up until the message box starts to close. visible goes from the first frame of the actual message until the message close animation is done. So in most cases you probably want both.

I changed this to check for both. Also I added back a check for !IsMoveRouteOverwritten() I guess in theory if you press a direction and ESC at the same time, the player directional move could cause an event to run, which then sets a move route, which then prevents the player from opening the menu in the same frame...

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:player_scene branch 3 times, most recently from 9f759f2 to 01ed2be Apr 9, 2019

@Ghabry

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Has deps so not mergeable but except for the menu calling and message waiting which I find suspicious and will do more testing with I give my 👍 here.

Refactor player update routine order based on Cherry

Can you find the post where Cherry explained this?

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

More info here #1691

My original conversation with Cherry is here:
#1626 (comment)

@fdelapena

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:player_scene branch from 01ed2be to edd87c3 Apr 20, 2019

fmatthew5876 added some commits Mar 17, 2019

Refactor player update routine order
* Put vehicle checks in their own function
* Change conditions on player input to match Cherry's description.

fmatthew5876 added some commits Mar 17, 2019

Refactor Gameover handling
* Remove Game_Temp::gameover
* Use Scene::instance->SetRequestedScene()
* Handle gameover call events like other menu calls
Correct handling of menu_calling flag
* When ESC is pressed, set the flag
* Clear if teleported
* Clear if foreground event runs
* On next update, if flag is set, clear it and request menu scene
* Disable movement / decision if we called menu
Devirtualize IsStopping()
IsStopping() means not moving or jumping. Don't confuse it
with airship stuff.

It's also called very often, so it should not be virtual.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:player_scene branch from edd87c3 to 411711d Apr 27, 2019

@Ghabry

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

hm the moving is really blocked until the box is closed, not until it starts closing. Nobody noticed this for 10 years.

&& !Game_Message::visible
&& !IsMoveRouteOverwritten()
&& !IsPaused() // RPG_RT compatible logic, but impossible to set pause on player
&& !Game_Map::IsAnyEventStarting())

This comment has been minimized.

Copy link
@Ghabry

Ghabry Apr 29, 2019

Member

imo this could be in "IsMovable". When you look into IsMovable almost all of this is already in there (Graphics::TransitionPending is nonsense and can be left out).
Also IsMovable is currently dead code, nothing calls it.
IsMovable even contains a case you forgot:

bla

(fancy APNG, should be animated, couldnt get 60 FPS gif to work)

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Apr 30, 2019

Author Contributor

Nice catch! Thanks

@Ghabry

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

also when the airship is asc/desc'ing the menu is delayed until the animation finishes, in Player is immediate, not a regression, just for the notes.

Fixes for vehicles and player controls
* Delay menu calling until after boarding finishes
* Don't allow movement while airship is ascent / descent
* Remove IsMovable() dead code.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:player_scene branch from a5ef15b to f2726ff Apr 30, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

I fixed the vehicle issues, including the menu thing.

Those are good catches, thanks for that.

I opted to just remove IsMovable() since it isn't really applicable anywhere else. Keeps the logic localized in one place and easier to follow imo.

New test cases for this added to #1691, all passing with my new commit.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Test 4 in #1691 proves we need to check message_waiting for all controls

If I remove the message_waiting check, then in Player we will:

  • ESC: will print "hello" and then call the menu.
  • Direction: player will step and then message box appears.
  • Enter: Player will board or trigger event and message box appears.
Fix movement vs menu / encounter calling behavior
* When ESC is pressed during a move route, menu is called when move route
finishes.
* When player steps and then presses ESC, menu is called when step
finishes
* When player steps, encounter_calling is set, and then a
  parallel event triggers a move route before the player finishes their
step, the battle will not trigger until all movement is done.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:player_scene branch from 7c1ee2a to cd920ed Apr 30, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

It looks like Ghabry's original concerns about IsMoveRouteOvewritten() were valid. You can press ESC during move route and menu will be summoned when the move route finishes.

Also more edges cases related to this are fixed, and described in test cases.

@Ghabry

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Oh, so the vehicle menu calling was just an edge case, good finds 👍

@Ghabry

Ghabry approved these changes Apr 30, 2019

@Ghabry Ghabry merged commit 3ef8b59 into EasyRPG:master Apr 30, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.