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

Improve Window_Message timing accuracy - Messages 6 of N #2026

Merged
merged 14 commits into from May 20, 2020

Conversation

fmatthew5876
Copy link
Contributor

@fmatthew5876 fmatthew5876 commented Dec 31, 2019

Depends on #1964

Fixes more cases of #1706

This is yet another refactor of Window_Message. I've tried to clean up the Update loop to make it more understandable.

Unfortunately the number of edge cases here is just insane. So making this code readable is a real challenge.

There are a number of notable issues

  • RPG_RT animates open/close after the main loop. Since we don't do this, I had to put in hacks to emulate the timing.
  • I've finally nailed down the exact timing (and the rationale for why it works this way) for window open, close, and the interpreter blocking.
  • There are many, many subtle edge cases previously unknown with character timing. Even more-so with speed=1. Many have been discovered and added.

For this PR I have a much improved workflow to measure accuracy. To time any message, you can setup the following:

Foreground Event:

Msg: \^
FlashScreen 31,31,31,31 0.0s (no wait)
Var1 = 0
Msg: MESSAGE TO TEST
Msg: Time \v[1]

Parallel event

Var1+=1

To get individual character timings within a message, you have to do screen captures. I am doing this with simplescreenrecorder on linux by recording RPG_RT at 60fps. Then I use ffmpeg -i vid.mkv frames%0004d.png to dump the frames to png files and look at them.

The flash command in the above event helps identify exactly the frame where the message event command is executed and pushed into the messaging system.
The above construct will measure the exact number of frames the message takes to render.

Instead of writing page after page of test cases in issues. I have a test game with lots of message tests.

The test game is here.

https://github.com/fmatthew5876/EasyRPG-InterpreterTestGame

All maps under the "Messages" Map tree have the #1706 test cases.
In particular the map Messages / MESSAGE CRAZY has a lot of NPCs with different text timing edge cases. Just talk to each NPC in RPG_RT and Player and compare.

#1706 Retested:

  • 1 - 16 Pass
  • 19 -24 Pass
  • 25 Timers retested - still working
  • 28-30 Pass
  • 31 We don't emulate system graphic corruption and shouldn't
  • 32-39 Pass

#1706 Newly fixed:

  • 26 Inn interruption tested with text, choices, and number input.

  • 40 - tested and working

  • 41 - Timings match

#1706 Still broken:

  • 17 & 18 Pushed to later PR
  • 27 Test 1 - Pushed to later PR
  • 27 Test 2 - box doesn't close.
  • 42 - We don't interrupt the message
  • 43 - Currently off by 1 frame.
  • 44 - We set show_message chunk but it gets cleared every frame due to Game_Map::UpdateForegroundEvents() always calling Game_Interpreter::Clear() even when no events run.
  • 45 Don't match

@fmatthew5876
Copy link
Contributor Author

This one is ready. There are still some minor things left. I can address them in later PRs.

This has been tested and now text timings are very accurate.

@fmatthew5876
Copy link
Contributor Author

This one just underwent rebase hell getting it over the stuff that just went in. Leaving a note for myself to do some testing of this again once dependencies are in and this is closer to being merged.

@carstene1ns
Copy link
Member

Another test for this is PeuterisGrey. After the intro some textboxes are broken in current master.

@fmatthew5876
Copy link
Contributor Author

After #2019 goes in I'll rebase this again and retest everything.

@carstene1ns
Copy link
Member

This issue is fixed here (offset):
screenshot_3

This one is still present (name/item/variable substitution):
screenshot_0 screenshot_1 screenshot_2

@carstene1ns carstene1ns added the Awaiting Rebase Pull requests with conflicting files due to former merge label Mar 3, 2020
@fmatthew5876 fmatthew5876 removed the Awaiting Rebase Pull requests with conflicting files due to former merge label Mar 3, 2020
@fmatthew5876
Copy link
Contributor Author

Rebased. Still needs to be retested.

src/window_message.cpp Outdated Show resolved Hide resolved
@Ghabry
Copy link
Member

Ghabry commented Apr 27, 2020

Well with 0.6.2 out we can now get this here in and push to Android Beta (really Beta channel, not main xD) to get some feedback.

@Ghabry
Copy link
Member

Ghabry commented May 14, 2020

Kinda message (choice) related:

There is a known regression about Inn continuation being broken when it costs money: https://github.com/rueter37/Player/commit/43a22be917b44837ea9fd9ba9d9aa8870b4239b1 (ugly solution)

The problem is that the async op is defined through a Choice and is never processed. Is this something that must be solved in messages, e.g. by passing in the async context:

Window_Message::InputChoice window_message.cpp:714
Window_Message::Update window_message.cpp:416
Game_Message::Update game_message.cpp:203
Game_Map::Update game_map.cpp:965 <- Has async ctx

Should thsi be solved on message or on interpreter side?

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented May 15, 2020

The inn regression is completely fixed in #2212

The cross section of that PR with this one is minimal. I'm fine with rebasing either one onto the other.

@carstene1ns carstene1ns added the Awaiting Rebase Pull requests with conflicting files due to former merge label May 19, 2020
* Reorganize update sequence
* Add debug logging to debug frame timing issues
* Simplify and streamline
* Add hacks to match RPG_RT open/close timing and intpreter
* Fix waits for printable and non-printable characters.

It appears that RPG_RT animates the message open and close *after*
the main loop. This causes a lot of complications for us, because much
code depends on whether the message window is visible or closing.

To workaround this difference we add close started and close finished
flags and wrap them.
Allows for edge case when \> actually causes a wait.
If a half width character printed but didn't wait, it will
always wait if proceeded by a full width char.
* Check for summon gold window before animating the message box
* Handle gold window in repeated messages
Fixes cases where inn message interrupts current text.
Fixes timing for multi-page level up messages
@fmatthew5876 fmatthew5876 removed the Awaiting Rebase Pull requests with conflicting files due to former merge label May 20, 2020
@Ghabry
Copy link
Member

Ghabry commented May 20, 2020

Inn is in(n) so can be merged.
When 0.6.2.1 is out the current master can be pushed to Android Beta channel.
I don't want to push the beta before because the internal release number for Android is based on "commits in repo" and it is possible that stable is rejected because the number is lower 🙄🤔

@Ghabry Ghabry merged commit 9330717 into EasyRPG:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants