SCI32: Clean up CelObj

* Rewrap comments to 80 columns
* Clarify comments where possible
* Use smart pointers where appropriate
* Change view/pic flags detection to always use word-size
  (byte-size check for flag 0x80 was a compiler optimisation)

1 files changed, 128 insertions, 164 deletions

diff --git a/engines/sci/graphics/celobj32.h b/engines/sci/graphics/celobj32.h
index 6e50f7232e..02b2859f5c 100644
--- a/engines/sci/graphics/celobj32.h
+++ b/engines/sci/graphics/celobj32.h
@@ -33,14 +33,14 @@ namespace Sci {
 typedef Common::Rational Ratio;
 
 // SCI32 has four different coordinate systems:
-// 1. low resolution, 2. game/script resolution,
-// 3. text/bitmap resolution, 4. screen resolution
+// 1. low resolution, 2. game/script resolution, 3. text/bitmap resolution,
+// 4. screen resolution
 //
-// In CelObj, these values are used when there is
-// no baked in resolution of cels.
+// In CelObj, these values are used when there is no baked in resolution of
+// cels.
 //
-// In ScreenItem, it is used when deciding which
-// path to take to calculate dimensions.
+// In ScreenItem, it is used when deciding which path to take to calculate
+// dimensions.
 enum {
 	kLowResX = 320,
 	kLowResY = 200
@@ -60,8 +60,7 @@ enum CelCompressionType {
 };
 
 /**
- * A CelInfo32 object describes the basic properties of a
- * cel object.
+ * A CelInfo32 object describes the basic properties of a cel object.
  */
 struct CelInfo32 {
 	/**
@@ -70,26 +69,24 @@ struct CelInfo32 {
 	CelType type;
 
 	/**
-	 * For cel objects that draw from resources, the ID of
-	 * the resource to load.
+	 * For cel objects that draw from resources, the ID of the resource to load.
 	 */
 	GuiResourceId resourceId;
 
 	/**
-	 * For CelObjView, the loop number to draw from the
-	 * view resource.
+	 * For CelObjView, the loop number to draw from the view resource.
 	 */
 	int16 loopNo;
 
 	/**
-	 * For CelObjView and CelObjPic, the cel number to draw
-	 * from the view or pic resource.
+	 * For CelObjView and CelObjPic, the cel number to draw from the view or pic
+	 * resource.
 	 */
 	int16 celNo;
 
 	/**
-	 * For CelObjMem, a segment register pointing to a heap
-	 * resource containing headered bitmap data.
+	 * For CelObjMem, a segment register pointing to a heap resource containing
+	 * headered bitmap data.
 	 */
 	reg_t bitmap;
 
@@ -98,18 +95,16 @@ struct CelInfo32 {
 	 */
 	uint8 color;
 
-	// NOTE: In at least SCI2.1/SQ6, color is left
-	// uninitialised.
 	CelInfo32() :
+		// In SSCI, color is left uninitialised
 		type(kCelTypeMem),
 		resourceId(0),
 		loopNo(0),
 		celNo(0),
 		bitmap(NULL_REG) {}
 
-	// NOTE: This is the equivalence criteria used by
-	// CelObj::searchCache in at least SCI2.1/SQ6. Notably,
-	// it does not check the color field.
+	// This is the equivalence criteria used by CelObj::searchCache in at least
+	// SSCI SQ6. Notably, it does not check the color field.
 	inline bool operator==(const CelInfo32 &other) {
 		return (
 			type == other.type &&
@@ -143,13 +138,12 @@ struct CelInfo32 {
 class CelObj;
 struct CelCacheEntry {
 	/**
-	 * A monotonically increasing cache ID used to identify
-	 * the least recently used item in the cache for
-	 * replacement.
+	 * A monotonically increasing cache ID used to identify the least recently
+	 * used item in the cache for replacement.
 	 */
 	int id;
-	CelObj *celObj;
-	CelCacheEntry() : id(0), celObj(nullptr) {}
+	Common::ScopedPtr<CelObj> celObj;
+	CelCacheEntry() : id(0) {}
 };
 
 typedef Common::Array<CelCacheEntry> CelCache;
@@ -166,9 +160,9 @@ enum {
 
 struct CelScalerTable {
 	/**
-	 * A lookup table of indexes that should be used to find
-	 * the correct column to read from the source bitmap
-	 * when drawing a scaled version of the source bitmap.
+	 * A lookup table of indexes that should be used to find the correct column
+	 * to read from the source bitmap when drawing a scaled version of the
+	 * source bitmap.
 	 */
 	int valuesX[kCelScalerTableSize];
 
@@ -178,9 +172,9 @@ struct CelScalerTable {
 	Ratio scaleX;
 
 	/**
-	 * A lookup table of indexes that should be used to find
-	 * the correct row to read from a source bitmap when
-	 * drawing a scaled version of the source bitmap.
+	 * A lookup table of indexes that should be used to find the correct row to
+	 * read from a source bitmap when drawing a scaled version of the source
+	 * bitmap.
 	 */
 	int valuesY[kCelScalerTableSize];
 
@@ -202,25 +196,23 @@ class CelScaler {
 	int _activeIndex;
 
 	/**
-	 * Activates a scale table for the given X and Y ratios.
-	 * If there is no table that matches the given ratios,
-	 * the least most recently used table will be replaced
-	 * and activated.
+	 * Activates a scale table for the given X and Y ratios. If there is no
+	 * table that matches the given ratios, the least most recently used table
+	 * will be replaced and activated.
 	 */
 	void activateScaleTables(const Ratio &scaleX, const Ratio &scaleY);
 
 	/**
-	 * Builds a pixel lookup table in `table` for the given
-	 * ratio. The table will be filled up to the specified
-	 * size, which should be large enough to draw across the
-	 * entire target buffer.
+	 * Builds a pixel lookup table in `table` for the given ratio. The table
+	 * will be filled up to the specified size, which should be large enough to
+	 * draw across the entire target buffer.
 	 */
 	void buildLookupTable(int *table, const Ratio &ratio, const int size);
 
 public:
 	CelScaler() :
-	_scaleTables(),
-	_activeIndex(0) {
+		_scaleTables(),
+		_activeIndex(0) {
 		CelScalerTable &table = _scaleTables[0];
 		table.scaleX = Ratio();
 		table.scaleY = Ratio();
@@ -236,7 +228,7 @@ public:
 	/**
 	 * Retrieves scaler tables for the given X and Y ratios.
 	 */
-	const CelScalerTable *getScalerTable(const Ratio &scaleX, const Ratio &scaleY);
+	const CelScalerTable &getScalerTable(const Ratio &scaleX, const Ratio &scaleY);
 };
 
 #pragma mark -
@@ -244,53 +236,47 @@ public:
 
 class ScreenItem;
 /**
- * A cel object is the lowest-level rendering primitive in
- * the SCI engine and draws itself directly to a target
- * pixel buffer.
+ * A cel object is the lowest-level rendering primitive in the SCI engine and
+ * draws itself directly to a target pixel buffer.
  */
 class CelObj {
 protected:
 	/**
-	 * When true, every second line of the cel will be
-	 * rendered as a black line.
+	 * When true, every second line of the cel will be rendered as a black line.
 	 *
 	 * @see ScreenItem::_drawBlackLines
-	 * @note Using a static member because otherwise this
-	 * would otherwise need to be copied down through
-	 * several calls. (SSCI did similar, using a global
-	 * variable.)
+	 * @note Using a static member because otherwise this would otherwise need
+	 * to be copied down through several calls. (SSCI did similar, using a
+	 * global variable.)
 	 */
 	static bool _drawBlackLines;
 
 	/**
-	 * When true, this cel will be horizontally mirrored
-	 * when it is drawn. This is an internal flag that is
-	 * set by draw methods based on the combination of the
-	 * cel's `_mirrorX` property and the owner screen item's
-	 * `_mirrorX` property.
+	 * When true, this cel will be horizontally mirrored when it is drawn. This
+	 * is an internal flag that is set by draw methods based on the combination
+	 * of the cel's `_mirrorX` property and the owner screen item's `_mirrorX`
+	 * property.
 	 */
 	bool _drawMirrored;
 
 public:
-	static CelScaler *_scaler;
+	static Common::ScopedPtr<CelScaler> _scaler;
 
 	/**
-	 * The basic identifying information for this cel. This
-	 * information effectively acts as a composite key for
-	 * a cel object, and any cel object can be recreated
-	 * from this data alone.
+	 * The basic identifying information for this cel. This information
+	 * effectively acts as a composite key for a cel object, and any cel object
+	 * can be recreated from this data alone.
 	 */
 	CelInfo32 _info;
 
 	/**
-	 * The offset to the cel header for this cel within the
-	 * raw resource data.
+	 * The offset to the cel header for this cel within the raw resource data.
 	 */
 	uint32 _celHeaderOffset;
 
 	/**
-	 * The offset to the embedded palette for this cel
-	 * within the raw resource data.
+	 * The offset to the embedded palette for this cel within the raw resource
+	 * data.
 	 */
 	uint32 _hunkPaletteOffset;
 
@@ -300,36 +286,32 @@ public:
 	uint16 _width, _height;
 
 	/**
-	 * TODO: Documentation
+	 * The origin of the cel, relative to the top-left corner, in cel
+	 * coordinates.
 	 */
 	Common::Point _origin;
 
 	/**
-	 * The dimensions of the original coordinate system for
-	 * the cel. Used to scale cels from their native size
-	 * to the correct size on screen.
+	 * The dimensions of the original coordinate system for the cel. Used to
+	 * scale cels from their native size to the correct size on screen.
 	 *
-	 * @note This is set to scriptWidth/Height for
-	 * CelObjColor. For other cel objects, the value comes
-	 * from the raw resource data. For text bitmaps, this is
-	 * the width/height of the coordinate system used to
-	 * generate the text, which also defaults to
-	 * scriptWidth/Height but seems to typically be changed
-	 * to more closely match the native screen resolution.
+	 * @note This is set to scriptWidth/Height for CelObjColor. For other cel
+	 * objects, the value comes from the raw resource data. For text bitmaps,
+	 * this is the width/height of the coordinate system used to generate the
+	 * text, which also defaults to scriptWidth/Height but seems to typically be
+	 * changed to more closely match the native screen resolution.
 	 */
 	uint16 _xResolution, _yResolution;
 
 	/**
-	 * The skip (transparent) color for the cel. When
-	 * compositing, any pixels matching this color will not
-	 * be copied to the buffer.
+	 * The skip (transparent) color for the cel. When compositing, any pixels
+	 * matching this color will not be copied to the buffer.
 	 */
 	uint8 _skipColor;
 
 	/**
-	 * Whether or not this cel has any transparent regions.
-	 * This is used for optimised drawing of non-transparent
-	 * cels.
+	 * Whether or not this cel has any transparent regions. This is used for
+	 * optimised drawing of non-transparent cels.
 	 */
 	bool _transparent;
 
@@ -339,15 +321,14 @@ public:
 	CelCompressionType _compressionType;
 
 	/**
-	 * Whether or not this cel should be palette-remapped?
+	 * Whether or not this cel contains remap pixels.
 	 */
 	bool _remap;
 
 	/**
-	 * If true, the cel contains pre-mirrored picture data.
-	 * This value comes directly from the resource data and
-	 * is XORed with the `_mirrorX` property of the owner
-	 * screen item when rendering.
+	 * If true, the cel contains pre-mirrored picture data. This value comes
+	 * directly from the resource data and is XORed with the `_mirrorX` property
+	 * of the owner screen item when rendering.
 	 */
 	bool _mirrorX;
 
@@ -364,71 +345,63 @@ public:
 	virtual ~CelObj() {};
 
 	/**
-	 * Draws the cel to the target buffer using the priority
-	 * and positioning information from the given screen
-	 * item. The mirroring of the cel will be unchanged from
-	 * any previous call to draw.
+	 * Draws the cel to the target buffer using the priority and positioning
+	 * information from the given screen item. The mirroring of the cel will be
+	 * unchanged from any previous call to draw.
 	 */
 	void draw(Buffer &target, const ScreenItem &screenItem, const Common::Rect &targetRect) const;
 
 	/**
-	 * Draws the cel to the target buffer using the priority
-	 * and positioning information from the given screen
-	 * item and the given mirror flag.
+	 * Draws the cel to the target buffer using the priority and positioning
+	 * information from the given screen item and the given mirror flag.
 	 *
-	 * @note In SCI engine, this function was a virtual
-	 * function, but CelObjView, CelObjPic, and CelObjMem
-	 * all used the same function and the compiler
-	 * deduplicated the copies; we deduplicate the source by
-	 * putting the implementation on CelObj instead of
-	 * copying it to 3/4 of the subclasses.
+	 * @note In SSCI, this function was a virtual function, but CelObjView,
+	 * CelObjPic, and CelObjMem all used the same function and the compiler
+	 * deduplicated the copies; we deduplicate the source by putting the
+	 * implementation on CelObj instead of copying it to 3/4 of the subclasses.
 	 */
 	virtual void draw(Buffer &target, const ScreenItem &screenItem, const Common::Rect &targetRect, const bool mirrorX);
 
 	/**
-	 * Draws the cel to the target buffer using the
-	 * positioning and mirroring information from the
-	 * provided arguments.
+	 * Draws the cel to the target buffer using the positioning and mirroring
+	 * information from the provided arguments.
 	 *
-	 * @note In SCI engine, this function was a virtual
-	 * function, but CelObjView, CelObjPic, and CelObjMem
-	 * all used the same function and the compiler
-	 * deduplicated the copies; we deduplicate the source by
-	 * putting the implementation on CelObj instead of
-	 * copying it to 3/4 of the subclasses.
+	 * @note In SSCI, this function was a virtual function, but CelObjView,
+	 * CelObjPic, and CelObjMem all used the same function and the compiler
+	 * deduplicated the copies; we deduplicate the source by putting the
+	 * implementation on CelObj instead of copying it to 3/4 of the subclasses.
 	 */
 	virtual void draw(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition, const bool mirrorX);
 
 	/**
-	 * Draws the cel to the target buffer using the given
-	 * position and scaling parameters. The mirroring of the
-	 * cel will be unchanged from any previous call to draw.
+	 * Draws the cel to the target buffer using the given position and scaling
+	 * parameters. The mirroring of the cel will be unchanged from any previous
+	 * call to draw.
 	 */
 	void drawTo(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition, const Ratio &scaleX, const Ratio &scaleY) const;
 
 	/**
-	 * Creates a copy of this cel on the free store and
-	 * returns a pointer to the new object. The new cel will
-	 * point to a shared copy of bitmap/resource data.
+	 * Creates a copy of this cel on the free store and returns a pointer to the
+	 * new object. The new cel will point to a shared copy of bitmap/resource
+	 * data.
 	 */
 	virtual CelObj *duplicate() const = 0;
 
 	/**
-	 * Retrieves a pointer to the raw resource data for this
-	 * cel. This method cannot be used with a CelObjColor.
+	 * Retrieves a pointer to the raw resource data for this cel. This method
+	 * cannot be used with a CelObjColor.
 	 */
 	virtual const SciSpan<const byte> getResPointer() const = 0;
 
 	/**
-	 * Reads the pixel at the given coordinates. This method
-	 * is valid only for CelObjView and CelObjPic.
+	 * Reads the pixel at the given coordinates. This method is valid only for
+	 * CelObjView and CelObjPic.
 	 */
 	virtual uint8 readPixel(const uint16 x, const uint16 y, const bool mirrorX) const;
 
 	/**
-	 * Submits the palette from this cel to the palette
-	 * manager for integration into the master screen
-	 * palette.
+	 * Submits the palette from this cel to the palette manager for integration
+	 * into the master screen palette.
 	 */
 	void submitPalette() const;
 
@@ -454,7 +427,8 @@ private:
 	void drawUncompHzFlipMap(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const;
 	void scaleDrawMap(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const;
 	void scaleDrawUncompMap(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const;
-	// NOTE: The original includes versions of the above functions with priority parameters, which were not actually used in SCI32
+	// SSCI includes versions of the above functions with priority parameters
+	// which are not actually used in SCI32
 
 	void drawHzFlipNoMD(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const;
 	void drawNoFlipNoMD(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const;
@@ -464,37 +438,34 @@ private:
 	void drawUncompHzFlipNoMDNoSkip(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition) const;
 	void scaleDrawNoMD(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const;
 	void scaleDrawUncompNoMD(Buffer &target, const Ratio &scaleX, const Ratio &scaleY, const Common::Rect &targetRect, const Common::Point &scaledPosition) const;
-	// NOTE: The original includes versions of the above functions with priority parameters, which were not actually used in SCI32
+	// SSCI includes versions of the above functions with priority parameters
+	// which are not actually used in SCI32
 
 #pragma mark -
 #pragma mark CelObj - Caching
 protected:
 	/**
-	 * A monotonically increasing cache ID used to identify
-	 * the least recently used item in the cache for
-	 * replacement.
+	 * A monotonically increasing cache ID used to identify the least recently
+	 * used item in the cache for replacement.
 	 */
 	static int _nextCacheId;
 
 	/**
-	 * A cache of cel objects used to avoid reinitialisation
-	 * overhead for cels with the same CelInfo32.
+	 * A cache of cel objects used to avoid reinitialisation overhead for cels
+	 * with the same CelInfo32.
 	 */
-	// NOTE: At least SQ6 uses a fixed cache size of 100.
-	static CelCache *_cache;
+	static Common::ScopedPtr<CelCache> _cache;
 
 	/**
-	 * Searches the cel cache for a CelObj matching the
-	 * provided CelInfo32. If not found, -1 is returned.
-	 * nextInsertIndex will receive the index of the oldest
-	 * item in the cache, which can be used to replace
-	 * the oldest item with a newer item.
+	 * Searches the cel cache for a CelObj matching the provided CelInfo32. If
+	 * not found, -1 is returned. `nextInsertIndex` will receive the index of
+	 * the oldest item in the cache, which can be used to replace the oldest
+	 * item with a newer item.
 	 */
 	int searchCache(const CelInfo32 &celInfo, int *nextInsertIndex) const;
 
 	/**
-	 * Puts a copy of this CelObj into the cache at the
-	 * given cache index.
+	 * Puts a copy of this CelObj into the cache at the given cache index.
 	 */
 	void putCopyInCache(int index) const;
 };
@@ -503,22 +474,20 @@ protected:
 #pragma mark CelObjView
 
 /**
- * A CelObjView is the drawing primitive for a View type
- * resource. Each CelObjView corresponds to a single cel
- * within a single loop of a view.
+ * A CelObjView is the drawing primitive for a View type resource. Each
+ * CelObjView corresponds to a single cel within a single loop of a view.
  */
 class CelObjView : public CelObj {
 private:
 	/**
-	 * Analyses resources without baked-in remap flags
-	 * to determine whether or not they should be remapped.
+	 * Analyses resources without baked-in remap flags to determine whether or
+	 * not they should be remapped.
 	 */
 	bool analyzeUncompressedForRemap() const;
 
 	/**
-	 * Analyses compressed resources without baked-in remap
-	 * flags to determine whether or not they should be
-	 * remapped.
+	 * Analyses compressed resources without baked-in remap flags to determine
+	 * whether or not they should be remapped.
 	 */
 	bool analyzeForRemap() const;
 
@@ -532,9 +501,8 @@ public:
 	static int16 getNumCels(const GuiResourceId viewId, const int16 loopNo);
 
 	/**
-	 * Draws the cel to the target buffer using the
-	 * positioning, mirroring, and scaling information from
-	 * the provided arguments.
+	 * Draws the cel to the target buffer using the positioning, mirroring, and
+	 * scaling information from the provided arguments.
 	 */
 	void draw(Buffer &target, const Common::Rect &targetRect, const Common::Point &scaledPosition, bool mirrorX, const Ratio &scaleX, const Ratio &scaleY);
 
@@ -548,16 +516,14 @@ public:
 #pragma mark CelObjPic
 
 /**
- * A CelObjPic is the drawing primitive for a Picture type
- * resource. Each CelObjPic corresponds to a single cel
- * within a picture.
+ * A CelObjPic is the drawing primitive for a Picture type resource. Each
+ * CelObjPic corresponds to a single cel within a picture.
  */
 class CelObjPic : public CelObj {
 private:
 	/**
-	 * Analyses uncompressed resources without baked-in skip
-	 * flags to determine whether or not they can use fast
-	 * blitting.
+	 * Analyses uncompressed resources without baked-in skip flags to determine
+	 * whether or not they can use fast blitting.
 	 */
 	bool analyzeUncompressedForSkip() const;
 
@@ -568,14 +534,13 @@ public:
 	uint8 _celCount;
 
 	/**
-	 * The position of this cel relative to the top-left
-	 * corner of the picture.
+	 * The position of this cel relative to the top-left corner of the picture.
 	 */
 	Common::Point _relativePosition;
 
 	/**
-	 * The z-buffer priority for this cel. Higher prorities
-	 * are drawn on top of lower priorities.
+	 * The z-buffer priority for this cel. Higher prorities are drawn on top of
+	 * lower priorities.
 	 */
 	int16 _priority;
 
@@ -593,10 +558,9 @@ public:
 #pragma mark CelObjMem
 
 /**
- * A CelObjMem is the drawing primitive for arbitrary
- * bitmaps generated in memory. Generated bitmaps in SCI32
- * include text & vector drawings and per-pixel screen
- * transitions like dissolves.
+ * A CelObjMem is the drawing primitive for arbitrary bitmaps generated in
+ * memory. Generated bitmaps in SCI32 include text & vector drawings and
+ * per-pixel screen transitions like dissolves.
  */
 class CelObjMem : public CelObj {
 public: