Picture magnification lag

[gadesx]

Detected a bug that decrease the FPS a lot:

Map without pictures: 60FPS
Map with a picture with 50 of magnification: 60FPS
Map with a picture with 100 of magnification: 60FPS
Map with a picture with 120 of magnification: 17FPS
Map with a picture with 200 of magnification: 7FPS
Map with a picture with 300 of magnification: 3FPS

[fdelapena]

I've tested the map with ID 5 from:
http://dl.dropboxusercontent.com/u/22524283/rpg%20maker/EASYRPG%20TESTER%203.rar
And I get magnification issues with 200% zoom as reported. The picture is big (2400x2400) and the blending operation might being done including offscreen data instead of clipped one.
There is more than 1 picture with partial opacity loaded in this test (needs multiple blending operations).

[fdelapena]

Confirmed, after making the image smaller (640x640) performance goes up (57-58 FPS).
A possible solution may be clipping visible image before running blitting operations.

[Ghabry]

Could somebody reupload the test case for our oldest issue?

[gadesx]

maybe this one
EASYRPG TESTER 3.zip

[Ghabry]

Runs at ~40 FPS here, even when I increase the picture zoom.

Don't know if this was fixed by accident but the only real performance issue I notice is the Tone change in the 60seconds MovePicture which results in a tone recalculation every few frames. And because the pic is 2400x2400 this takes really long (compared to RPG_RT where it is a constant operation: 256 palette changes).

[gadesx]

What CPU do you have? I use a Core 2 Duo of 2.66ghz.
The mayor problem for my games it's use the big pictures of shadows, that run the game around 5fps instead 60 as normal rpg_rt, no need of magnification use, 100% only.
Maybe make the calcs of a image with 256 palette as 256 too it's better
than full colour that only make performance issues

[Ghabry]

Changing the color palette doesn't help because we convert all images to 32bit upon loading. The only way to make this faster would be doing the image operations on the GPU because the GPU is optimized for this.

I tested that on my Windows system. Has a m3-6Y30 (Max 2.2 Ghz). But probably much faster than your 2.2 Ghz core2... Will try to run it at the lowest clockspeed (900mhz) when testing.

[Albeleon]

#1380 improves Tone performance quite a bit, so maybe this is less of an issue now? Needs testing

[gadesx]

I don't know if the pictures in easyrpg its based in 32bit for make the mix with all, but if the original image format is 8 bit, its better use a mix of graphics based in the original number of colors that make all be as 32bits, cpu dependant for nothing, if 256color picture is mixed with the tileset as millions of colors

[gadesx]

Same image with 2017 player 5 FPS
with 2018 player 40 FPS
In 2k3, always 60FPS

[fdelapena]

Because blitting is not hardware accelerated (and probably 2k3 is), and because this is a clear corner case, it is probably not worth to fix remaining performance IMO, as pictures with that size + magnification on them should be rare to find. This could be reopened if some games still perform bad with some similar issue.

[Ghabry]

I just had an idea how to improve the situation a bit:

I guess many cases are "constant zooms", so only zoomed on creation and then not touched again. (so only zoom in ShowPicture, any MovePicture zoom disables the feature).

In this case the zoomed image could be precalculated (at least on our major platforms that have enough RAM).

[Ghabry]

This could work but should be more tested honestly as it touches the sprite code ^^': With a picture that is only zoomed once on creation it runs with 60 FPS. That optimisation is disabled when a second zoom operation happens as it is to be expected that this has a dynamic zoom making the caching slower than the live calculation.

Note that the example of gadesx won't run faster as it uses Move Picture to do a dynamic zoom. But I would expect e.g. that moving clouds usually do not zoom.

The code is a bit longer as I had to redo some of the Sprite-code which used lots of different rectangles and I have honestly no idea what they were all used for. After removing all of them the blitting still works fine and the code is simpler.

Also got rid of SetSpriteRect which made the calculations harder. That function is only used a single time, so I moved the calculation their.

