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

ToneBlit performance and Method Implementation, Sprite Flashes, other fixes #1380

Merged
merged 4 commits into from Jul 11, 2018

Conversation

Projects
None yet
4 participants
@Albeleon
Member

Albeleon commented Jun 24, 2018

The issues: ToneBlit is not very efficient, Battle tones and Gradual TintScreen are tinted differently, and Sprite flashes.

ToneBlit is very heavy performance-wise, which forces us to use two different ways to implement tintscreen in our screen:

  1. We use ToneBlit for any individual tile, background and sprite when they're drawn.
  2. After all the drawables are drawn, we use ToneBlit to the screen.
  • The 1st method, with a little modification, allows us to generate Sprite flashes that ignore the tintscreen (for example, a white flash with a dark-red tintscreen, it should appear completely white instead of red-ish). The 2nd doesn't allow that.
  • The 1st method is more efficient when the tintscreen remains static, but slower when the tone screen is gradually changing.
  • The 2nd is used for Gradual Tintscreen and Battle scenes. The 1st one is used anywhere else.

The ideal solution would be using the first to fix the flash bug. For that, we try to make the ToneBlit function more efficient. It sacrifices legibility in order to gain a little more of performance. These changes (cache variables, combining checks, etc.) improve the performance so there's not much difference for the 1st and 2nd method when a gradual tone screen happens, and therefore the 1st method is viable for everything.

To implement this:

  • Bitmap: We modify the ToneBlit to make it more efficient.
  • Sprite: We invert the positions of ToneBlit and BlendBlit. This way the flash color will have priority and not be tinted.
  • Spriteset_Map: Now in all the cases it uses the 1st method, so the code that allows the 2nd method is deleted.
  • Spriteset_Battle: We change from the 2nd method to the 1st one. We set the tone for the background and the battler sprites.
  • Background: We include the option of setting and getting a tone, and then draw it.
  • Screen: We eliminate the tone functions since no other class uses them.

Also, another fixes:

  • Game_Character flashes were limited to 128 no matter what the actual value was. An attribute flash_alpha has been added to account for this, and when the flash is set and invoked in its subclasses (Event, Vehicle, Player) we use that attribute.
  • Battle_Animation "flash_length" has been changed from 5 to 12, because the way it was before it was almost unnoticeable in comparison with RPG_RT.
  • Battle_Animation and Game_Interpreter_Map: The value of the flashes before with the << 3 operation meant the flash value for each part would be [0, 248]. With "x * 255 / 31", the values is really [0, 255].

Fix #1225

@Albeleon

This comment has been minimized.

Member

Albeleon commented Jun 24, 2018

#1371 The original reason why I checked this:
"4.12: RM2000 (not tested in RM2003): Maybe it's my impression but when an attack animation shows up to an enemy, shouldn't the enemy flashes inside the animation (not afterwards) be clearer? In the video, sometimes they don't appear."

@Ghabry

This comment has been minimized.

Member

Ghabry commented Jun 24, 2018

Could you split the single commit in two?
The bitmap.cpp change is completely unrelated to the rest.

The easiest way to do this hear would be doing "amend commit" and unchecking "bitmap.cpp". Then commiting "bitmap.cpp" using an additional commit.

pixels[j] = ((uint32_t)hard_light_lookup[tone.red][(pixel >> rs) & 0xFF] << rs)
| ((uint32_t)hard_light_lookup[tone.green][(pixel >> gs) & 0xFF] << gs)
| ((uint32_t)hard_light_lookup[tone.blue][(pixel >> bs) & 0xFF] << bs)
| ((uint32_t)((pixel >> as) & 0xFF) << as);

This comment has been minimized.

@Ghabry

Ghabry Jun 24, 2018

Member

The problem with the different branches is now that there is lots of code duplication. Could you extract common code into "static inline" functions for improved readability.

// Only needed here, other codepaths are sanity checked by pixman
if (x < 0 || y < 0 || x >= width() || y >= height()) {
return;
}
if (&src != this) {
pixman_image_composite32(src.GetOperator(), src.bitmap, (pixman_image_t*)NULL, bitmap, src_rect.x, src_rect.y, 0, 0, x, y, src_rect.width, src_rect.height);

This comment has been minimized.

@Ghabry

Ghabry Jun 24, 2018

Member

this was on 7 lines before, now it's on one long.

@Ghabry

This comment has been minimized.

Member

Ghabry commented Jun 24, 2018

For more context: The Tone/Saturation code had a optimisation that decided if the change is a gradient or static.
See #1017

But this change had visual issues e.g. for weather effects (can't find the issue number) and needs testing if @Albeleon fixed it.

In general the assumption that there is a "screen layer" and everything below it is affected by tone doesn't hold true anymore since Cherrys RPG2k3E release because each picture can be individually configured if it is affected by tone.

This PR needs some benchmarking.

@Ghabry

This comment has been minimized.

Member

Ghabry commented Jun 25, 2018

This also fixes #1225

@carstene1ns carstene1ns added this to the 0.5.4 milestone Jul 1, 2018

@Ghabry

This comment has been minimized.

Member

Ghabry commented Jul 2, 2018

This also needs a rebase on upstrema to fix the Android build failure.

Albeleon added some commits Jun 24, 2018

Implement "ToneBlit for individual elements", Fix sprite flash with t…
…one.

ToneBlit is very heavy performance-wise, which forced us to use two different ways to implement tintscreen in our screen:
  - 1st method: "ToneBlit for individual elements (tiles, background, weather, sprites, etc.) when they're drawn": when tintscreen remains static it was more efficient, but slower when the tone screen gradually changes. This method allows us to fix a Sprite flash bug (a sprite flash shouldn't be tinted by the screen tone; i.e. a white flash with a dark tintscreen should stay white).
  - 2nd method: "After all the drawables are drawn, ToneBlit to the screen": it's used during battles and when a tintscreen gradually changes, in this last case to improve performance.

The ideal solution would be using the 1st method to fix the flash bug. To implement it:
 - Sprite: We invert the positions of ToneBlit and BlendBlit. This way the flash color will have priority and not be tinted in the 1st method.
 - Spriteset_Map: Now in all the cases it uses the 1st method, so the code that allows the 2nd method is deleted.
 - Spriteset_Battle: We change from the 2nd method to the 1st one. We set the tone for the background and the battler sprites.
 - Background: We include the option of setting and getting a tone, and then draw it.
 - Screen: We eliminate the tone functions since no other class uses them.
Improve Bitmap::ToneBlit performance.
In order to make ToneBlit more efficient we need to make a few changes, some of them reducing readability to gain the minimal performance:
 - We create a different "for" depending of whether it should ignore "alpha checking" or not, instead of inside the for, and reduces branches inside.
 - We cache the pitch() value to avoid having to calculate the function inside each for and do a division operation with sizeof().
 - We combine the check of each "for" by cache the value beforehand (limit_width, limit_height).
 - In case both saturation and color operations are done, we create a unique branch where they share the same for avoid having to do the iteration twice.
 - We use uint16_t for the index in the "for".
 - To increase readability after so many duplicates and branches, we create static inline saturation_tone and color_tone which changes a pixel's color with those parameters. By being inline instead of a function, it improves performance.
Correcting some flash issues (fix for #1371 4.13):
 - Game_Character flashes were limited to 128 no matter what the actual value was. An attribute "flash_alpha" has been added to account for this, and when the flash is set and invoked in its subclasses (Event, Vehicle, Player) we use that attribute.
 - Battle_Animation "flash_length" has been changed from 5 to 12, because the way it was before it was almost unnoticeable in comparison with RPG_RT.
 - Battle_Animation and Game_Interpreter_Map: The translation of the flash values [0,31] with "<< 3" operation meant the flash value for each part would be [0, 248]. With "x * 255 / 31", the values are [0, 255].
Blit Weather images without showing the transparent pixels:
The reason why they were printed was because Weather uses ToneBlit over itself, which the way bitmap.cpp deals with means it shouldn't skip pixels with alpha 0. To solve it, we add to ToneBlit an optional input bool "check_alpha" (default false), which will check that parameter even if "&src == this". We apply it to the rain and snow (check_alpha = true).
@Ghabry

This comment has been minimized.

Member

Ghabry commented Jul 2, 2018

For benchmarking you can use this patch which unlocks the FPS but still keeps the updates at 60:

diff --git a/src/player.cpp b/src/player.cpp
index f646bc9c..d39cac16 100644
--- a/src/player.cpp
+++ b/src/player.cpp
@@ -248,7 +248,7 @@ void Player::Update(bool update_scene) {
 	static const double framerate_interval = 1000.0 / Graphics::GetDefaultFps();
 	next_frame = start_time + framerate_interval;
 
-#ifdef EMSCRIPTEN
+#if 1
 	// Ticks in emscripten are unreliable due to how the main loop works:
 	// This function is only called 60 times per second instead of theoretical
 	// 1000s of times.
@@ -303,12 +303,17 @@ void Player::Update(bool update_scene) {
 	}
 
 	Audio().Update();
-	Input::Update();
 
 	std::shared_ptr<Scene> old_instance = Scene::instance;
 
 	int speed_modifier = GetSpeedModifier();
 
+	double cur_time = (double)DisplayUi->GetTicks();
+	if (cur_time < next_frame) {
+		return;
+	}
+	Input::Update();
+
 	for (int i = 0; i < speed_modifier; ++i) {
 		Graphics::Update();
 		if (update_scene) {
@Ghabry

This comment has been minimized.

Member

Ghabry commented Jul 2, 2018

With my benchmark I get the following FPS:

Test Upstream This PR
No effect 1050 1050
Gradient finished 1050 1040
Tone gradient 650 970
Saturation gradient 623 970
Tone+Sat grad. 500 960

That "gradient finished" is in the benchmark slower then no effect is strange but because the difference is so small it could be a benchmark issue.
The FPS measurement is not fully precise, error is ±50 FPS but you can see that this PR is significantly faster in all operations.

@Ghabry

Ghabry approved these changes Jul 2, 2018

@Albeleon Albeleon referenced this pull request Jul 10, 2018

Closed

Picture magnification lag #57

@carstene1ns carstene1ns merged commit 4bebc3d into EasyRPG:master Jul 11, 2018

7 checks passed

Android (armeabi-v7a) Build finished.
Details
GCW0 Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details

@Albeleon Albeleon deleted the Albeleon:flashtone branch Jul 11, 2018

@Albeleon Albeleon referenced this pull request Jul 12, 2018

Open

Ascendence: Visual bugs #1293

2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment