Migrate AnimationInfo's internal fixed-point fields to FixedPoint

Add FixedPoint::divideWithoutWidening for cases where the caller can
already show the wide intermediate isn't needed, and use it in
AnimationInfo::setNewAnimation to avoid a 64-bit division that the
original int32_t arithmetic never required.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

Source/engine/animationinfo.cpp

@@ -27,16 +27,16 @@ int8_t AnimationInfo::getFrameToUseForRendering() const
 	if (currentFrame >= relevantFramesForDistributing_)
 		return currentFrame;
 
-	int16_t ticksSinceSequenceStarted = ticksSinceSequenceStarted_;
+	TicksFixed ticksSinceSequenceStarted = ticksSinceSequenceStarted_;
 	if (ticksSinceSequenceStarted_ < 0) {
-		ticksSinceSequenceStarted = 0;
-		Log("getFrameToUseForRendering: Invalid ticksSinceSequenceStarted_ {}", ticksSinceSequenceStarted_);
+		ticksSinceSequenceStarted = TicksFixed::fromInt(0);
+		Log("getFrameToUseForRendering: Invalid ticksSinceSequenceStarted_ {}", ticksSinceSequenceStarted_.raw());
 	}
 
 	// we don't use the processed game ticks alone but also the fraction of the next game tick (if a rendering happens between game ticks). This helps to smooth the animations.
-	const int32_t totalTicksForCurrentAnimationSequence = getProgressToNextGameTick() + ticksSinceSequenceStarted;
+	const FixedPoint<int, 7U> totalTicksForCurrentAnimationSequence = getProgressToNextGameTick() + ticksSinceSequenceStarted;
 
-	auto absoluteAnimationFrame = static_cast<int8_t>(totalTicksForCurrentAnimationSequence * tickModifier_ / baseValueFraction / baseValueFraction);
+	auto absoluteAnimationFrame = static_cast<int8_t>(totalTicksForCurrentAnimationSequence.multiplyWithoutWidening(tickModifier_).whole());
 	if (skippedFramesFromPreviousAnimation_ > 0) {
 		// absoluteAnimationFrames contains also the Frames from the previous Animation, so if we want to get the current Frame we have to remove them
 		absoluteAnimationFrame -= skippedFramesFromPreviousAnimation_;
@@ -61,8 +61,8 @@ int8_t AnimationInfo::getFrameToUseForRendering() const
 
 uint8_t AnimationInfo::getAnimationProgress() const
 {
-	int16_t ticksSinceSequenceStarted = std::max<int16_t>(0, ticksSinceSequenceStarted_);
-	int32_t tickModifier = tickModifier_;
+	TicksFixed ticksSinceSequenceStarted = std::max(TicksFixed::fromInt(0), ticksSinceSequenceStarted_);
+	RateFixed tickModifier = tickModifier_;
 
 	if (relevantFramesForDistributing_ <= 0) {
 		if (ticksPerFrame <= 0) {
@@ -71,13 +71,15 @@ uint8_t AnimationInfo::getAnimationProgress() const
 		}
 		// This logic is used if animation distribution is not active (see getFrameToUseForRendering).
 		// In this case the variables calculated with animation distribution are not initialized and we have to calculate them on the fly with the given information.
-		ticksSinceSequenceStarted = ((currentFrame * ticksPerFrame) + tickCounterOfCurrentFrame) * baseValueFraction;
-		tickModifier = baseValueFraction / ticksPerFrame;
+		ticksSinceSequenceStarted = TicksFixed::fromInt((currentFrame * ticksPerFrame) + tickCounterOfCurrentFrame);
+		tickModifier = RateFixed(RateFixed::fromInt(1) / ticksPerFrame);
 	}
 
-	const int32_t totalTicksForCurrentAnimationSequence = getProgressToNextGameTick() + ticksSinceSequenceStarted;
-	const int32_t progressInAnimationFrames = totalTicksForCurrentAnimationSequence * tickModifier;
-	const int32_t animationFraction = progressInAnimationFrames / numberOfFrames / baseValueFraction;
+	const FixedPoint<int, 7> totalTicksForCurrentAnimationSequence = getProgressToNextGameTick() + ticksSinceSequenceStarted;
+	const FixedPoint<int, 7> progressInAnimationFrames = totalTicksForCurrentAnimationSequence.multiplyWithoutWidening(tickModifier);
+	// The returned fraction stays in the same baseValueFraction-raw scale (not a whole number):
+	// the fixed*fixed multiply above already performs the one rescale this needs.
+	const int animationFraction = (progressInAnimationFrames / numberOfFrames).raw();
 	assert(animationFraction <= baseValueFraction);
 	return static_cast<uint8_t>(animationFraction);
 }
@@ -102,9 +104,9 @@ void AnimationInfo::setNewAnimation(OptionalClxSpriteList celSprite, int8_t numb
 	currentFrame = numSkippedFrames;
 	tickCounterOfCurrentFrame = 0;
 	this->ticksPerFrame = ticksPerFrame;
-	ticksSinceSequenceStarted_ = 0;
+	ticksSinceSequenceStarted_ = TicksFixed::fromInt(0);
 	relevantFramesForDistributing_ = 0;
-	tickModifier_ = 0;
+	tickModifier_ = RateFixed::fromInt(0);
 	isPetrified = false;
 
 	if (numSkippedFrames != 0 || flags != AnimationDistributionFlags::None) {
@@ -133,7 +135,7 @@ void AnimationInfo::setNewAnimation(OptionalClxSpriteList celSprite, int8_t numb
 			// The Animation Distribution Logic needs to account how many game ticks passed since the Animation started.
 			// Because processAnimation will increase this later (in same game tick as setNewAnimation), we correct this upfront.
 			// This also means Rendering should never happen with ticksSinceSequenceStarted_ < 0.
-			ticksSinceSequenceStarted_ = -baseValueFraction;
+			ticksSinceSequenceStarted_ = TicksFixed::fromInt(-1);
 		}
 
 		if ((flags & AnimationDistributionFlags::SkipsDelayOfLastFrame) == AnimationDistributionFlags::SkipsDelayOfLastFrame) {
@@ -159,25 +161,30 @@ void AnimationInfo::setNewAnimation(OptionalClxSpriteList celSprite, int8_t numb
 		// The truncated Frames from previous Animation will also be shown, so we also have to distribute them for the given time (game ticks)
 		relevantAnimationTicksForDistribution += (skippedFramesFromPreviousAnimation_ * ticksPerFrame);
 
-		// At this point we use fixed point math for the fragment calculations
-		relevantAnimationTicksForDistribution *= baseValueFraction;
-		relevantAnimationTicksWithSkipping *= baseValueFraction;
+		// At this point we use fixed point math for the fragment calculations.
+		// relevantAnimationTicksForDistribution/WithSkipping are bounded by int8_t products
+		// (numberOfFrames/ticksPerFrame are each at most int8_t), so an int32_t-backed FixedPoint
+		// here has enough headroom on its own; we use divideWithoutWidening below to avoid
+		// promoting to a 64-bit intermediate that this bounded ratio never needs.
+		using WideTicksFixed = FixedPoint<int32_t, 7>;
+		WideTicksFixed relevantTicksForDistribution = WideTicksFixed::fromInt(relevantAnimationTicksForDistribution);
+		WideTicksFixed relevantTicksWithSkipping = WideTicksFixed::fromInt(relevantAnimationTicksWithSkipping);
 
 		// The preview animation was shown some times (less than one game tick)
 		// So we overall have a longer time the animation is shown
-		ticksSinceSequenceStarted_ += previewShownGameTickFragments;
-		relevantAnimationTicksWithSkipping += previewShownGameTickFragments;
+		ticksSinceSequenceStarted_ += TicksFixed::fromRaw(previewShownGameTickFragments);
+		relevantTicksWithSkipping += WideTicksFixed::fromRaw(previewShownGameTickFragments);
 
 		// if we skipped Frames we need to expand the game ticks to make one game tick for this Animation "faster"
-		int32_t tickModifier = 0;
-		if (relevantAnimationTicksWithSkipping != 0)
-			tickModifier = baseValueFraction * relevantAnimationTicksForDistribution / relevantAnimationTicksWithSkipping;
+		WideTicksFixed tickModifier = WideTicksFixed::fromInt(0);
+		if (relevantTicksWithSkipping != 0)
+			tickModifier = relevantTicksForDistribution.divideWithoutWidening(relevantTicksWithSkipping);
 
 		// tickModifier specifies the Animation fraction per game tick, so we have to remove the delay from the variable
 		tickModifier /= ticksPerFrame;
 
 		relevantFramesForDistributing_ = relevantAnimationFramesForDistributing;
-		tickModifier_ = static_cast<uint16_t>(tickModifier);
+		tickModifier_ = RateFixed(tickModifier);
 	}
 }
 
@@ -192,9 +199,9 @@ void AnimationInfo::changeAnimationData(OptionalClxSpriteList celSprite, int8_t
 
 		this->numberOfFrames = numberOfFrames;
 		this->ticksPerFrame = ticksPerFrame;
-		ticksSinceSequenceStarted_ = 0;
+		ticksSinceSequenceStarted_ = TicksFixed::fromInt(0);
 		relevantFramesForDistributing_ = 0;
-		tickModifier_ = 0;
+		tickModifier_ = RateFixed::fromInt(0);
 	}
 	this->sprites = celSprite;
 }
@@ -202,30 +209,30 @@ void AnimationInfo::changeAnimationData(OptionalClxSpriteList celSprite, int8_t
 void AnimationInfo::processAnimation(bool reverseAnimation /*= false*/)
 {
 	tickCounterOfCurrentFrame++;
-	ticksSinceSequenceStarted_ += baseValueFraction;
+	ticksSinceSequenceStarted_ += TicksFixed::fromInt(1);
 	if (tickCounterOfCurrentFrame >= ticksPerFrame) {
 		tickCounterOfCurrentFrame = 0;
 		if (reverseAnimation) {
 			--currentFrame;
 			if (currentFrame == -1) {
 				currentFrame = numberOfFrames - 1;
-				ticksSinceSequenceStarted_ = 0;
+				ticksSinceSequenceStarted_ = TicksFixed::fromInt(0);
 			}
 		} else {
 			++currentFrame;
 			if (currentFrame >= numberOfFrames) {
 				currentFrame = 0;
-				ticksSinceSequenceStarted_ = 0;
+				ticksSinceSequenceStarted_ = TicksFixed::fromInt(0);
 			}
 		}
 	}
 }
 
-uint8_t AnimationInfo::getProgressToNextGameTick() const
+FixedPoint<uint8_t, 7> AnimationInfo::getProgressToNextGameTick() const
 {
 	if (isPetrified)
-		return 0;
-	return ProgressToNextGameTick;
+		return FixedPoint<uint8_t, 7>::fromRaw(0);
+	return FixedPoint<uint8_t, 7>::fromRaw(ProgressToNextGameTick);
 }
 
 } // namespace devilution

Source/engine/animationinfo.h

@@ -9,6 +9,7 @@
 #include <type_traits>
 
 #include "engine/clx_sprite.hpp"
+#include "utils/fixed_point.hpp"
 
 namespace devilution {
 
@@ -109,15 +110,28 @@ public:
 	void processAnimation(bool reverseAnimation = false);
 
 	/**
-	 * @brief Fractions in AnimationInfo are stored as fixed point (baseValueFraction/128 correspondents to 1/100%).
+	 * @brief The scale of the raw fixed point fractions still used at some boundaries (see
+	 * getAnimationProgress() and previewShownGameTickFragments); baseValueFraction/128
+	 * corresponds to 1/100%. Internally, AnimationInfo uses FixedPoint<..., 7> instead, which is
+	 * the same scale expressed as a type rather than a raw value.
 	 */
 	constexpr static uint8_t baseValueFraction = 128;
 
 private:
 	/**
-	 * @brief returns the progress as a fraction in time to the next game tick or no progress if the animation is frozen (see baseValueFraction)
+	 * @brief A fixed point count of game ticks, signed because ProcessAnimationPending
+	 * distribution deliberately starts it out negative.
 	 */
-	[[nodiscard]] uint8_t getProgressToNextGameTick() const;
+	using TicksFixed = FixedPoint<int16_t, 7>;
+	/**
+	 * @brief A fixed point rate of animation-fractions advanced per game tick.
+	 */
+	using RateFixed = FixedPoint<uint16_t, 7>;
+
+	/**
+	 * @brief returns the progress as a fraction in time to the next game tick or no progress if the animation is frozen
+	 */
+	[[nodiscard]] FixedPoint<uint8_t, 7> getProgressToNextGameTick() const;
 
 	/**
 	 * @brief Animation Frames that will be adjusted for the skipped Frames/game ticks
@@ -128,13 +142,13 @@ private:
 	 */
 	int8_t skippedFramesFromPreviousAnimation_;
 	/**
-	 * @brief Specifies how many animations-fractions (see baseValueFraction) are displayed between two game ticks. this can be more than one frame, if animations are skipped or less than one frame if the same animation is shown in multiple times (delay specified).
+	 * @brief Specifies how many animation-fractions are displayed between two game ticks. this can be more than one frame, if animations are skipped or less than one frame if the same animation is shown in multiple times (delay specified).
 	 */
-	uint16_t tickModifier_;
+	RateFixed tickModifier_;
 	/**
 	 * @brief Number of game ticks after the current animation sequence started
 	 */
-	int16_t ticksSinceSequenceStarted_;
+	TicksFixed ticksSinceSequenceStarted_;
 };
 
 } // namespace devilution

Source/utils/fixed_point.hpp

@@ -130,6 +130,53 @@ public:
 		return *this;
 	}
 
