Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup animationinfo #7707

Closed
wants to merge 1 commit into from
Closed

Cleanup animationinfo #7707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 80 additions & 76 deletions Source/engine/animationinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,8 @@ int8_t AnimationInfo::getFrameToUseForRendering() const
if (currentFrame >= relevantFramesForDistributing_)
return currentFrame;

int16_t ticksSinceSequenceStarted = ticksSinceSequenceStarted_;
int16_t ticksSinceSequenceStarted = std::max<int16_t>(0, ticksSinceSequenceStarted_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'd leave it as it is, as the log and the local variable are less confusing that way.
For clarity we could also change the if from ticksSinceSequenceStarted_ to ticksSinceSequenceStarted.
But maybe that is just personal taste 😉.

if (ticksSinceSequenceStarted_ < 0) {
ticksSinceSequenceStarted = 0;
Log("getFrameToUseForRendering: Invalid ticksSinceSequenceStarted_ {}", ticksSinceSequenceStarted_);
}

Expand Down Expand Up @@ -82,6 +81,82 @@ uint8_t AnimationInfo::getAnimationProgress() const
return static_cast<uint8_t>(animationFraction);
}

void AnimationInfo::configureFrameDistribution(AnimationDistributionFlags flags, int8_t numberOfFrames, int8_t distributeFramesBeforeFrame, int8_t numSkippedFrames, int8_t ticksPerFrame, uint8_t previewShownGameTickFragments)
{
// Animation Frames that will be adjusted for the skipped Frames/game ticks
int8_t relevantAnimationFramesForDistributing = numberOfFrames;

if (distributeFramesBeforeFrame != 0) {
// After an attack hits (_pAFNum or _pSFNum) it can be canceled or another attack can be queued and this means the animation is canceled.
// In normal attacks frame skipping always happens before the attack actual hit.
// This has the advantage that the sword or bow always points to the enemy when the hit happens (_pAFNum or _pSFNum).
// Our distribution logic must also regard this behaviour, so we are not allowed to distribute the skipped animations after the actual hit (_pAnimStopDistributingAfterFrame).
relevantAnimationFramesForDistributing = distributeFramesBeforeFrame - 1;
}

// Game ticks that will be adjusted for the skipped Frames/game ticks
int32_t relevantAnimationTicksForDistribution = relevantAnimationFramesForDistributing * ticksPerFrame;

// How many game ticks will the Animation be really shown (skipped Frames and game ticks removed)
int32_t relevantAnimationTicksWithSkipping = relevantAnimationTicksForDistribution - (numSkippedFrames * ticksPerFrame);

if (flags & AnimationDistributionFlags::ProcessAnimationPending) {
// If processAnimation will be called after setNewAnimation (in same game tick as setNewAnimation), we increment the Animation-Counter.
// If no delay is specified, this will result in complete skipped frame (see processAnimation).
// But if we have a delay specified, this would only result in a reduced time the first frame is shown (one skipped delay).
// Because of that, we only the remove one game tick from the time the Animation is shown
relevantAnimationTicksWithSkipping -= 1;
// 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;
}

if (flags & AnimationDistributionFlags::SkipsDelayOfLastFrame) {
// The logic for player/monster/... (not processAnimation) only checks the frame not the delay.
// That means if a delay is specified, the last-frame is shown less than the other frames
// Example:
// If we have a animation with 3 frames and with a delay of 1 (ticksPerFrame = 2).
// The logic checks "if (frame == 3) { start_new_animation(); }"
// This will result that frame 4 is the last shown Animation Frame.
// GameTick Frame Cnt
// 1 1 0
// 2 1 1
// 3 2 0
// 3 2 1
// 4 3 0
// 5 - -
// in game tick 5 ProcessPlayer sees Frame = 3 and stops the animation.
// But Frame 3 is only shown 1 game tick and all other Frames are shown 2 game ticks.
// That's why we need to remove the Delay of the last Frame from the time (game ticks) the Animation is shown
relevantAnimationTicksWithSkipping -= (ticksPerFrame - 1);
}

// 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;

// 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;

// 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;
}

// 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);
}

void AnimationInfo::setNewAnimation(OptionalClxSpriteList celSprite, int8_t numberOfFrames, int8_t ticksPerFrame, AnimationDistributionFlags flags /*= AnimationDistributionFlags::None*/, int8_t numSkippedFrames /*= 0*/, int8_t distributeFramesBeforeFrame /*= 0*/, uint8_t previewShownGameTickFragments /*= 0*/)
{
if ((flags & AnimationDistributionFlags::RepeatedAction) == AnimationDistributionFlags::RepeatedAction && distributeFramesBeforeFrame != 0 && this->numberOfFrames == numberOfFrames && currentFrame + 1 >= distributeFramesBeforeFrame && currentFrame != this->numberOfFrames - 1) {
Expand All @@ -108,76 +183,7 @@ void AnimationInfo::setNewAnimation(OptionalClxSpriteList celSprite, int8_t numb
isPetrified = false;

if (numSkippedFrames != 0 || flags != AnimationDistributionFlags::None) {
// Animation Frames that will be adjusted for the skipped Frames/game ticks
int8_t relevantAnimationFramesForDistributing = numberOfFrames;
if (distributeFramesBeforeFrame != 0) {
// After an attack hits (_pAFNum or _pSFNum) it can be canceled or another attack can be queued and this means the animation is canceled.
// In normal attacks frame skipping always happens before the attack actual hit.
// This has the advantage that the sword or bow always points to the enemy when the hit happens (_pAFNum or _pSFNum).
// Our distribution logic must also regard this behaviour, so we are not allowed to distribute the skipped animations after the actual hit (_pAnimStopDistributingAfterFrame).
relevantAnimationFramesForDistributing = distributeFramesBeforeFrame - 1;
}

// Game ticks that will be adjusted for the skipped Frames/game ticks
int32_t relevantAnimationTicksForDistribution = relevantAnimationFramesForDistributing * ticksPerFrame;

// How many game ticks will the Animation be really shown (skipped Frames and game ticks removed)
int32_t relevantAnimationTicksWithSkipping = relevantAnimationTicksForDistribution - (numSkippedFrames * ticksPerFrame);

if ((flags & AnimationDistributionFlags::ProcessAnimationPending) == AnimationDistributionFlags::ProcessAnimationPending) {
// If processAnimation will be called after setNewAnimation (in same game tick as setNewAnimation), we increment the Animation-Counter.
// If no delay is specified, this will result in complete skipped frame (see processAnimation).
// But if we have a delay specified, this would only result in a reduced time the first frame is shown (one skipped delay).
// Because of that, we only the remove one game tick from the time the Animation is shown
relevantAnimationTicksWithSkipping -= 1;
// 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;
}

if ((flags & AnimationDistributionFlags::SkipsDelayOfLastFrame) == AnimationDistributionFlags::SkipsDelayOfLastFrame) {
// The logic for player/monster/... (not processAnimation) only checks the frame not the delay.
// That means if a delay is specified, the last-frame is shown less than the other frames
// Example:
// If we have a animation with 3 frames and with a delay of 1 (ticksPerFrame = 2).
// The logic checks "if (frame == 3) { start_new_animation(); }"
// This will result that frame 4 is the last shown Animation Frame.
// GameTick Frame Cnt
// 1 1 0
// 2 1 1
// 3 2 0
// 3 2 1
// 4 3 0
// 5 - -
// in game tick 5 ProcessPlayer sees Frame = 3 and stops the animation.
// But Frame 3 is only shown 1 game tick and all other Frames are shown 2 game ticks.
// That's why we need to remove the Delay of the last Frame from the time (game ticks) the Animation is shown
relevantAnimationTicksWithSkipping -= (ticksPerFrame - 1);
}

// 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;

// 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;

// 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;

// 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);
configureFrameDistribution(flags, numberOfFrames, distributeFramesBeforeFrame, numSkippedFrames, ticksPerFrame, previewShownGameTickFragments);
}
}

Expand Down Expand Up @@ -207,7 +213,7 @@ void AnimationInfo::processAnimation(bool reverseAnimation /*= false*/)
tickCounterOfCurrentFrame = 0;
if (reverseAnimation) {
--currentFrame;
if (currentFrame == -1) {
if (currentFrame < 0) {
currentFrame = numberOfFrames - 1;
ticksSinceSequenceStarted_ = 0;
}
Expand All @@ -223,9 +229,7 @@ void AnimationInfo::processAnimation(bool reverseAnimation /*= false*/)

uint8_t AnimationInfo::getProgressToNextGameTick() const
{
if (isPetrified)
return 0;
return ProgressToNextGameTick;
return isPetrified ? 0 : ProgressToNextGameTick;
}

} // namespace devilution
2 changes: 2 additions & 0 deletions Source/engine/animationinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ class AnimationInfo {
*/
[[nodiscard]] uint8_t getAnimationProgress() const;

void configureFrameDistribution(AnimationDistributionFlags flags, int8_t numberOfFrames, int8_t distributeFramesBeforeFrame, int8_t numSkippedFrames, int8_t ticksPerFrame, uint8_t previewShownGameTickFragments);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a private method?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure if this is a good move. On the one hand the function is smaller. On the other hand, the new function has only 21 lines less than setNewAnimation and now we have another function with many parameters. 🤷

Maybe we could remove some of the parameters. If we use the ticksPerFrame member (assigned in setNewAnimation) instead of passing it as a variable. The same for numberOfFrames.

We could also replace numSkippedFrames with currentFrame, but that might be going a bit too far.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be right. I was too concerned with salvaging the potentially useful parts of the original PR, but I didn't take enough time to consider the best way to approach cleaning up this file. I think I may just close it instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this function include documentation, e.g. `@brief'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes


/**
* @brief Sets the new Animation with all relevant information for rendering
* @param sprites Animation sprites
Expand Down
Loading