diff --git a/src/cache.cpp b/src/cache.cpp
index a6ae6693f..dc45905c2 100644
--- a/src/cache.cpp
+++ b/src/cache.cpp
@@ -81,7 +81,7 @@ namespace {
 	std::unordered_map<tile_key_type, std::weak_ptr<Bitmap>> cache_tiles;
 
 	// rect, flip_x, flip_y, tone, blend
-	using effect_key_type = std::tuple<BitmapRef, Rect, bool, bool, Tone, Color>;
+	using effect_key_type = std::tuple<BitmapRef, bool, bool, double, double, Tone, Color>;
 	std::map<effect_key_type, std::weak_ptr<Bitmap>> cache_effects;
 
 	std::string system_name;
@@ -442,12 +442,13 @@ BitmapRef Cache::Tile(StringView filename, int tile_id) {
 	} else { return it->second.lock(); }
 }
 
-BitmapRef Cache::SpriteEffect(const BitmapRef& src_bitmap, const Rect& rect, bool flip_x, bool flip_y, const Tone& tone, const Color& blend) {
+BitmapRef Cache::SpriteEffect(const BitmapRef& src_bitmap, bool flip_x, bool flip_y, double zoom_x, double zoom_y, const Tone& tone, const Color& blend) {
 	const effect_key_type key {
 		src_bitmap,
-		rect,
 		flip_x,
 		flip_y,
+		zoom_x,
+		zoom_y,
 		tone,
 		blend
 	};
@@ -457,8 +458,10 @@ BitmapRef Cache::SpriteEffect(const BitmapRef& src_bitmap, const Rect& rect, boo
 	if (it == cache_effects.end() || it->second.expired()) {
 		BitmapRef bitmap_effects;
 
-		auto create = [&rect] () -> BitmapRef {
-			return Bitmap::Create(rect.width, rect.height, true);
+		Rect rect = src_bitmap->GetRect();
+
+		auto create = [&src_bitmap] () -> BitmapRef {
+			return Bitmap::Create(src_bitmap->GetWidth(), src_bitmap->GetHeight(), true);
 		};
 
 		if (tone != Tone()) {
@@ -486,6 +489,19 @@ BitmapRef Cache::SpriteEffect(const BitmapRef& src_bitmap, const Rect& rect, boo
 			}
 		}
 
+		if (zoom_x != 1.0 || zoom_y != 1.0) {
+			int width = static_cast<int>(rect.width * zoom_x);
+			int height = static_cast<int>(rect.height * zoom_y);
+			if (!bitmap_effects) {
+				bitmap_effects = Bitmap::Create(width, height, true);
+				bitmap_effects->ZoomOpacityBlit(0, 0, 0, 0, *src_bitmap, src_bitmap->GetRect(), zoom_x, zoom_y, Opacity::Opaque());
+			} else {
+				auto bitmap_effects_zoomed = Bitmap::Create(width, height, true);
+				bitmap_effects_zoomed->ZoomOpacityBlit(0, 0, 0, 0, *bitmap_effects, bitmap_effects->GetRect(), zoom_x, zoom_y, Opacity::Opaque());
+				bitmap_effects = bitmap_effects_zoomed;
+			}
+		}
+
 		assert(bitmap_effects && "Effect cache used but no effect applied!");
 
 		return(cache_effects[key] = bitmap_effects).lock();
diff --git a/src/cache.h b/src/cache.h
index ba6a523a3..3fa878f08 100644
--- a/src/cache.h
+++ b/src/cache.h
@@ -55,7 +55,7 @@ namespace Cache {
 	BitmapRef System2(StringView filename);
 
 	BitmapRef Tile(StringView filename, int tile_id);
-	BitmapRef SpriteEffect(const BitmapRef& src_bitmap, const Rect& rect, bool flip_x, bool flip_y, const Tone& tone, const Color& blend);
+	BitmapRef SpriteEffect(const BitmapRef& src_bitmap, bool flip_x, bool flip_y, double zoom_x, double zoom_y, const Tone& tone, const Color& blend);
 
 	void Clear();
 	void ClearAll();
diff --git a/src/sprite.cpp b/src/sprite.cpp
index 16203e9fc..f23b8e599 100644
--- a/src/sprite.cpp
+++ b/src/sprite.cpp
@@ -41,19 +41,21 @@ void Sprite::BlitScreen(Bitmap& dst) {
 	if (!bitmap || (opacity_top_effect <= 0 && opacity_bottom_effect <= 0))
 		return;
 
-	BitmapRef draw_bitmap = Refresh(src_rect_effect);
+	BitmapRef draw_bitmap = Refresh();
 	if (!draw_bitmap) {
 		return;
 	}
 
 	bitmap_changed = false;
 
-	Rect rect = src_rect_effect.GetSubRect(src_rect);
+	Rect rect = src_rect;
 	if (draw_bitmap == bitmap_effects) {
-		// When a "sprite rect" (src_rect_effect) is used bitmap_effects
-		// only has the size of this subrect instead of the whole bitmap
-		rect.x %= bitmap_effects->GetWidth();
-		rect.y %= bitmap_effects->GetHeight();
+		if (zoom_effect_cache) {
+			rect.x *= zoom_x_effect;
+			rect.y *= zoom_y_effect;
+			rect.width *= zoom_x_effect;
+			rect.height *= zoom_y_effect;
+		}
 	}
 
 	BlitScreenIntern(dst, *draw_bitmap, rect);
@@ -61,16 +63,20 @@ void Sprite::BlitScreen(Bitmap& dst) {
 
 void Sprite::BlitScreenIntern(Bitmap& dst, Bitmap const& draw_bitmap, Rect const& src_rect) const
 {
-	double zoom_x = zoom_x_effect;
-	double zoom_y = zoom_y_effect;
-
-	dst.EffectsBlit(x, y, ox, oy, draw_bitmap, src_rect,
+	if (zoom_effect_cache) {
+		dst.EffectsBlit(x, y, ox * zoom_x_effect, oy * zoom_y_effect, draw_bitmap, src_rect,
 		Opacity(opacity_top_effect, opacity_bottom_effect, bush_effect),
-		zoom_x, zoom_y, angle_effect,
+		1.0, 1.0, angle_effect,
 		waver_effect_depth, waver_effect_phase, static_cast<Bitmap::BlendMode>(blend_type_effect));
+	} else {
+		dst.EffectsBlit(x, y, ox, oy, draw_bitmap, src_rect,
+			Opacity(opacity_top_effect, opacity_bottom_effect, bush_effect),
+			zoom_x_effect, zoom_y_effect, angle_effect,
+			waver_effect_depth, waver_effect_phase, static_cast<Bitmap::BlendMode>(blend_type_effect));
+	}
 }
 
-BitmapRef Sprite::Refresh(Rect& rect) {
+BitmapRef Sprite::Refresh() {
 	if (zoom_x_effect == 1.0 && zoom_y_effect == 1.0 && angle_effect == 0.0 && waver_effect_depth == 0) {
 		// Prevent effect sprite creation when not in the viewport
 		// TODO: Out of bounds math adjustments for zoom, angle and waver
@@ -80,19 +86,18 @@ BitmapRef Sprite::Refresh(Rect& rect) {
 		}
 	}
 
-	rect.Adjust(bitmap->GetWidth(), bitmap->GetHeight());
-
 	bool no_tone = tone_effect == Tone();
 	bool no_flash = flash_effect.alpha == 0;
 	bool no_flip = !flipx_effect && !flipy_effect;
-	bool no_effects = no_tone && no_flash && no_flip;
+	bool no_zoom = !zoom_effect_cache || (zoom_x_effect == 1.0 && zoom_y_effect == 1.0);
+	bool no_effects = no_tone && no_flash && no_flip && no_zoom;
 	bool effects_changed = tone_effect != current_tone ||
 		flash_effect != current_flash ||
 		flipx_effect != current_flip_x ||
-		flipy_effect != current_flip_y;
-	bool effects_rect_changed = rect != bitmap_effects_src_rect;
+		flipy_effect != current_flip_y ||
+		zoom_effect_cache && (zoom_x_effect != current_zoom_x || zoom_y_effect != current_zoom_y);
 
-	if (no_effects || effects_changed || effects_rect_changed || bitmap_changed) {
+	if (no_effects || effects_changed || bitmap_changed) {
 		bitmap_effects.reset();
 	}
 
@@ -105,9 +110,18 @@ BitmapRef Sprite::Refresh(Rect& rect) {
 		current_flash = flash_effect;
 		current_flip_x = flipx_effect;
 		current_flip_y = flipy_effect;
+		current_zoom_x = zoom_x_effect;
+		current_zoom_y = zoom_y_effect;
 
-		bitmap_effects = Cache::SpriteEffect(bitmap, rect, flipx_effect, flipy_effect, current_tone, current_flash);
-		bitmap_effects_src_rect = rect;
+		double zx = current_zoom_x;
+		double zy = current_zoom_y;
+
+		if (no_zoom) {
+			zx = 1.0;
+			zy = 1.0;
+		}
+
+		bitmap_effects = Cache::SpriteEffect(bitmap, flipx_effect, flipy_effect, zx, zy, current_tone, current_flash);
 
 		return bitmap_effects;
 	}
@@ -121,8 +135,6 @@ void Sprite::SetBitmap(BitmapRef const& nbitmap) {
 		src_rect = bitmap->GetRect();
 	}
 
-	src_rect_effect = src_rect;
-
 	bitmap_changed = true;
 }
 
@@ -136,4 +148,3 @@ void Sprite::SetOpacity(int opacity_top, int opacity_bottom) {
 		opacity_bottom_effect = opacity_bottom;
 	}
 }
-
diff --git a/src/sprite.h b/src/sprite.h
index 3e0857f8d..62b494efe 100644
--- a/src/sprite.h
+++ b/src/sprite.h
@@ -24,6 +24,7 @@
 #include "memory_management.h"
 #include "rect.h"
 #include "tone.h"
+#include <cassert>
 
 /**
  * Sprite class.
@@ -41,7 +42,6 @@ public:
 	void SetBitmap(BitmapRef const& bitmap);
 	Rect const& GetSrcRect() const;
 	void SetSrcRect(Rect const& src_rect);
-	void SetSpriteRect(Rect const& sprite_rect);
 	int GetX() const;
 	void SetX(int x);
 	int GetY() const;
@@ -102,6 +102,18 @@ public:
 	 */
 	void SetFlashEffect(const Color &color);
 
+protected:
+	bool GetZoomEffectCache() const;
+
+	/**
+	 * Configures whether the effect cache includes the zoom effect.
+	 * This is useful when the image is only zoomed once on load.
+	 * When the zoom is changed while the cache is active the Player will abort.
+	 *
+	 * @param enable Whether to enable the zoom cache.
+	 */
+	void SetZoomEffectCache(bool enable);
+
 private:
 	BitmapRef bitmap;
 
@@ -111,7 +123,6 @@ private:
 	int ox = 0;
 	int oy = 0;
 
-	Rect src_rect_effect;
 	int opacity_top_effect = 255;
 	int opacity_bottom_effect = 128;
 	int bush_effect = 0;
@@ -127,20 +138,21 @@ private:
 
 	BitmapRef bitmap_effects;
 
-	Rect bitmap_effects_src_rect;
-
 	Tone current_tone;
 	Color current_flash;
 	bool flipx_effect = false;
 	bool flipy_effect = false;
 	bool current_flip_x = false;
 	bool current_flip_y = false;
+	double current_zoom_x = 1.0;
+	double current_zoom_y = 1.0;
+	bool zoom_effect_cache = false;
 	bool bitmap_changed = true;
 
 	void BlitScreen(Bitmap& dst);
 	void BlitScreenIntern(Bitmap& dst, Bitmap const& draw_bitmap,
 							Rect const& src_rect) const;
-	BitmapRef Refresh(Rect& rect);
+	BitmapRef Refresh();
 };
 
 inline int Sprite::GetWidth() const {
@@ -204,10 +216,12 @@ inline double Sprite::GetZoomY() const {
 }
 
 inline void Sprite::SetZoomX(double zoom_x) {
+	assert(!zoom_effect_cache || zoom_effect_cache && zoom_x == zoom_x_effect);
 	zoom_x_effect = zoom_x;
 }
 
 inline void Sprite::SetZoomY(double zoom_y) {
+	assert(!zoom_effect_cache || zoom_effect_cache && zoom_y == zoom_y_effect);
 	zoom_y_effect = zoom_y;
 }
 
@@ -275,10 +289,6 @@ inline void Sprite::SetWaverPhase(double phase) {
 	waver_effect_phase = phase;
 }
 
-inline void Sprite::SetSpriteRect(Rect const& nsprite_rect) {
-	src_rect_effect = nsprite_rect;
-}
-
 inline void Sprite::SetFlipX(bool flipx) {
 	flipx_effect = flipx;
 }
@@ -295,4 +305,12 @@ inline void Sprite::SetFlashEffect(const Color &color) {
 	flash_effect = color;
 }
 
+inline bool Sprite::GetZoomEffectCache() const {
+	return zoom_effect_cache;
+}
+
+inline void Sprite::SetZoomEffectCache(bool enable) {
+	zoom_effect_cache = enable;
+}
+
 #endif
diff --git a/src/sprite_character.cpp b/src/sprite_character.cpp
index d291b5ad5..702b38b75 100644
--- a/src/sprite_character.cpp
+++ b/src/sprite_character.cpp
@@ -67,7 +67,7 @@ void Sprite_Character::Update() {
 		int row = character->GetFacing();
 		auto frame = character->GetAnimFrame();
 		if (frame >= lcf::rpg::EventPage::Frame_middle2) frame = lcf::rpg::EventPage::Frame_middle;
-		SetSrcRect({frame * chara_width, row * chara_height, chara_width, chara_height});
+		SetSrcRect({frame * chara_width + sprite_rect.x, row * chara_height + sprite_rect.y, chara_width, chara_height});
 	}
 
 	SetFlashEffect(character->GetFlashColor());
@@ -147,7 +147,7 @@ void Sprite_Character::OnCharSpriteReady(FileRequestResult*) {
 	chara_height = rect.height / 4;
 	SetOx(chara_width / 2);
 	SetOy(chara_height);
-	SetSpriteRect(rect);
+	sprite_rect = rect;
 
 	Update();
 }
diff --git a/src/sprite_character.h b/src/sprite_character.h
index 16920c48d..648a8cb3c 100644
--- a/src/sprite_character.h
+++ b/src/sprite_character.h
@@ -102,6 +102,8 @@ private:
 	void OnCharSpriteReady(FileRequestResult*);
 
 	FileRequestBinding request_id;
+
+	Rect sprite_rect;
 };
 
 #endif
diff --git a/src/sprite_picture.cpp b/src/sprite_picture.cpp
index c4b15771c..3c1553a18 100644
--- a/src/sprite_picture.cpp
+++ b/src/sprite_picture.cpp
@@ -43,6 +43,16 @@ void Sprite_Picture::OnPictureShow() {
 	const bool is_battle = Game_Battle::IsBattleRunning();
 	const auto& pic = Main_Data::game_pictures->GetPicture(pic_id);
 
+	// Zoom caching can only be enabled for the initial zoom level
+	// Afterwards caching is disallowed for memory & performance reasons
+	initial_magnify = pic.data.current_magnify;
+	SetZoomEffectCache(false);
+	if (initial_magnify != 100) {
+		SetZoomX(initial_magnify / 100.0);
+		SetZoomY(initial_magnify / 100.0);
+		SetZoomEffectCache(true);
+	}
+
 	if (feature_priority_layers) {
 		// Battle Animations are above pictures
 		Drawable::Z_t priority;
@@ -98,6 +108,10 @@ void Sprite_Picture::Draw(Bitmap& dst) {
 
 	SetX(x);
 	SetY(y);
+
+	if (GetZoomEffectCache() && initial_magnify != data.current_magnify) {
+		SetZoomEffectCache(false);
+	}
 	SetZoomX(data.current_magnify / 100.0);
 	SetZoomY(data.current_magnify / 100.0);
 
diff --git a/src/sprite_picture.h b/src/sprite_picture.h
index 7db8c9785..b9dda4d93 100644
--- a/src/sprite_picture.h
+++ b/src/sprite_picture.h
@@ -44,6 +44,7 @@ private:
 	const bool feature_spritesheet = false;
 	const bool feature_priority_layers = false;
 	const bool feature_bottom_trans = false;
+	int initial_magnify = -1;
 };
 
 #endif