+	/**
+	 * @brief Multiplies by another fixed point value like `operator*`, but without the extra wide intermediate.
+	 *
+	 * Computes entirely in `Promoted` (the type `raw() * factor.raw()` would naturally have), with
+	 * no wider scratch type. Only use this where the caller can already show that the pre-rescale
+	 * product of the two raw values fits in `Promoted` - otherwise the result is silently wrong, not
+	 * just imprecise. Prefer `operator*` unless that guarantee holds and the wider intermediate's
+	 * cost (e.g. a 64-bit multiply on a 32-bit target) actually matters here.
+	 */
+	template <typename OtherStorageT>
+	[[nodiscard]] DVL_ALWAYS_INLINE constexpr auto multiplyWithoutWidening(FixedPoint<OtherStorageT, FractionalBits> factor) const
+	{
+		using Promoted = decltype(raw_ * factor.raw());
+		const Promoted product = static_cast<Promoted>(raw_) * static_cast<Promoted>(factor.raw());
+		return FixedPoint<Promoted, FractionalBits>::fromRaw(static_cast<Promoted>(product >> FractionalBits));
+	}
+
+	/**
+	 * @brief Divides by another fixed point value, rescaling the result and narrowing it back to this type.
+	 */
+	template <typename OtherStorageT>
+	DVL_ALWAYS_INLINE constexpr FixedPoint &operator/=(FixedPoint<OtherStorageT, FractionalBits> divisor)
+	{
+		using Promoted = decltype(raw_ / divisor.raw());
+		using Wide = fixed_point_detail::DoubleWidth<Promoted>;
+		const Wide dividend = static_cast<Wide>(raw_) << FractionalBits;
+		raw_ = static_cast<StorageT>(dividend / static_cast<Wide>(divisor.raw()));
+		return *this;
+	}
+
+	/**
+	 * @brief Divides by another fixed point value like `operator/`, but without the extra wide intermediate.
+	 *
+	 * Computes entirely in `Promoted` (the type `raw() / divisor.raw()` would naturally have), with
+	 * no wider scratch type. Only use this where the caller can already show that this value's raw
+	 * value shifted left by `FractionalBits` fits in `Promoted` - otherwise the result is silently
+	 * wrong, not just imprecise. Prefer `operator/` unless that guarantee holds and the wider
+	 * intermediate's cost (e.g. a 64-bit division on a 32-bit target) actually matters here.
+	 */
+	template <typename OtherStorageT>
+	[[nodiscard]] DVL_ALWAYS_INLINE constexpr auto divideWithoutWidening(FixedPoint<OtherStorageT, FractionalBits> divisor) const
+	{
+		using Promoted = decltype(raw_ / divisor.raw());
+		const Promoted dividend = static_cast<Promoted>(raw_) << FractionalBits;
+		return FixedPoint<Promoted, FractionalBits>::fromRaw(static_cast<Promoted>(dividend / static_cast<Promoted>(divisor.raw())));
+	}
+
 	template <typename OtherStorageT>
 	[[nodiscard]] DVL_ALWAYS_INLINE constexpr bool operator==(FixedPoint<OtherStorageT, FractionalBits> other) const { return raw_ == other.raw(); }
 	template <typename OtherStorageT>
@@ -197,6 +244,22 @@ template <typename StorageT1, typename StorageT2, unsigned FractionalBits>
 	return FixedPoint<Promoted, FractionalBits>::fromRaw(static_cast<Promoted>(product >> FractionalBits));
 }
 
+/**
+ * @brief Divides two fixed point values, rescaling the result.
+ *
+ * The result is backed by whatever storage type dividing the two raw storage types would
+ * naturally promote to (see the class documentation), computed using an even wider intermediate
+ * so scaling the dividend up before dividing doesn't overflow.
+ */
+template <typename StorageT1, typename StorageT2, unsigned FractionalBits>
+[[nodiscard]] DVL_ALWAYS_INLINE constexpr auto operator/(FixedPoint<StorageT1, FractionalBits> a, FixedPoint<StorageT2, FractionalBits> b)
+{
+	using Promoted = decltype(a.raw() / b.raw());
+	using Wide = fixed_point_detail::DoubleWidth<Promoted>;
+	const Wide dividend = static_cast<Wide>(a.raw()) << FractionalBits;
+	return FixedPoint<Promoted, FractionalBits>::fromRaw(static_cast<Promoted>(dividend / static_cast<Wide>(b.raw())));
+}
+
 template <typename StorageT, unsigned FractionalBits>
 [[nodiscard]] DVL_ALWAYS_INLINE constexpr auto operator-(FixedPoint<StorageT, FractionalBits> a)
 {

test/fixed_point_test.cpp

@@ -95,6 +95,36 @@ TEST(FixedPointTest, MultiplyByFixedPointDoesNotOverflowWideStorage)
 	EXPECT_EQ(large * doubled, Fixed26_6::fromInt(4000));
 }
 
