SCI32: Clean up ScreenItem

* Rewrap comments to 80 columns
* Clarify comments where possible
* Use smart pointers where appropriate

1 files changed, 31 insertions, 51 deletions

diff --git a/engines/sci/graphics/screen_item32.cpp b/engines/sci/graphics/screen_item32.cpp
index 14e71735fb..4e1ed8399b 100644
--- a/engines/sci/graphics/screen_item32.cpp
+++ b/engines/sci/graphics/screen_item32.cpp
@@ -39,7 +39,6 @@ uint32 ScreenItem::_nextCreationId = 0;
 
 ScreenItem::ScreenItem(const reg_t object) :
 _creationId(_nextCreationId++),
-_celObj(nullptr),
 _object(object),
 _pictureId(-1),
 _created(g_sci->_gfxFrameout->getScreenCount()),
@@ -59,7 +58,6 @@ _plane(plane),
 _useInsetRect(false),
 _z(0),
 _celInfo(celInfo),
-_celObj(nullptr),
 _fixedPriority(false),
 _position(0, 0),
 _object(make_reg(0, _nextObjectId++)),
@@ -76,7 +74,6 @@ _plane(plane),
 _useInsetRect(false),
 _z(0),
 _celInfo(celInfo),
-_celObj(nullptr),
 _fixedPriority(false),
 _position(rect.left, rect.top),
 _object(make_reg(0, _nextObjectId++)),
@@ -98,7 +95,6 @@ _scale(scaleInfo),
 _useInsetRect(false),
 _z(0),
 _celInfo(celInfo),
-_celObj(nullptr),
 _fixedPriority(false),
 _position(position),
 _object(make_reg(0, _nextObjectId++)),
@@ -115,7 +111,6 @@ _plane(other._plane),
 _scale(other._scale),
 _useInsetRect(other._useInsetRect),
 _celInfo(other._celInfo),
-_celObj(nullptr),
 _object(other._object),
 _mirrorX(other._mirrorX),
 _scaledPosition(other._scaledPosition),
