SCI32: Clean up GfxTransitions32

* Use containers where appropriate
* Re-wrap doxygen comments to 80 columns
* Clarify comments for parts of the engine that are understood now
  but were not understood at the time of the initial
  implementation

engines/sci/engine/kgraphics32.cpp

@@ -368,10 +368,10 @@ reg_t kSetShowStyle(EngineState *s, int argc, reg_t *argv) {
 	const uint16 type = argv[0].toUint16();
 	reg_t planeObj = argv[1];
 	int16 seconds = argv[2].toSint16();
-	// NOTE: This value seems to indicate whether the transition is an
+	// This value indicates whether the transition is an "exit" transition (0)
-	// “exit” transition (0) or an “enter” transition (-1) for fade
+	// or an "enter" transition (-1) for fade transitions. For other types of
-	// transitions. For other types of transitions, it indicates a palette
+	// transitions, it indicates a palette index value to use when filling the
-	// index value to use when filling the screen.
+	// screen.
 	int16 back = argv[3].toSint16();
 	int16 priority = argv[4].toSint16();
 	int16 animate = argv[5].toSint16();
@@ -404,9 +404,8 @@ reg_t kSetShowStyle(EngineState *s, int argc, reg_t *argv) {
 		error("Illegal show style %d for plane %04x:%04x", type, PRINT_REG(planeObj));
 	}
 
-	// NOTE: The order of planeObj and showStyle are reversed
+	// The order of planeObj and showStyle are reversed because this is how
-	// because this is how SCI3 called the corresponding method
+	// SSCI3 called the corresponding method on the KernelMgr
-	// on the KernelMgr
 	g_sci->_gfxTransitions32->kernelSetShowStyle(argc, planeObj, (ShowStyleType)type, seconds, back, priority, animate, refFrame, pFadeArray, divisions, blackScreen);
 
 	return s->r_acc;
@@ -881,8 +880,8 @@ reg_t kSetScroll(EngineState *s, int argc, reg_t *argv) {
 	const int16 deltaY = argv[2].toSint16();
 	const GuiResourceId pictureId = argv[3].toUint16();
 	const bool animate = argv[4].toUint16();
-	// NOTE: speed was accepted as an argument, but then never actually used
+	// argv[5] was some speed argument, but it was not actually used by SSCI, so
-	// const int16 speed = argc > 5 ? (bool)argv[5].toSint16() : -1;
+	// we ignore it here
 	const bool mirrorX = argc > 6 ? (bool)argv[6].toUint16() : false;
 
 	g_sci->_gfxTransitions32->kernelSetScroll(plane, deltaX, deltaY, pictureId, animate, mirrorX);

engines/sci/graphics/transitions32.cpp

@@ -126,7 +126,7 @@ void GfxTransitions32::processShowStyles() {
 			g_sci->_gfxFrameout->frameOut(true);
 			throttle();
 		}
-	} while(continueProcessing && doFrameOut);
+	} while (continueProcessing && doFrameOut);
 }
 
 void GfxTransitions32::processEffects(PlaneShowStyle &showStyle) {
@@ -249,14 +249,13 @@ void GfxTransitions32::kernelSetShowStyle(const uint16 argc, const reg_t planeOb
 
 	if (createNewEntry) {
 		entry = new PlaneShowStyle;
-		// NOTE: SCI2.1 engine tests if allocation returned a null pointer
+		// SSCI2.1 tests if allocation returned a null pointer but then only
-		// but then only avoids setting currentStep if this is so. Since
+		// avoids setting currentStep if this is so. Since this nonsensical, we
-		// this is a nonsensical approach, we do not do that here
+		// do not do that here
 		entry->currentStep = 0;
 		entry->processed = false;
 		entry->divisions = hasDivisions ? divisions : _defaultDivisions[type];
 		entry->plane = planeObj;
-		entry->fadeColorRangesCount = 0;
 
 		if (getSciVersion() < SCI_VERSION_2_1_MIDDLE) {
 			// for pixel dissolve
@@ -267,32 +266,26 @@ void GfxTransitions32::kernelSetShowStyle(const uint16 argc, const reg_t planeOb
 			entry->screenItems.clear();
 			entry->width = plane->_gameRect.width();
 			entry->height = plane->_gameRect.height();
-		} else {
+		} else if (hasFadeArray) {
-			entry->fadeColorRanges = nullptr;
+			// SSCI2.1mid does no check to verify that an array is successfully
-			if (hasFadeArray) {
+			// retrieved
-				// NOTE: SCI2.1mid engine does no check to verify that an array is
+			SciArray &table = *_segMan->lookupArray(pFadeArray);
-				// successfully retrieved, and SegMan will cause a fatal error
-				// if we try to use a memory segment that is not an array
-				SciArray &table = *_segMan->lookupArray(pFadeArray);
 
-				uint32 rangeCount = table.size();
+			const uint32 rangeCount = table.size();
-				entry->fadeColorRangesCount = rangeCount;
 
-				// NOTE: SCI engine code always allocates memory even if the range
+			// SSCI always allocates memory even if the range table has no
-				// table has no entries, but this does not really make sense, so
+			// entries, but this does not really make sense, so we avoid the
-				// we avoid the allocation call in this case
+			// allocation call in this case
-				if (rangeCount > 0) {
+			if (rangeCount > 0) {
-					entry->fadeColorRanges = new uint16[rangeCount];
+				entry->fadeColorRanges.reserve(rangeCount);
-					for (size_t i = 0; i < rangeCount; ++i) {
+				for (uint32 i = 0; i < rangeCount; ++i) {
-						entry->fadeColorRanges[i] = table.getAsInt16(i);
+					entry->fadeColorRanges.push_back(table.getAsInt16(i));
-					}
 				}
 			}
 		}
 	}
 
-	// NOTE: The original engine had no nullptr check and would just crash
+	// SSCI had no nullptr check and would just crash if it got to here
-	// if it got to here
 	if (entry == nullptr) {
 		error("Cannot edit non-existing ShowStyle entry");
 	}
@@ -397,10 +390,9 @@ ShowStyleList::iterator GfxTransitions32::deleteShowStyle(const ShowStyleList::i
 		break;
 	case kShowStyleFadeIn:
 	case kShowStyleFadeOut:
-		if (getSciVersion() > SCI_VERSION_2_1_EARLY && showStyle->fadeColorRangesCount > 0) {
+		// SSCI manually allocated the color ranges for fades and deleted that
-			delete[] showStyle->fadeColorRanges;
+		// memory here, but we use a container so there is no extra cleanup
-		}
+		// needed
-		break;
 	case kShowStyleNone:
 	case kShowStyleMorph:
 	case kShowStyleHShutterIn:
@@ -944,8 +936,8 @@ bool GfxTransitions32::processFade(const int8 direction, PlaneShowStyle &showSty
 		percent *= 100;
 		percent /= showStyle.divisions - 1;
 
-		if (showStyle.fadeColorRangesCount > 0) {
+		if (showStyle.fadeColorRanges.size()) {
-			for (int i = 0, len = showStyle.fadeColorRangesCount; i < len; i += 2) {
+			for (uint i = 0, len = showStyle.fadeColorRanges.size(); i < len; i += 2) {
 				g_sci->_gfxPalette32->setFade(percent, showStyle.fadeColorRanges[i], showStyle.fadeColorRanges[i + 1]);
 			}
 		} else {

engines/sci/graphics/transitions32.h

@@ -28,7 +28,7 @@
 #include "sci/engine/vm_types.h"
 
 namespace Sci {
-enum ShowStyleType /* : uint8 */ {
+enum ShowStyleType {
 	kShowStyleNone = 0,
 	kShowStyleHShutterOut = 1,
 	kShowStyleHShutterIn = 2,
@@ -48,14 +48,12 @@ enum ShowStyleType /* : uint8 */ {
 };
 
 /**
- * Show styles represent transitions applied to draw planes.
+ * A show style represents a transition applied to a Plane. One show style per
- * One show style per plane can be active at a time.
+ * plane can be active at a time.
  */
 struct PlaneShowStyle {
 	/**
-	 * The ID of the plane this show style belongs to.
+	 * The ID of the plane this transition applies to.
-	 * In SCI2.1mid (at least SQ6), per-plane transitions
-	 * were removed and a single plane ID is used.
 	 */
 	reg_t plane;
 
@@ -65,9 +63,8 @@ struct PlaneShowStyle {
 	ShowStyleType type;
 
 	/**
-	 * When true, the show style is an entry transition
+	 * When true, the show style is an entry transition to a new room. When
-	 * to a new room. When false, it is an exit
+	 * false, it is an exit transition away from an old room.
-	 * transition away from an old room.
 	 */
 	bool fadeUp;
 
@@ -77,38 +74,35 @@ struct PlaneShowStyle {
 	int16 divisions;
 
 	/**
-	 * The color used by transitions that draw CelObjColor
+	 * The color used by transitions that draw CelObjColor screen items. -1 for
-	 * screen items. -1 for transitions that do not draw
+	 * transitions that do not draw screen items.
-	 * screen items.
 	 */
 	int16 color;
 
-	// TODO: Probably uint32
+	/**
-	// TODO: This field probably should be used in order to
+	 * The amount of time, in ticks, between each cycle of the animation.
-	// provide time-accurate processing of show styles. In the
+	 */
-	// actual SCI engine (at least 2–2.1mid) it appears that
-	// style transitions are drawn “as fast as possible”, one
-	// step per loop, even though this delay field exists
 	int delay;
 
-	// TODO: Probably bool, but never seems to be true?
+	/**
+	 * If true, GfxTransitions32 will yield back to the main game loop after
+	 * calculating the next frame. Otherwise, GfxTransitions32 takes exclusive
+	 * control over the game loop until the transition has completed.
+	 */
 	bool animate;
 
 	/**
-	 * The wall time at which the next step of the animation
+	 * The time at which the next step of the animation should execute.
-	 * should execute.
 	 */
 	uint32 nextTick;
 
 	/**
-	 * During playback of the show style, the current step
+	 * During playback of the show style, the current step (out of `divisions`).
-	 * (out of divisions).
 	 */
 	int currentStep;
 
 	/**
-	 * Whether or not this style has finished running and
+	 * Whether or not this style has finished running and is ready for disposal.
-	 * is ready for disposal.
 	 */
 	bool processed;
 
@@ -117,32 +111,30 @@ struct PlaneShowStyle {
 	//
 
 	/**
-	 * A list of screen items, each representing one
+	 * A list of screen items, each representing one block of a wipe transition.
-	 * block of a wipe transition.
+	 * These screen items are owned by GfxFrameout.
 	 */
 	Common::Array<ScreenItem *> screenItems;
 
 	/**
-	 * For wipe transitions, the number of edges with a
+	 * For wipe transitions, the number of edges with a moving wipe (1, 2, or
-	 * moving wipe (1, 2, or 4).
+	 * 4).
 	 */
 	uint8 numEdges;
 
 	/**
-	 * The dimensions of the plane, in game script
+	 * The dimensions of the plane, in game script coordinates.
-	 * coordinates.
 	 */
 	int16 width, height;
 
 	/**
-	 * For pixel dissolve transitions, the screen item
+	 * For pixel dissolve transitions, the screen item used to render the
-	 * used to render the transition.
+	 * transition. This screen item is owned by GfxFrameout.
 	 */
 	ScreenItem *bitmapScreenItem;
 
 	/**
-	 * For pixel dissolve transitions, the bitmap used
+	 * For pixel dissolve transitions, the bitmap used to render the transition.
-	 * to render the transition.
 	 */
 	reg_t bitmap;
 
@@ -152,15 +144,13 @@ struct PlaneShowStyle {
 	uint32 dissolveMask;
 
 	/**
-	 * The first pixel that was dissolved in a pixel
+	 * The first pixel that was dissolved in a pixel dissolve transition.
-	 * dissolve transition.
 	 */
 	uint32 firstPixel;
 
 	/**
-	 * The last pixel that was dissolved. Once all
+	 * The last pixel that was dissolved. Once all pixels have been dissolved,
-	 * pixels have been dissolved, `pixel` will once
+	 * `pixel` will once again equal `firstPixel`.
-	 * again equal `firstPixel`.
 	 */
 	uint32 pixel;
 
@@ -169,21 +159,15 @@ struct PlaneShowStyle {
 	//
 
 	/**
-	 * The number of entries in the fadeColorRanges array.
+	 * An array of palette indexes, in the order [ fromColor, toColor, ... ].
-	 */
-	uint8 fadeColorRangesCount;
-
-	/**
-	 * A pointer to an dynamically sized array of palette
-	 * indexes, in the order [ fromColor, toColor, ... ].
 	 * Only colors within this range are transitioned.
 	 */
-	uint16 *fadeColorRanges;
+	Common::Array<uint16> fadeColorRanges;
 };
 
 /**
- * PlaneScroll describes a transition between two different
+ * PlaneScroll describes a transition between two different pictures within a
- * pictures within a single plane.
+ * single plane.
  */
 struct PlaneScroll {
 	/**
@@ -197,28 +181,26 @@ struct PlaneScroll {
 	int16 x, y;
 
 	/**
-	 * The distance that should be scrolled. Only one of
+	 * The distance that should be scrolled. Only one of `deltaX` or `deltaY`
-	 * `deltaX` or `deltaY` may be set.
+	 * may be set.
 	 */
 	int16 deltaX, deltaY;
 
 	/**
-	 * The pic that should be created and scrolled into
+	 * The pic that should be created and scrolled into view inside the plane.
-	 * view inside the plane.
 	 */
 	GuiResourceId newPictureId;
 
 	/**
-	 * The picture that should be scrolled out of view
+	 * The picture that should be scrolled out of view and deleted from the
-	 * and deleted from the plane.
+	 * plane.
 	 */
 	GuiResourceId oldPictureId;
 
 	/**
-	 * If true, the scroll animation is interleaved
+	 * If true, GfxTransitions32 will yield back to the main game loop after
-	 * with other updates to the graphics. If false,
+	 * calculating the next frame. Otherwise, GfxTransitions32 takes exclusive
-	 * the scroll will be exclusively animated until
+	 * control over the game loop until the transition has completed.
-	 * it is finished.
 	 */
 	bool animate;
 
@@ -235,6 +217,7 @@ class GfxTransitions32 {
 public:
 	GfxTransitions32(SegManager *_segMan);
 	~GfxTransitions32();
+
 private:
 	SegManager *_segMan;
 
@@ -264,209 +247,185 @@ public:
 	inline bool hasShowStyles() const { return !_showStyles.empty(); }
 
 	/**
-	 * Processes all active show styles in a loop
+	 * Processes all active show styles in a loop until they are finished.
-	 * until they are finished.
 	 */
 	void processShowStyles();
 
 	/**
-	 * Processes show styles that are applied
+	 * Processes show styles that are applied through
-	 * through `GfxFrameout::palMorphFrameOut`.
+	 * `GfxFrameout::palMorphFrameOut`.
 	 */
 	void processEffects(PlaneShowStyle &showStyle);
 
-	// NOTE: This signature is taken from SCI3 Phantasmagoria 2
-	// and is valid for all implementations of SCI32
 	void kernelSetShowStyle(const uint16 argc, const reg_t planeObj, const ShowStyleType type, const int16 seconds, const int16 direction, const int16 priority, const int16 animate, const int16 frameOutNow, reg_t pFadeArray, int16 divisions, const int16 blackScreen);
 
 	/**
-	 * Sets the range that will be used by
+	 * Sets the range that will be used by `GfxFrameout::palMorphFrameOut` to
-	 * `GfxFrameout::palMorphFrameOut` to alter
+	 * alter palette entries.
-	 * palette entries.
 	 */
 	void kernelSetPalStyleRange(const uint8 fromColor, const uint8 toColor);
 
 	/**
-	 * A map of palette entries that can be morphed
+	 * A map of palette entries that can be morphed by the Morph show style.
-	 * by the Morph show style.
 	 */
 	int8 _styleRanges[256];
 
 private:
 	/**
-	 * Default sequence values for pixel dissolve
+	 * Default sequence values for pixel dissolve transition bit masks.
-	 * transition bit masks.
 	 */
 	int *_dissolveSequenceSeeds;
 
 	/**
-	 * Default values for `PlaneShowStyle::divisions`
+	 * Default values for `PlaneShowStyle::divisions` for the current SCI
-	 * for the current SCI version.
+	 * version.
 	 */
 	int16 *_defaultDivisions;
 
 	/**
-	 * The list of PlaneShowStyles that are
+	 * The list of PlaneShowStyles that are currently active.
-	 * currently active.
 	 */
 	ShowStyleList _showStyles;
 
 	/**
-	 * Finds a show style that applies to the given
+	 * Finds a show style that applies to the given plane.
-	 * plane.
 	 */
 	PlaneShowStyle *findShowStyleForPlane(const reg_t planeObj);
 
 	/**
-	 * Finds the iterator for a show style that
+	 * Finds the iterator for a show style that applies to the given plane.
-	 * applies to the given plane.
 	 */
 	ShowStyleList::iterator findIteratorForPlane(const reg_t planeObj);
 
 	/**
-	 * Deletes the given PlaneShowStyle and returns
+	 * Deletes the given PlaneShowStyle and returns the next PlaneShowStyle from
-	 * the next PlaneShowStyle from the list of
+	 * the list of styles.
-	 * styles.
 	 */
 	ShowStyleList::iterator deleteShowStyle(const ShowStyleList::iterator &showStyle);
 
 	/**
-	 * Initializes the given PlaneShowStyle for a
+	 * Initializes the given PlaneShowStyle for a horizontal wipe effect for
-	 * horizontal wipe effect for SCI2 to 2.1early.
+	 * SCI2 to 2.1early.
 	 */
 	void configure21EarlyHorizontalWipe(PlaneShowStyle &showStyle, const int16 priority);
 
 	/**
-	 * Initializes the given PlaneShowStyle for a
+	 * Initializes the given PlaneShowStyle for a horizontal shutter effect for
-	 * horizontal shutter effect for SCI2 to 2.1early.
+	 * SCI2 to 2.1early.
 	 */
 	void configure21EarlyHorizontalShutter(PlaneShowStyle &showStyle, const int16 priority);
 
 	/**
-	 * Initializes the given PlaneShowStyle for an
+	 * Initializes the given PlaneShowStyle for an iris effect for SCI2 to
-	 * iris effect for SCI2 to 2.1early.
+	 * 2.1early.
 	 */
 	void configure21EarlyIris(PlaneShowStyle &showStyle, const int16 priority);
 
 	/**
-	 * Initializes the given PlaneShowStyle for a
+	 * Initializes the given PlaneShowStyle for a pixel dissolve effect for SCI2
-	 * pixel dissolve effect for SCI2 to 2.1early.
+	 * to 2.1early.
 	 */
 	void configure21EarlyDissolve(PlaneShowStyle &showStyle, const int16 priority, const Common::Rect &gameRect);
 
 	/**
-	 * Processes one tick of the given
+	 * Processes one tick of the given PlaneShowStyle.
-	 * PlaneShowStyle.
 	 */
 	bool processShowStyle(PlaneShowStyle &showStyle, uint32 now);
 
 	/**
-	 * Performs an instant transition between two
+	 * Performs an instant transition between two rooms.
-	 * rooms.
 	 */
 	bool processNone(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders into a room
+	 * Performs a transition that renders into a room with a horizontal shutter
-	 * with a horizontal shutter effect.
+	 * effect.
 	 */
 	bool processHShutterOut(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders to black
+	 * Performs a transition that renders to black with a horizontal shutter
-	 * with a horizontal shutter effect.
+	 * effect.
 	 */
 	void processHShutterIn(const PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders into a room
+	 * Performs a transition that renders into a room with a vertical shutter
-	 * with a vertical shutter effect.
+	 * effect.
 	 */
 	void processVShutterOut(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders to black
+	 * Performs a transition that renders to black with a vertical shutter
-	 * with a vertical shutter effect.
+	 * effect.
 	 */
 	void processVShutterIn(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders into a room
+	 * Performs a transition that renders into a room with a wipe to the left.
-	 * with a wipe to the left.
 	 */
 	void processWipeLeft(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders to black
+	 * Performs a transition that renders to black with a wipe to the right.
-	 * with a wipe to the right.
 	 */
 	void processWipeRight(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders into a room
+	 * Performs a transition that renders into a room with a wipe upwards.
-	 * with a wipe upwards.
 	 */
 	void processWipeUp(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders to black
+	 * Performs a transition that renders to black with a wipe downwards.
-	 * with a wipe downwards.
 	 */
 	void processWipeDown(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders into a room
+	 * Performs a transition that renders into a room with an iris effect.
-	 * with an iris effect.
 	 */
 	bool processIrisOut(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders to black
+	 * Performs a transition that renders to black with an iris effect.
-	 * with an iris effect.
 	 */
 	bool processIrisIn(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders between
+	 * Performs a transition that renders between rooms using a block dissolve
-	 * rooms using a block dissolve effect.
+	 * effect.
 	 */
 	void processDissolveNoMorph(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that renders between
+	 * Performs a transition that renders between rooms with a pixel dissolve
-	 * rooms with a pixel dissolve effect.
+	 * effect.
 	 */
 	bool processPixelDissolve(PlaneShowStyle &showStyle);
 
 	/**
-	 * SCI2 to 2.1early implementation of pixel
+	 * SCI2 to 2.1early implementation of pixel dissolve.
-	 * dissolve.
 	 */
 	bool processPixelDissolve21Early(PlaneShowStyle &showStyle);
 
 	/**
-	 * SCI2.1mid and later implementation of
+	 * SCI2.1mid and later implementation of pixel dissolve.
-	 * pixel dissolve.
 	 */
 	bool processPixelDissolve21Mid(const PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a transition that fades to black
+	 * Performs a transition that fades to black between rooms.
-	 * between rooms.
 	 */
 	bool processFade(const int8 direction, PlaneShowStyle &showStyle);
 
 	/**
-	 * Morph transition calls back into the
+	 * Morph transition calls back into the transition system's `processEffects`
-	 * transition system's `processEffects`
+	 * method, which then applies transitions other than None, Fade, or Morph.
-	 * method, which then applies transitions
-	 * other than None, Fade, or Morph.
 	 */
 	bool processMorph(PlaneShowStyle &showStyle);
 
 	/**
-	 * Performs a generic transition for any of
+	 * Performs a generic transition for any of the wipe/shutter/iris effects.
-	 * the wipe/shutter/iris effects.
 	 */
 	bool processWipe(const int8 direction, PlaneShowStyle &showStyle);
 
@@ -476,8 +435,7 @@ public:
 	inline bool hasScrolls() const { return !_scrolls.empty(); }
 
 	/**
-	 * Processes all active plane scrolls
+	 * Processes all active plane scrolls in a loop until they are finished.
-	 * in a loop until they are finished.
 	 */
 	void processScrolls();
 
@@ -490,8 +448,7 @@ private:
 	ScrollList _scrolls;
 
 	/**
-	 * Performs a scroll of the content of
+	 * Performs a scroll of the content of a plane.
-	 * a plane.
 	 */
 	bool processScroll(PlaneScroll &scroll);
 };