Effects: Use the filename for the cache, not the bitmap pointer

The pointer can be reused by a new bitmap and cause wrong animations.

For individual tiles the filename is lost. "filename" in bitmap was renamed
to "id" and gets now filename+tileid in that case.

Fix #3256

Co-Authored-By: Viet Dinh <[email protected]>

src/bitmap.cpp

@@ -129,7 +129,7 @@ Bitmap::Bitmap(Filesystem_Stream::InputStream stream, bool transparent, uint32_t
 
 	original_bpp = image_out.bpp;
 
-	filename = ToString(stream.GetName());
+	id = ToString(stream.GetName());
 }
 
 Bitmap::Bitmap(const uint8_t* data, unsigned bytes, bool transparent, uint32_t flags) {

src/bitmap.h

@@ -217,12 +217,21 @@ class Bitmap {
 	Color GetShadowColor() const;
 
 	/**
-	 * Gets the filename this bitmap was loaded from.
-	 * This will be empty when the origin was not a file.
+	 * Returns an identifier for the bitmap.
+	 * When the bitmap was loaded from a file this contains the filename.
+	 * In all other cases this is implementation defined (and can be empty).
 	 *
-	 * @return filename
+	 * @return Bitmap identifier
 	 */
-	StringView GetFilename() const;
+	StringView GetId() const;
+
+	/**
+	 * Sets the identifier of the bitmap.
+	 * To avoid bugs the function will reject changing non-empty IDs.
+	 *
+	 * @param id new identifier
+	 */
+	void SetId(std::string id);
 
 	/**
 	 * Gets bpp of the source image.
@@ -608,7 +617,7 @@ class Bitmap {
 	Color bg_color, sh_color;
 	FontRef font;
 
-	std::string filename;
+	std::string id;
 
 	/** Bpp of the source image */
 	int original_bpp;
@@ -689,8 +698,13 @@ inline bool Bitmap::GetTransparent() const {
 	return format.alpha_type != PF::NoAlpha;
 }
 
-inline StringView Bitmap::GetFilename() const {
-	return filename;
+inline StringView Bitmap::GetId() const {
+	return id;
+}
+
+inline void Bitmap::SetId(std::string id) {
+	assert(this->id.empty());
+	this->id = id;
 }
 
 inline FontRef Bitmap::GetFont() const {

src/cache.cpp

@@ -82,7 +82,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<Bitmap*, Rect, bool, bool, Tone, Color>;
+	using effect_key_type = std::tuple<std::string, Rect, bool, bool, Tone, Color>;
 	std::map<effect_key_type, std::weak_ptr<Bitmap>> cache_effects;
 
 	std::string system_name;
@@ -448,13 +448,17 @@ BitmapRef Cache::Tile(StringView filename, int tile_id) {
 		rect.x += sub_tile_id % 6 * 16;
 		rect.y += sub_tile_id / 6 * 16;
 
-		return(cache_tiles[key] = Bitmap::Create(*chipset, rect)).lock();
+		auto bmp = Bitmap::Create(*chipset, rect);
+		bmp->SetId(fmt::format("{}/{}", chipset->GetId(), tile_id));
+		cache_tiles[key] = bmp;
+
+		return bmp;
 	} 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) {
 	const effect_key_type key {
-		src_bitmap.get(),
+		src_bitmap->GetId(),
 		rect,
 		flip_x,
 		flip_y,
@@ -464,6 +468,8 @@ BitmapRef Cache::SpriteEffect(const BitmapRef& src_bitmap, const Rect& rect, boo
 
 	const auto it = cache_effects.find(key);
 
+	Output::Debug("{} {}", (void*)src_bitmap.get(), src_bitmap->GetId());
+
 	if (it == cache_effects.end() || it->second.expired()) {
 		BitmapRef bitmap_effects;