SCI32: Clean up ScreenItem

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

4 files changed, 109 insertions, 152 deletions

diff --git a/engines/sci/engine/object.h b/engines/sci/engine/object.h
index ffc4ac2f3d..9ab6ca34da 100644
--- a/engines/sci/engine/object.h
+++ b/engines/sci/engine/object.h
@@ -169,7 +169,6 @@ public:
 		}
 	}
 
-	// NOTE: In real engine, -info- is treated as byte size
 	void clearInfoSelectorFlag(infoSelectorFlags flag) {
 		if (getSciVersion() == SCI_VERSION_3) {
 			_infoSelectorSci3 &= ~flag;
diff --git a/engines/sci/graphics/plane32.cpp b/engines/sci/graphics/plane32.cpp
index d9e0d2d9ff..08708f2c45 100644
--- a/engines/sci/graphics/plane32.cpp
+++ b/engines/sci/graphics/plane32.cpp
@@ -201,10 +201,9 @@ void Plane::addPicInternal(const GuiResourceId pictureId, const Common::Point *p
 		} else {
 			screenItem->_position = celObj->_relativePosition;
 		}
-		_screenItemList.add(screenItem);
+		screenItem->_celObj.reset(celObj);
 
-		delete screenItem->_celObj;
-		screenItem->_celObj = celObj;
+		_screenItemList.add(screenItem);
 	}
 	_type = (g_sci->_features->hasTransparentPicturePlanes() && transparent) ? kPlaneTypeTransparentPicture : kPlaneTypePicture;
 }
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;
 		}