@@ -127,18 +122,18 @@ _drawBlackLines(other._drawBlackLines) {
 }
 
 void ScreenItem::operator=(const ScreenItem &other) {
-	// 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. kCelTypeMem types are unconditionally invalidated because
-	// the properties of a CelObjMem can "change" when a game deletes a bitmap
-	// and then creates a new one that reuses the old bitmap's offset in
-	// BitmapTable (as happens in the LSL7 About screen when hovering names).
+	// SSCI 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.
+	//
+	// kCelTypeMem types are unconditionally invalidated because the properties
+	// of a CelObjMem can "change" when a game deletes a bitmap and then creates
+	// a new one that reuses the old bitmap's offset in BitmapTable (as happens
+	// in the LSL7 About screen when hovering names).
 	if (_celInfo.type == kCelTypeMem || _celInfo != other._celInfo) {
 		_celInfo = other._celInfo;
-		delete _celObj;
-		_celObj = nullptr;
+		_celObj.reset();
 	}
 
 	_creationId = other._creationId;
@@ -153,10 +148,6 @@ void ScreenItem::operator=(const ScreenItem &other) {
 	_drawBlackLines = other._drawBlackLines;
 }
 
-ScreenItem::~ScreenItem() {
-	delete _celObj;
-}
-
 void ScreenItem::init() {
 	_nextObjectId = 20000;
 	_nextCreationId = 0;
@@ -176,17 +167,12 @@ void ScreenItem::setFromObject(SegManager *segMan, const reg_t object, const boo
 		_celInfo.celNo = readSelectorValue(segMan, object, SELECTOR(cel));
 
 		if (_celInfo.resourceId <= kPlanePic) {
-			// TODO: Enhance GfxView or ResourceManager to allow
-			// metadata for resources to be retrieved once, from a
-			// single location
 			Resource *view = g_sci->getResMan()->findResource(ResourceId(kResourceTypeView, _celInfo.resourceId), false);
 			if (!view) {
 				error("Failed to load %s", _celInfo.toString().c_str());
 			}
 
-			// NOTE: +2 because the header size field itself is excluded from
-			// the header size in the data
-			const uint16 headerSize = view->getUint16SEAt(0) + 2;
+			const uint16 headerSize = view->getUint16SEAt(0) + /* header size field */ sizeof(uint16);
 			const uint8 loopCount = view->getUint8At(2);
 			const uint8 loopSize = view->getUint8At(12);
 
@@ -225,8 +211,7 @@ void ScreenItem::setFromObject(SegManager *segMan, const reg_t object, const boo
 	}
 
 	if (updateCel || updateBitmap) {
-		delete _celObj;
-		_celObj = nullptr;
+		_celObj.reset();
 	}
 
 	if (readSelectorValue(segMan, object, SELECTOR(fixPriority))) {
@@ -305,8 +290,8 @@ void ScreenItem::calcRects(const Plane &plane) {
 		}
 
 		// Cel may use a coordinate system that is not the same size as the
-		// script coordinate system (usually this means high-resolution
-		// pictures with low-resolution scripts)
+		// script coordinate system (usually this means high-resolution pictures
+		// with low-resolution scripts)
 		if (celObj._xResolution != kLowResX || celObj._yResolution != kLowResY) {
 			// high resolution coordinates
 
@@ -386,7 +371,7 @@ void ScreenItem::calcRects(const Plane &plane) {
 
 				mulinc(temp, celToScreenX, Ratio());
 
-				CelObjPic *celObjPic = dynamic_cast<CelObjPic *>(_celObj);
+				CelObjPic *celObjPic = dynamic_cast<CelObjPic *>(_celObj.get());
 				if (celObjPic == nullptr) {
 					error("Expected a CelObjPic");
 				}
@@ -415,9 +400,8 @@ void ScreenItem::calcRects(const Plane &plane) {
 
 			if (!scaleX.isOne() || !scaleY.isOne()) {
 				mulinc(_screenItemRect, scaleX, scaleY);
-				// TODO: This was in the original code, baked into the
-				// multiplication though it is not immediately clear
-				// why this is the only one that reduces the BR corner
+				// TODO: This was in SSCI, baked into the multiplication. It is
+				// not clear why this is the only one that reduces the BR corner
 				_screenItemRect.right -= 1;
 				_screenItemRect.bottom -= 1;
 			}
@@ -434,7 +418,7 @@ void ScreenItem::calcRects(const Plane &plane) {
 					temp.right -= 1;
 				}
 
-				CelObjPic *celObjPic = dynamic_cast<CelObjPic *>(_celObj);
+				CelObjPic *celObjPic = dynamic_cast<CelObjPic *>(_celObj.get());
 				if (celObjPic == nullptr) {
 					error("Expected a CelObjPic");
 				}
@@ -487,19 +471,19 @@ void ScreenItem::calcRects(const Plane &plane) {
 }
 
 CelObj &ScreenItem::getCelObj() const {
-	if (_celObj == nullptr) {
+	if (!_celObj) {
 		switch (_celInfo.type) {
 			case kCelTypeView:
-				_celObj = new CelObjView(_celInfo.resourceId, _celInfo.loopNo, _celInfo.celNo);
+				_celObj.reset(new CelObjView(_celInfo.resourceId, _celInfo.loopNo, _celInfo.celNo));
 			break;
 			case kCelTypePic:
 				error("Internal error, pic screen item with no cel.");
 			break;
 			case kCelTypeMem:
-				_celObj = new CelObjMem(_celInfo.bitmap);
+				_celObj.reset(new CelObjMem(_celInfo.bitmap));
 			break;
 			case kCelTypeColor:
-				_celObj = new CelObjColor(_celInfo.color, _insetRect.width(), _insetRect.height());
+				_celObj.reset(new CelObjColor(_celInfo.color, _insetRect.width(), _insetRect.height()));
 			break;
 		}
 	}
@@ -534,7 +518,7 @@ void ScreenItem::printDebugInfo(Console *con) const {
 
 	con->debugPrintf("    %s\n", _celInfo.toString().c_str());
 
-	if (_celObj != nullptr) {
+	if (_celObj) {
 		con->debugPrintf("    width %d, height %d, x-resolution %d, y-resolution %d\n",
 			_celObj->_width,
 			_celObj->_height,
@@ -583,8 +567,7 @@ void ScreenItem::update() {
 	}
 	_deleted = 0;
 
-	delete _celObj;
-	_celObj = nullptr;
+	_celObj.reset();
 }
 
 Common::Rect ScreenItem::getNowSeenRect(const Plane &plane) const {
@@ -647,15 +630,13 @@ Common::Rect ScreenItem::getNowSeenRect(const Plane &plane) const {
 
 		if (!scaleX.isOne() || !scaleY.isOne()) {
 			// Different games use a different cel scaling mode, but the
-			// difference isn't consistent across SCI versions; instead,
-			// it seems to be related to an update that happened during
-			// SCI2.1mid where games started using hi-resolution game
-			// scripts
+			// difference isn't consistent across SCI versions; instead, it
+			// seems to be related to an update that happened during SCI2.1mid
+			// where games started using high-resolution game scripts
 			if (scriptWidth == kLowResX) {
 				mulinc(nsRect, scaleX, scaleY);
-				// TODO: This was in the original code, baked into the
-				// multiplication though it is not immediately clear
-				// why this is the only one that reduces the BR corner
+				// TODO: This was in SSCI, baked into the multiplication. It is
+				// not clear why this is the only one that reduces the BR corner
 				nsRect.right -= 1;
 				nsRect.bottom -= 1;
 			} else {
@@ -693,9 +674,8 @@ Common::Rect ScreenItem::getNowSeenRect(const Plane &plane) const {
 
 		if (!scaleX.isOne() || !scaleY.isOne()) {
 			mulinc(nsRect, scaleX, scaleY);
-			// TODO: This was in the original code, baked into the
-			// multiplication though it is not immediately clear
-			// why this is the only one that reduces the BR corner
+			// TODO: This was in SSCI, baked into the multiplication. It is not
+			// clear why this is the only one that reduces the BR corner
 			nsRect.right -= 1;
 			nsRect.bottom -= 1;
 		}