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 Event Update Ordering #1628

Merged
merged 19 commits into from Mar 26, 2019

Conversation

@fmatthew5876
Copy link
Contributor

commented Feb 9, 2019

The PR refactors a number of interpreter and event related items

  • Adds support for triggered_by_decision_key chunk
  • Adds support for pause chunk
  • Adds support for waiting_execution chunk
  • Adds support for processed chunk
  • Fixes the execution order of event update routines to match RPG_RT.
  • Correctly handles "PreUpdate" which is the partial update routine done at startup which only updates events but not player or vehicles. This also happens when you teleport or use escape/teleport from the menu. I haven't addressed the teleport PreUpdate() cases yet and may not do so in this PR.

Fix: #1623
Fix: #1247
Fix: #1595
Fix: #663
Fix: #1664

Breaks: #937

@fdelapena fdelapena added this to the 0.6.x milestone Feb 12, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_collide branch from 9653881 to 594be29 Feb 20, 2019

@fmatthew5876 fmatthew5876 changed the title Refactor Event Collision Refactor Interpreter Triggering Mar 2, 2019

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

This PR is now becoming very experimental as I refactor how and when the interpreter starts. Please don't recommend people to run it in continuous builds until we have a good suite of passing test cases.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_collide branch 14 times, most recently from 124150d to fac5d8c Mar 2, 2019

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_collide branch 6 times, most recently from 590dc37 to 377f196 Mar 4, 2019

fmatthew5876 added some commits Mar 7, 2019

Allow calling scenes from PreUpdate()
The pre-update that happens when Scene_Map initializes is allowed to
change the scene (for example CallSaveMenu) before the first
actual Update() routine runs.

To enable this behavior we:
* Split the scene calling logic to a separate method
* Encapsulate the call to Game_Map::Update(true)
* Add an OnTransitionFinish() hook to check for scene calls
  only after PreUpdate() has been called.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_collide branch from acad1d5 to 86ab84f Mar 17, 2019

@Ghabry

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

This PR breaks faster decrement of the Wait counter when an interpreter is preempted when walking against events. I also checked against #1687 with same result.

Have two parallel events on same-as-hero-layer:

``
@> Wait: 5.0 seconds
@> Text: A

@> Wait: 5.0 seconds
@> Text: B
``

Walk against one of the events the whole time, this event will show the message after 2.5 seconds.
(This can be fixed in a later PR, as this is a quite obscure "feature")

@Ghabry

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

I can confirm that this really breaks Pom Gets Wifi, so the Pom-Bug is actually two problems in one: Movability while a move route is active + this wait bug.

So to track this will simply reopen #937 when this gets merged.

@Ghabry

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

"Check for map refresh before exiting intpreter Update()"

if (force_through) {
SetThrough(true);
}
auto sg = makeScopeGuard([&](){

This comment has been minimized.

Copy link
@Ghabry

Ghabry Mar 24, 2019

Member

is the usage of a scope guard really needed here?
There is only one exit path and we don't throw/catch exceptions.
Though what actually bugs me is that this comes from liblcf. We already have other borderline-stuff in liblcf (INI) we call from Player but imo here the header should be copied into Player and made a private/not-installed header in liblcf.

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Mar 24, 2019

Author Contributor

I always get paranoid whenever there is a thing like this where you temporarily set some state, call a function, and then restore it. This gets optimized away and we ever change the code later to throw it's still safe.

Overly paranoid for sure but that's usually how I do these things as a habit. Can take it away if its really objectionable.

This comment has been minimized.

Copy link
@Ghabry

Ghabry Mar 24, 2019

Member

I understand the intention but I want to discourage that liblcf becomes a general collection of convenience headers that are used by Player because we don't use C++17.
So I would be already happy if you put a copy of scopeguard into the Player src folder, too.

fmatthew5876 added some commits Mar 18, 2019

Check for map refresh before exiting interpreter Update()
Fixes cases where the interpreter loop exits early due to some
condition. Ensures map is always refreshed.

Fixes Test case 1.3 in #1664
Fix max_stop_count logic when move route started
Fixes test cases 11 and 38 in #1567

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:event_collide branch from 1d25f5c to 92fb064 Mar 24, 2019

// Placed after the interpreter update because multiple updates per frame are allowed.
// This results in waits to finish quicker when an event collides with this event and
// emulates a RPG Maker bug)
// Only update the event once per frame

This comment has been minimized.

Copy link
@Ghabry

Ghabry Mar 24, 2019

Member

just marking this here because this comment must be readded when this is fixed again

// Placed after the interpreter update because multiple updates per frame are allowed.
// This results in waits to finish quicker when an event collides with this event and 
// emulates a RPG Maker bug) 
Graphics::Update();
if (was_transition_pending && !Graphics::IsTransitionPending() && Scene::instance->IsInitialized()) {

This comment has been minimized.

Copy link
@Ghabry

Ghabry Mar 24, 2019

Member

I don't find this intuitive. Could you add a comment to explain that this is used by Scene Map to handle scene calling before the first update?

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Mar 24, 2019

Author Contributor

I don't find it intuitive at all. In fact I hate it, but I don't know how else to solve this.

I will add the comment.

@Ghabry

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

Well this PR was much easier to review than the STop Count one but MakeWay will be hell again

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

I've added #1724 to discuss ScopeGuard and other utilities. We can follow up with another PR whatever we decide there.

For the wait bug, we can reopen #937 and put details in there until its fixed again after #1687.

I've addressed the other comments.

@Ghabry

Ghabry approved these changes Mar 24, 2019

return ce->trigger == RPG::EventPage::Trigger_parallel &&
(!ce->switch_flag || Game_Switches.Get(ce->switch_id))
&& !ce->event_commands.empty();
}

This comment has been minimized.

Copy link
@carstene1ns

carstene1ns Mar 26, 2019

Member

Maybe these could be combined?

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Mar 26, 2019

Author Contributor

Sure, I'll drop a commit for that into the next one #1687

@@ -142,11 +142,18 @@ void Game_Interpreter::SetContinuation(Game_Interpreter::ContinuationFunction fu
continuation = func;
}

bool Game_Interpreter::ReachedLoopLimit() const {
return loop_count >= 10000;

This comment has been minimized.

Copy link
@carstene1ns

carstene1ns Mar 26, 2019

Member

This should be a constant here already. I have seen this in another commit.

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Mar 26, 2019

Author Contributor

Thats fixed in #1713

} else if (trigger == RPG::EventPage::Trigger_collision) {
CheckEventTriggerTouch(GetX(),GetY());
return;
}
}

This comment has been minimized.

Copy link
@carstene1ns

carstene1ns Mar 26, 2019

Member

These returns seem unnecessary now.

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Mar 26, 2019

Author Contributor

Will fix in #1687 along with the previous comment

This comment has been minimized.

Copy link
@fmatthew5876

fmatthew5876 Mar 26, 2019

Author Contributor

This all gets blown away in #1697 so I'm leaving it alone for now.

@carstene1ns carstene1ns merged commit ac845a7 into EasyRPG:master Mar 26, 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:event_collide branch Mar 26, 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.