From e25d928a9cb82d458c7f592ec81576f0d57d8eca Mon Sep 17 00:00:00 2001
From: Colin Snover
Date: Fri, 18 Mar 2016 13:08:37 -0500
Subject: SCI32: Fix crashes in kIsOnMe caused by stale CelObjs

---
 engines/sci/graphics/celobj32.h        |  4 ++++
 engines/sci/graphics/frameout.cpp      |  9 ++++++++-
 engines/sci/graphics/screen_item32.cpp | 12 +++++++++++-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/engines/sci/graphics/celobj32.h b/engines/sci/graphics/celobj32.h
index 600ae82d32..0bb4b03ae2 100644
--- a/engines/sci/graphics/celobj32.h
+++ b/engines/sci/graphics/celobj32.h
@@ -104,6 +104,10 @@ struct CelInfo32 {
 			bitmap == other.bitmap
 		);
 	}
+
+	inline bool operator!=(const CelInfo32 &other) {
+		return !(*this == other);
+	}
 };
 
 class CelObj;
diff --git a/engines/sci/graphics/frameout.cpp b/engines/sci/graphics/frameout.cpp
index b654cbf2b9..062cd8b600 100644
--- a/engines/sci/graphics/frameout.cpp
+++ b/engines/sci/graphics/frameout.cpp
@@ -1381,7 +1381,7 @@ void GfxFrameout::kernelFrameOut(const bool shouldShowBits) {
 #pragma mark Mouse cursor
 
 reg_t GfxFrameout::kernelIsOnMe(const reg_t object, const Common::Point &position, bool checkPixel) const {
-	reg_t planeObject = readSelector(_segMan, object, SELECTOR(plane));
+	const reg_t planeObject = readSelector(_segMan, object, SELECTOR(plane));
 	Plane *plane = _visiblePlanes.findByObject(planeObject);
 	if (plane == nullptr) {
 		return make_reg(0, 0);
@@ -1392,6 +1392,13 @@ reg_t GfxFrameout::kernelIsOnMe(const reg_t object, const Common::Point &positio
 		return make_reg(0, 0);
 	}
 
+	// NOTE: The original engine passed a copy of the ScreenItem into isOnMe
+	// as a hack around the fact that the screen items in `_visiblePlanes`
+	// did not have their `_celObj` pointers cleared when their CelInfo was
+	// updated by `Plane::decrementScreenItemArrayCounts`. We handle this
+	// this more intelligently by clearing `_celObj` in the copy assignment
+	// operator, which is only ever called by `decrementScreenItemArrayCounts`
+	// anyway.
 	return make_reg(0, isOnMe(*screenItem, *plane, position, checkPixel));
 }
 
diff --git a/engines/sci/graphics/screen_item32.cpp b/engines/sci/graphics/screen_item32.cpp
index c44d3e96f1..c3fdbb6845 100644
--- a/engines/sci/graphics/screen_item32.cpp
+++ b/engines/sci/graphics/screen_item32.cpp
@@ -115,7 +115,17 @@ _screenRect(other._screenRect) {
 }
 
 void ScreenItem::operator=(const ScreenItem &other) {
-	_celInfo = other._celInfo;
+	// NOTE: The original engine did not check for differences in `_celInfo`
+	// to clear `_celObj` here; instead, it unconditionally set `_celInfo`,
+	// didn't clear `_celObj`, and did hacky stuff in `kIsOnMe` to avoid
+	// testing a mismatched `_celObj`. See `GfxFrameout::kernelIsOnMe` for
+	// more detail.
+	if (_celInfo != other._celInfo) {
+		_celInfo = other._celInfo;
+		delete _celObj;
+		_celObj = nullptr;
+	}
+
 	_screenRect = other._screenRect;
 	_mirrorX = other._mirrorX;
 	_useInsetRect = other._useInsetRect;
-- 
cgit v1.2.3