+TEST(FixedPointTest, MultiplyWithoutWideningMatchesOperatorStarWhenItFits)
+{
+	const Fixed10_6 half = Fixed10_6::fromRaw(32); // 0.5
+	EXPECT_EQ(Fixed10_6::fromInt(10).multiplyWithoutWidening(half), Fixed10_6::fromInt(5));
+	EXPECT_EQ(Fixed10_6::fromInt(3).multiplyWithoutWidening(Fixed10_6::fromInt(4)), Fixed10_6::fromInt(12));
+}
+
+TEST(FixedPointTest, DivideByFixedPoint)
+{
+	const Fixed10_6 half = Fixed10_6::fromRaw(32); // 0.5
+	EXPECT_EQ(Fixed10_6::fromInt(10) / half, Fixed10_6::fromInt(20));
+	EXPECT_EQ(Fixed10_6::fromInt(12) / Fixed10_6::fromInt(4), Fixed10_6::fromInt(3));
+}
+
+TEST(FixedPointTest, DivideByFixedPointDoesNotOverflowWideStorage)
+{
+	const Fixed26_6 large = Fixed26_6::fromInt(2000);
+	const Fixed26_6 tiny = Fixed26_6::fromRaw(1); // smallest positive representable value
+	// Scaling `large` up by 2^FractionalBits before dividing would overflow a narrower
+	// intermediate; this only works out to fromInt(2000 * 64) if the wide intermediate holds.
+	EXPECT_EQ(large / tiny, Fixed26_6::fromInt(2000 * 64));
+}
+
+TEST(FixedPointTest, DivideWithoutWideningMatchesOperatorSlashWhenItFits)
+{
+	const Fixed10_6 half = Fixed10_6::fromRaw(32); // 0.5
+	EXPECT_EQ(Fixed10_6::fromInt(10).divideWithoutWidening(half), Fixed10_6::fromInt(20));
+	EXPECT_EQ(Fixed10_6::fromInt(12).divideWithoutWidening(Fixed10_6::fromInt(4)), Fixed10_6::fromInt(3));
+}
+
 TEST(FixedPointTest, WideningConversion)
 {
 	const Fixed10_6 narrow = Fixed10_6::fromInt(5);