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

Fix Fade race condition #1262

Merged
merged 6 commits into from
Mar 4, 2025
Merged

Fix Fade race condition #1262

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a470b7a
Add missing include for `<stdexcept>` to `Fade`
DanRStevens Mar 4, 2025
1142e8b
Use `std::domain_error` when `Fade` duration is zero
DanRStevens Mar 4, 2025
19e5682
Update `isFaded` to avoid race conditions
DanRStevens Mar 4, 2025
3c1c34d
Add constant `alphaOpaque` for better readability
DanRStevens Mar 4, 2025
5730f69
Add constant `alphaTransparent` for better readability
DanRStevens Mar 4, 2025
65cc107
Replace `static_cast` with explicit template type parameter
DanRStevens Mar 4, 2025
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
18 changes: 13 additions & 5 deletions NAS2D/Renderer/Fade.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,27 @@
#include "../Math/Rectangle.h"

#include <algorithm>
#include <stdexcept>


using namespace NAS2D;


namespace
{
constexpr uint8_t alphaTransparent = 0;
constexpr uint8_t alphaOpaque = 255;
}


Fade::Fade(DelegateType onFadeComplete) :
Fade{Color::Black, onFadeComplete}
{
}


Fade::Fade(Color fadeColor, DelegateType onFadeComplete) :
mFadeColor{fadeColor.alphaFade(255)},
mFadeColor{fadeColor.alphaFade(alphaOpaque)},
mDirection{FadeDirection::None},
mDuration{},
mFadeTimer{},
Expand Down Expand Up @@ -59,7 +67,7 @@ bool Fade::isFading() const

bool Fade::isFaded() const
{
return (mFadeColor.alpha == 255);
return (mDirection == FadeDirection::None) && (mFadeColor.alpha == alphaOpaque);
}


Expand All @@ -70,7 +78,7 @@ void Fade::update()
return;
}

const auto step = static_cast<uint8_t>(std::clamp(mFadeTimer.elapsedTicks() * 255u / static_cast<unsigned int>(mDuration.milliseconds), 0u, 255u));
const auto step = static_cast<uint8_t>(std::clamp<unsigned int>(mFadeTimer.elapsedTicks() * 255u / mDuration.milliseconds, 0u, 255u));
mFadeColor.alpha = (mDirection == FadeDirection::In) ? 255 - step : step;

if (step == 255)
Expand All @@ -86,7 +94,7 @@ void Fade::update()

void Fade::draw(Renderer& renderer) const
{
if (mFadeColor.alpha > 0)
if (mFadeColor.alpha != alphaTransparent)
{
const auto displayRect = Rectangle{{0, 0}, renderer.size()};
renderer.drawBoxFilled(displayRect, mFadeColor);
Expand All @@ -98,7 +106,7 @@ void Fade::setDuration(Duration newDuration)
{
if (newDuration.milliseconds == 0)
{
throw std::runtime_error("Fade duration must be positive");
throw std::domain_error("Fade duration must be positive");
}

mDuration = newDuration;
Expand Down