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 scene calling #1658

Merged
merged 5 commits into from Mar 5, 2019

Conversation

@fmatthew5876
Copy link
Contributor

commented Mar 2, 2019

Attempt to fix #1642

@Ghabry Ghabry added this to the 0.6.x milestone Mar 2, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:scene_call branch from dd57aef to 9c969f7 Mar 2, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

So this fixes #1642 with a caveat..

In RPG_RT, the save menu appears once, and then both events dissapear.
In this PR, both events dissapear and then the save menu appears once.

This is because of #1623 causing parallel events to run twice in the first frame of Player.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

@CherryDT

If you're around. I've noticed some very strange behavior with event code delaying by frames in ways that don't make sense..

When an event calls a menu scene, does this have any affect on how events execute that frame? Or does it just let them all execute like normal and then calls the menu at the end?

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:scene_call branch 2 times, most recently from cd29e34 to 5673a59 Mar 3, 2019

@CherryDT

This comment has been minimized.

Copy link

commented Mar 3, 2019

When an event calls a menu scene, does this have any affect on how events execute that frame? Or does it just let them all execute like normal and then calls the menu at the end?

It does have an effect. A quite complex effect actually, I believe.

It sets the scene value to menu immediately. This then has a couple of effects:

  • The drawing of the scene is skipped (I'm not sure if the drawing of the map scene has any side-effects)*
  • The map scene's update method will do a fade-out at the end and set a flag to remember to reinitialize the next time it is called
  • All event workers will immediately exit the main loop described here, the same way as if 10000 iterations had passed or there was no more code/events to process (this is probably the behavior you see)

Now to that last point: It's not as simple as just stopping all event execution. Remember that in general, there are two kinds of workers, the "parallel process workers" (where one exists for each such event) and the "global autostart event worker" (where only one exists) which handles autostart events and events otherwise triggered (e.g. by action button) and has more complexity in its main loop than the parallel process workers.

Also remember that the overall order is this (from my other post):

  • Reset the "already acted" flag for all events on the map (which is used to prevent events to trigger twice on the same frame in edge cases)
  • Ask all common events' parallel process workers to run
  • Ask all map events' parallel process workers to run
  • Ask all map events (including vehicles and party) to act (unless its "already acted" flag is set): Acting is taking an action like moving, but it will also check if the event is autostart and the start conditions are met, in this case its "waiting" flag is set. The "already acted" flag is also set. (If an event is triggered in another way, for example by action button, it would also get its "waiting" flag set.)
  • Ask the global autostart event worker to run. [...]

Now, both kinds of workers will check at the end of their loop whether the scene is no longer the map and would exit the loop in that case. However, since this happens after command execution, that means that even if the scene is changed, each worker may execute one command before reaching that check and exiting.

Therefore, if a parallel process changes the scene, you will see other events still executing one single command - those events who are parallel process events coming after the scene-changing one (and all map events come after all common events) and the first autostart/otherwise-triggered event, if any. (You should not see anything weird if an autostart/triggered event calls the menu, since the global autostart worker is the last one to be run.)

Note: I did not empirically test this, this is just the result of logical deduction based on what the code in the binary appears to be doing.


*: To help understanding what this means: The main loop normally does things in this order: Increase frame counter -> Update key input -> Update current scene -> Draw current scene -> Wait for next frame. The drawing can be skipped in case the updating used too much CPU time recently, or when the scene was changed during the updating of the old scene. (And there are occasions, like transitions and some things in battle, where a "mini-version" of this loop is run inside of the scene updating itself, without updating the frame counter!)

@CherryDT

This comment has been minimized.

Copy link

commented Mar 3, 2019

Note that this also means that you need to be very careful with your save-file-based testing method because opening the save screen has the same effect!

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

each worker may execute one command before reaching that check and exiting.

Thanks for confirming this Cherry, this matches exactly what I've seen empirically. And checking at the end of the loop makes more sense as to why this would be the case.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

Given the amount of testing I've done against this + Cherry's confirmations, this is ready for review and merge.

There is one more small piece of this where we don't load new events in the interpreter after the 1 command executes, but that's implemented in #1628. It can't be done here without that PR's refactors first.

Even without that, this is an improvement and good to merge.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

Note: This doesn't treat the special game over case mentioned in #1642.

I'll address that later as I don't want to cause a game over regression when this is more for fixing the interpreter.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Rebased to master

@Ghabry

Ghabry approved these changes Mar 4, 2019

Copy link
Member

left a comment

(just the commit msg typo)
And this exec 1 command after scene call is ridiculous :/

@Ghabry

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

commit msg typo: "Fixes bug there " -> "Fixes bug where "

and how about recyling the Scene::SceneType enum instead of creating a new one?

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:scene_call branch from f54c4ee to bd560b8 Mar 4, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Addressed review comments

@CherryDT CherryDT referenced this pull request Mar 5, 2019
@carstene1ns

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Commit message of c463dad is wrong now 🙈

  • Add eBattle enum values

fmatthew5876 added some commits Mar 2, 2019

Only allow calling 1 scene
If multiple events call scene on same frame, last one wins.
Integrate battles with normal scene call method
* Use scene_call object to trigger event battles
* Still set Game_Temp::battle_calling to handle transitions correctly
Scene calls- return false and increment index
* Fixes bug where OpenMainMenu would repeat when you
save and load from it.
Allow only 1 command to process when a scene is being called
When a scene is being called, each intepreter is allowed to execute only
one command per frame. That means each parallel event can advance by
1 action, and only 1 foreground event can advance

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:scene_call branch from bd560b8 to 6ca4288 Mar 5, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Commit message of c463dad is wrong now see_no_evil

  • Add eBattle enum values

Fixed, thanks!

@carstene1ns carstene1ns merged commit 9166e00 into EasyRPG:master Mar 5, 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 fmatthew5876 deleted the fmatthew5876:scene_call branch Mar 6, 2019

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.