SCI32: Clean up kIsOnMe and fix rounding bug

The implementation was not correctly rounding the scaled position
with mulru, leading to occasionally incorrect hit detection at
the boundaries of boxes.

5 files changed, 50 insertions, 59 deletions

diff --git a/engines/sci/engine/kgraphics32.cpp b/engines/sci/engine/kgraphics32.cpp
index 2a9005dab4..3a59ea5037 100644
--- a/engines/sci/engine/kgraphics32.cpp
+++ b/engines/sci/engine/kgraphics32.cpp
@@ -152,12 +152,12 @@ reg_t kObjectIntersect(EngineState *s, int argc, reg_t *argv) {
 
 // Tests if the coordinate is on the passed object
 reg_t kIsOnMe(EngineState *s, int argc, reg_t *argv) {
-	uint16 x = argv[0].toUint16();
-	uint16 y = argv[1].toUint16();
-	reg_t targetObject = argv[2];
-	uint16 checkPixels = argv[3].getOffset();
+	int16 x = argv[0].toSint16();
+	int16 y = argv[1].toSint16();
+	reg_t object = argv[2];
+	bool checkPixel = argv[3].toSint16();
 
-	return make_reg(0, g_sci->_gfxFrameout->kernelIsOnMe(x, y, checkPixels, targetObject));
+	return g_sci->_gfxFrameout->kernelIsOnMe(object, Common::Point(x, y), checkPixel);
 }
 
 reg_t kCreateTextBitmap(EngineState *s, int argc, reg_t *argv) {
diff --git a/engines/sci/graphics/frameout.cpp b/engines/sci/graphics/frameout.cpp
index 329b680f17..655e59de00 100644
--- a/engines/sci/graphics/frameout.cpp
+++ b/engines/sci/graphics/frameout.cpp
@@ -1977,73 +1977,55 @@ void GfxFrameout::kernelFrameOut(const bool shouldShowBits) {
 	}
 }
 
-uint16 GfxFrameout::kernelIsOnMe(int16 x, int16 y, uint16 checkPixels, reg_t screenObject) {
-	reg_t planeObject = readSelector(_segMan, screenObject, SELECTOR(plane));
-	Plane *screenItemPlane = _visiblePlanes.findByObject(planeObject); // Search for plane in visible planes
-	ScreenItem *screenItem = nullptr;
+#pragma mark -
+#pragma mark Mouse cursor
 
-	if (!screenItemPlane) {
-		// Specified plane not found
-		return 0;
+reg_t GfxFrameout::kernelIsOnMe(const reg_t object, const Common::Point &position, bool checkPixel) const {
+	reg_t planeObject = readSelector(_segMan, object, SELECTOR(plane));
+	Plane *plane = _visiblePlanes.findByObject(planeObject);
+	if (plane == nullptr) {
+		return make_reg(0, 0);
 	}
 
-	screenItem = screenItemPlane->_screenItemList.findByObject(screenObject);
-	if (!screenItem) {
-		// Specified screen object not in item list
-		return 0;
+	ScreenItem *screenItem = plane->_screenItemList.findByObject(object);
+	if (screenItem == nullptr) {
+		return make_reg(0, 0);
 	}
 
-	// Original SCI32 seems to have made a copy (?) of the screenitem?
-	// there is also a "or [ebp+arg_56], 1 - not sure what that did
-	return isOnMe(screenItemPlane, screenItem, x, y, checkPixels);
+	return make_reg(0, isOnMe(*screenItem, *plane, position, checkPixel));
 }
 
-uint16 GfxFrameout::isOnMe(Plane *screenItemPlane, ScreenItem *screenItem, int16 x, int16 y, uint16 checkPixels) {
-	// adjust coordinate according to resolution 
-	int32 adjustedX = x * getCurrentBuffer().screenWidth / getCurrentBuffer().scriptWidth;
-	int32 adjustedY = y * getCurrentBuffer().screenHeight / getCurrentBuffer().scriptHeight;
-
-	adjustedX += screenItemPlane->_planeRect.left;
-	adjustedY += screenItemPlane->_planeRect.top;
+bool GfxFrameout::isOnMe(const ScreenItem &screenItem, const Plane &plane, const Common::Point &position, const bool checkPixel) const {
 
-	//warning("kIsOnMe %s %d (%d, %d -> %d, %d) mouse %d, %d", _segMan->getObjectName(screenObject), checkPixels, screenItem->_screenRect.left, screenItem->_screenRect.top, screenItem->_screenRect.right, screenItem->_screenRect.bottom, adjustedX, adjustedY);
+	Common::Point scaledPosition(position);
+	mulru(scaledPosition, Ratio(_currentBuffer.screenWidth, _currentBuffer.scriptWidth), Ratio(_currentBuffer.screenHeight, _currentBuffer.scriptHeight));
+	scaledPosition.x += plane._planeRect.left;
+	scaledPosition.y += plane._planeRect.top;
 
-	if (!screenItem->_screenRect.contains(adjustedX, adjustedY)) {
-		// Specified coordinates are not within screen item
-		return 0;
+	if (!screenItem._screenRect.contains(scaledPosition)) {
+		return false;
 	}
 
-	//warning("HIT!");
-	if (checkPixels) {
-		//warning("Check Pixels");
-		CelObj &screenItemCelObject = screenItem->getCelObj();
+	if (checkPixel) {
+		CelObj &celObj = screenItem.getCelObj();
 
-		Common::Point celAdjustedPoint(adjustedX, adjustedY);
-		bool  celMirrored = screenItem->_mirrorX ^ screenItemCelObject._mirrorX;
+		bool mirrorX = screenItem._mirrorX ^ celObj._mirrorX;
 
-		celAdjustedPoint.x -= screenItem->_scaledPosition.x;
-		celAdjustedPoint.y -= screenItem->_scaledPosition.y;
+		scaledPosition.x -= screenItem._scaledPosition.x;
+		scaledPosition.y -= screenItem._scaledPosition.y;
 
-		Ratio celAdjustXRatio(screenItemCelObject._scaledWidth, getCurrentBuffer().screenWidth);
-		Ratio celAdjustYRatio(screenItemCelObject._scaledHeight, getCurrentBuffer().screenHeight);
-		mulru(celAdjustedPoint, celAdjustXRatio, celAdjustYRatio);
+		mulru(scaledPosition, Ratio(celObj._scaledWidth, _currentBuffer.screenWidth), Ratio(celObj._scaledHeight, _currentBuffer.screenHeight));
 
-		if ((screenItem->_scale.signal) && (screenItem->_scale.x) && (screenItem->_scale.y)) {
-			// Apply scaling
-			celAdjustedPoint.x = celAdjustedPoint.x * 128 / screenItem->_scale.x;
-			celAdjustedPoint.y = celAdjustedPoint.y * 128 / screenItem->_scale.y;
+		if (screenItem._scale.signal != kScaleSignalNone && screenItem._scale.x && screenItem._scale.y) {
+			scaledPosition.x = scaledPosition.x * 128 / screenItem._scale.x;
+			scaledPosition.y = scaledPosition.y * 128 / screenItem._scale.y;
 		}
 
-		byte coordinateColor = screenItemCelObject.readPixel(celAdjustedPoint.x, celAdjustedPoint.y, celMirrored);
-		byte transparentColor = screenItemCelObject._transparentColor;
-
-		if (coordinateColor == transparentColor) {
-			// Coordinate is transparent
-			//warning("TRANSPARENT!");
-			return 0;
-		}
+		uint8 pixel = celObj.readPixel(scaledPosition.x, scaledPosition.y, mirrorX);
+		return pixel != celObj._transparentColor;
 	}
-	return 1;
+
+	return true;
 }
 
 #pragma mark -
diff --git a/engines/sci/graphics/frameout.h b/engines/sci/graphics/frameout.h
index 2931fc090b..f864abc5bc 100644
--- a/engines/sci/graphics/frameout.h
+++ b/engines/sci/graphics/frameout.h
@@ -502,8 +502,17 @@ public:
 		return 1;
 	};
 
-	uint16 kernelIsOnMe(int16 x, int16 y, uint16 checkPixels, reg_t screenObject);
-	uint16 isOnMe(Plane *screenItemPlane, ScreenItem *screenItem, int16 x, int16 y, uint16 checkPixels);
+#pragma mark -
+#pragma mark Mouse cursor
+private:
+	/**
+	 * Determines whether or not the point given by
+	 * `position` is inside of the given screen item.
+	 */
+	bool isOnMe(const ScreenItem &screenItem, const Plane &plane, const Common::Point &position, const bool checkPixel) const;
+
+public:
+	reg_t kernelIsOnMe(const reg_t object, const Common::Point &position, const bool checkPixel) const;
 
 #pragma mark -
 #pragma mark Debugging
diff --git a/engines/sci/graphics/screen_item32.cpp b/engines/sci/graphics/screen_item32.cpp
index f3f397d632..138a0e47b0 100644
--- a/engines/sci/graphics/screen_item32.cpp
+++ b/engines/sci/graphics/screen_item32.cpp
@@ -399,7 +399,7 @@ void ScreenItem::calcRects(const Plane &plane) {
 	}
 }
 
-CelObj &ScreenItem::getCelObj() {
+CelObj &ScreenItem::getCelObj() const {
 	if (_celObj == nullptr) {
 		switch (_celInfo.type) {
 			case kCelTypeView:
diff --git a/engines/sci/graphics/screen_item32.h b/engines/sci/graphics/screen_item32.h
index 0840570721..06e1f6072f 100644
--- a/engines/sci/graphics/screen_item32.h
+++ b/engines/sci/graphics/screen_item32.h
@@ -125,7 +125,7 @@ public:
 	 * item. This member is populated by calling
 	 * `getCelObj`.
 	 */
-	CelObj *_celObj;
+	mutable CelObj *_celObj;
 
 	/**
 	 * If set, the priority for this screen item is fixed
@@ -250,7 +250,7 @@ public:
 	 * screen item. If a cel object does not already exist,
 	 * one will be created and assigned.
 	 */
-	CelObj &getCelObj();
+	CelObj &getCelObj() const;
 
 	void printDebugInfo(Console *con) const;