diff --git a/engines/sci/graphics/screen_item32.h b/engines/sci/graphics/screen_item32.h
index cdde2d3965..5acf759c32 100644
--- a/engines/sci/graphics/screen_item32.h
+++ b/engines/sci/graphics/screen_item32.h
@@ -49,22 +49,20 @@ class SegManager;
 #pragma mark ScreenItem
 
 /**
- * A ScreenItem is the engine-side representation of a
- * game script View.
+ * A ScreenItem is the engine-side representation of a game script View.
  */
 class ScreenItem {
 private:
 	/**
-	 * A serial used for screen items that are generated
-	 * inside the graphics engine, rather than the
-	 * interpreter.
+	 * A serial used for screen items that are generated inside the graphics
+	 * engine, rather than the interpreter.
 	 */
 	static uint16 _nextObjectId;
 
 	/**
-	 * A serial used to identify the creation order of
-	 * screen items, to ensure a stable sort order for
-	 * screen items with identical priorities and z-indexes.
+	 * A serial used to identify the creation order of screen items, to ensure a
+	 * stable sort order for screen items with identical priorities and
+	 * z-indexes.
 	 */
 	static uint32 _nextCreationId;
 
@@ -75,160 +73,145 @@ public:
 	reg_t _plane;
 
 	/**
-	 * Scaling data used to calculate the final screen
-	 * dimensions of the screen item as well as the scaling
-	 * ratios used when drawing the item to screen.
+	 * Scaling data used to calculate the final screen dimensions of the screen
+	 * item as well as the scaling ratios used when drawing the item to screen.
 	 */
 	ScaleInfo _scale;
 
 private:
 	/**
-	 * The position & dimensions of the screen item in
-	 * screen coordinates. This rect includes the offset
-	 * of the parent plane, but is not clipped to the
-	 * screen, so may include coordinates that are
-	 * offscreen.
+	 * The position & dimensions of the screen item in screen coordinates. This
+	 * rect includes the offset of the parent plane, but is not clipped to the
+	 * screen, so may include coordinates that are offscreen.
 	 */
 	Common::Rect _screenItemRect;
 
 	/**
-	 * If true, the `_insetRect` rectangle will be used
-	 * when calculating the dimensions of the screen item
-	 * instead of the cel's intrinsic width and height.
+	 * If true, the `_insetRect` rectangle will be used when calculating the
+	 * dimensions of the screen item instead of the cel's intrinsic width and
+	 * height.
 	 *
-	 * In other words, using an inset rect means that
-	 * the cel is cropped to the dimensions given in
-	 * `_insetRect`.
+	 * In other words, using an inset rect means that the cel is cropped to the
+	 * dimensions given in `_insetRect`.
 	 */
 	bool _useInsetRect;
 
 	/**
-	 * The cropping rectangle used when `_useInsetRect`
-	 * is true.
+	 * The cropping rectangle used when `_useInsetRect` is true.
 	 *
-	 * `_insetRect` is also used to describe the fill
-	 * rectangle of a screen item with a CelObjColor
-	 * cel.
+	 * `_insetRect` is also used to describe the fill rectangle of a screen item
+	 * with a CelObjColor cel.
 	 */
 	Common::Rect _insetRect;
 
 	/**
-	 * The z-index of the screen item in pseudo-3D space.
-	 * Higher values are drawn on top of lower values.
+	 * The z-index of the screen item in pseudo-3D space. Higher values are
+	 * drawn on top of lower values.
 	 */
 	int _z;
 
 	/**
-	 * Sets the common properties of a screen item that must
-	 * be set both during creation and update of a screen
-	 * item.
+	 * Sets the common properties of a screen item that must be set both during
+	 * creation and update of a screen item.
 	 */
 	void setFromObject(SegManager *segMan, const reg_t object, const bool updateCel, const bool updateBitmap);
 
 public:
 	/**
-	 * The creation order number, which ensures a stable
-	 * sort when screen items with identical priorities and
-	 * z-indexes are added to the screen item list.
+	 * The creation order number, which ensures a stable sort when screen items
+	 * with identical priorities and z-indexes are added to the screen item
+	 * list.
 	 */
 	uint32 _creationId;
 
 	/**
-	 * A descriptor for the cel object represented by the
-	 * screen item.
+	 * A descriptor for the cel object represented by the screen item.
 	 */
 	CelInfo32 _celInfo;
 
 	/**
-	 * The cel object used to actually render the screen
-	 * item. This member is populated by calling
-	 * `getCelObj`.
+	 * The cel object used to actually render the screen item. This member is
+	 * populated by calling `getCelObj`.
 	 */
-	mutable CelObj *_celObj;
+	mutable Common::ScopedPtr<CelObj> _celObj;
 
 	/**
-	 * If set, the priority for this screen item is fixed
-	 * in place. Otherwise, the priority of the screen item
-	 * is calculated from its y-position + z-index.
+	 * If set, the priority for this screen item is fixed in place. Otherwise,
+	 * the priority of the screen item is calculated from its y-position +
+	 * z-index.
 	 */
 	bool _fixedPriority;
 
 	/**
-	 * The rendering priority of the screen item, relative
-	 * only to the other screen items within the same plane.
-	 * Higher priorities are drawn above lower priorities.
+	 * The rendering priority of the screen item, relative only to the other
+	 * screen items within the same plane. Higher priorities are drawn above
+	 * lower priorities.
 	 */
 	int16 _priority;
 
 	/**
-	 * The top-left corner of the screen item, in game
-	 * script coordinates, relative to the parent plane.
+	 * The top-left corner of the screen item, in game script coordinates,
+	 * relative to the parent plane.
 	 */
 	Common::Point _position;
 
 	/**
-	 * The associated View script object that was
-	 * used to create the ScreenItem, or a numeric
-	 * value in the case of a ScreenItem that was
-	 * generated outside of the VM.
+	 * The associated View script object that was used to create the ScreenItem,
+	 * or a numeric value in the case of a ScreenItem that was generated by the
+	 * kernel.
 	 */
 	reg_t _object;
 
 	/**
-	 * For screen items representing picture resources,
-	 * the resource ID of the picture.
+	 * For screen items representing picture resources, the resource ID of the
+	 * picture.
 	 */
 	GuiResourceId _pictureId;
 
 	/**
 	 * Flags indicating the state of the screen item.
-	 * - `created` is set when the screen item is first
-	 *   created, either from a VM object or from within the
-	 *   engine itself
-	 * - `updated` is set when `created` is not already set
-	 *   and the screen item is updated from a VM object
-	 * - `deleted` is set by the parent plane, if the parent
-	 *   plane is a pic type and its picture resource ID has
-	 *   changed
+	 * - `created` is set when the screen item is first created, either from a
+	 *   VM object or from within the kernel
+	 * - `updated` is set when `created` is not already set and the screen item
+	 *   is updated from a VM object
+	 * - `deleted` is set by the parent plane, if the parent plane is a pic type
+	 *   and its picture resource ID has changed
 	 */
 	int _created, _updated, _deleted;
 
 	/**
-	 * For screen items that represent picture cels, this
-	 * value is set to match the `_mirrorX` property of the
-	 * parent plane and indicates that the cel should be
-	 * drawn horizontally mirrored. For final drawing, it is
-	 * XORed with the `_mirrorX` property of the cel object.
-	 * The cel object's `_mirrorX` property comes from the
-	 * resource data itself.
+	 * For screen items that represent picture cels, this value is set to match
+	 * the `_mirrorX` property of the parent plane and indicates that the cel
+	 * should be
+	 * drawn horizontally mirrored. For final drawing, it is XORed with the
+	 * `_mirrorX` property of the cel object. The cel object's `_mirrorX`
+	 * property comes from the resource data.
 	 */
 	bool _mirrorX;
 
 	/**
-	 * The scaling ratios to use when drawing this screen
-	 * item. These values are calculated according to the
-	 * scale info whenever the screen item is updated.
+	 * The scaling ratios to use when drawing this screen item. These values are
+	 * calculated according to the scale info whenever the screen item is
+	 * updated.
 	 */
 	Ratio _ratioX, _ratioY;
 
 	/**
-	 * The top-left corner of the screen item, in screen
-	 * coordinates.
+	 * The top-left corner of the screen item, in screen coordinates.
 	 */
 	Common::Point _scaledPosition;
 
 	/**
-	 * The position & dimensions of the screen item in
-	 * screen coordinates. This rect includes the offset of
-	 * the parent plane and is clipped to the screen.
+	 * The position & dimensions of the screen item in screen coordinates. This
+	 * rect includes the offset of the parent plane and is clipped to the
+	 * screen.
 	 */
 	Common::Rect _screenRect;
 
 	/**
-	 * Whether or not the screen item should be drawn
-	 * with black lines drawn every second line. This is
-	 * used when pixel doubling videos to improve apparent
-	 * sharpness at the cost of your eyesight.
+	 * Whether or not the screen item should be drawn with black lines drawn
+	 * every second line. This is used when pixel doubling videos to improve
+	 * apparent sharpness at the cost of your eyesight.
 	 */
 	bool _drawBlackLines;
 
@@ -242,7 +225,6 @@ public:
 	ScreenItem(const reg_t plane, const CelInfo32 &celInfo, const Common::Rect &rect);
 	ScreenItem(const reg_t plane, const CelInfo32 &celInfo, const Common::Point &position, const ScaleInfo &scaleInfo);
 	ScreenItem(const ScreenItem &other);
-	~ScreenItem();
 	void operator=(const ScreenItem &);
 
 	inline bool operator<(const ScreenItem &other) const {
@@ -337,39 +319,36 @@ public:
 	}
 
 	/**
-	 * Calculates the dimensions and scaling parameters for
-	 * the screen item, using the given plane as the parent
-	 * plane for screen rect positioning.
+	 * Calculates the dimensions and scaling parameters for the screen item,
+	 * using the given plane as the parent plane for screen rect positioning.
 	 *
-	 * @note This method was called Update in SCI engine.
+	 * @note This method was called Update in SSCI.
 	 */
 	void calcRects(const Plane &plane);
 
 	/**
-	 * Retrieves the corresponding cel object for this
-	 * screen item. If a cel object does not already exist,
-	 * one will be created and assigned.
+	 * Retrieves the corresponding cel object for this screen item. If a cel
+	 * object does not already exist, one will be created and assigned.
 	 */
 	CelObj &getCelObj() const;
 
 	void printDebugInfo(Console *con) const;
 
 	/**
-	 * Updates the properties of the screen item from a
-	 * VM object.
+	 * Updates the properties of the screen item from a VM object.
 	 */
 	void update(const reg_t object);
 
 	/**
-	 * Updates the properties of the screen item for one not belonging
-	 * to a VM object. Originally GraphicsMgr::UpdateScreenItem.
+	 * Updates the properties of the screen item for one not belonging to a VM
+	 * object. Originally GraphicsMgr::UpdateScreenItem.
 	 */
 	void update();
 
 	/**
-	 * Gets the "now seen" rect for the screen item, which
-	 * represents the current size and position of the
-	 * screen item on the screen in script coordinates.
+	 * Gets the "now seen" rect for the screen item, which represents the
+	 * current size and position of the screen item on the screen in script
+	 * coordinates.
 	 */
 	Common::Rect getNowSeenRect(const Plane &plane) const;
 };