Skip to content

Fix leaking singleton & easing functions#2

Closed
ismail-yilmaz wants to merge 3 commits intoTrilec:mainfrom
ismail-yilmaz:fix-singleton
Closed

Fix leaking singleton & easing functions#2
ismail-yilmaz wants to merge 3 commits intoTrilec:mainfrom
ismail-yilmaz:fix-singleton

Conversation

@ismail-yilmaz
Copy link
Copy Markdown

@ismail-yilmaz ismail-yilmaz commented Sep 6, 2025

Fixes leaking singleton, easing functions. See U++ discussion #274

@ismail-yilmaz ismail-yilmaz changed the title Fix singleton Fix leaking singleton & easing functions Sep 6, 2025
Comment thread Animation/Animation.cpp
struct Scheduler {
// Singleton accessor
static Scheduler& Inst() { static Scheduler* s = new Scheduler; return *s; }
static Scheduler& Inst() { return Single<Scheduler>(); }
Copy link
Copy Markdown

@klugier klugier Sep 6, 2025

Choose a reason for hiding this comment

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

What is the reason for Singleton here? Why don't simply have Scheduler per Animation instance. Singleton is anti pattern and it is good to think whenever it is need in certain context. FYI, @Trilec.

Comment thread Animation/Animation.cpp
static Scheduler& Inst() { return Single<Scheduler>(); }

// State
Vector<Animation::State*> active; // Owns State* pointers
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

State should not be a pointer and in DeleteState we shouldn't use delete s. In U++ world new/delete should be replace with safer mechanisms. Please keep in mind that you can allocated in Vector and then drop the iteam. FYI, @Trilec.

Comment thread Animation/Animation.h Outdated
inline const Fn& InElastic() { static Fn* f = new Fn(Bezier(0.600, -0.280, 0.735, 0.045)); return *f; }
inline const Fn& OutElastic() { static Fn* f = new Fn(Bezier(0.175, 0.885, 0.320, 1.275)); return *f; }
inline const Fn& InOutElastic() { static Fn* f = new Fn(Bezier(0.680, -0.550, 0.265, 1.550)); return *f; }
inline const Fn& Linear() { static auto f = Upp::MakeOne<Fn>(Bezier(0.000, 0.000, 1.000, 1.000)); return *f; }
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@ismail-yilmaz you said you have idea how to make rework of these functions and make it batter. Can you share your ideas?

Copy link
Copy Markdown
Author

@ismail-yilmaz ismail-yilmaz Sep 7, 2025

Choose a reason for hiding this comment

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

If I'm not missing anything, these are all precalculations and they can be constexpr. I think below approach would be much faster and elegant:

namespace Easing {

namespace detail {
    constexpr double BX(double x1, double x2, double t)
    {
        const double u = 1 - t;
        return 3 * u * u * t * x1 + 3 * u * t * t * x2 + t * t * t;
    }

    constexpr double BY(double y1, double y2, double t)
    {
        const double u = 1 - t;
        return 3 * u * u * t * y1 + 3 * u * t * t * y2 + t * t * t;
    }

    constexpr double Solve(double x1, double y1, double x2, double y2, double x)
    {
        if(x <= 0)
			return 0;
        if(x >= 1)
			return 1;

        double lo = 0, hi = 1, t = x;
        for(int i = 0; i < 8; i++) {
            double cx = BX(x1, x2, t);
            (cx < x ? lo : hi) = t;
            t = 0.5 * (lo + hi);
        }
        return BY(y1, y2, t);
    }
}

constexpr auto Bezier(double x1, double y1, double x2, double y2)
{
    return [x1, y1, x2, y2](double t) {
        return detail::Solve(x1, y1, x2, y2, t);
    };
}

// Predefined constexpr easing functions
constexpr auto Linear        = Bezier(0.000, 0.000, 1.000, 1.000);
constexpr auto OutBounce     = Bezier(0.680, -0.550, 0.265, 1.550);
constexpr auto InQuad        = Bezier(0.550, 0.085, 0.680, 0.530);
constexpr auto OutQuad       = Bezier(0.250, 0.460, 0.450, 0.940);
constexpr auto InOutQuad     = Bezier(0.455, 0.030, 0.515, 0.955);
constexpr auto InCubic       = Bezier(0.550, 0.055, 0.675, 0.190);
constexpr auto OutCubic      = Bezier(0.215, 0.610, 0.355, 1.000);
constexpr auto InOutCubic    = Bezier(0.645, 0.045, 0.355, 1.000);
constexpr auto InQuart       = Bezier(0.895, 0.030, 0.685, 0.220);
constexpr auto OutQuart      = Bezier(0.165, 0.840, 0.440, 1.000);
constexpr auto InOutQuart    = Bezier(0.770, 0.000, 0.175, 1.000);
constexpr auto InQuint       = Bezier(0.755, 0.050, 0.855, 0.060);
constexpr auto OutQuint      = Bezier(0.230, 1.000, 0.320, 1.000);
constexpr auto InOutQuint    = Bezier(0.860, 0.000, 0.070, 1.000);
constexpr auto InSine        = Bezier(0.470, 0.000, 0.745, 0.715);
constexpr auto OutSine       = Bezier(0.390, 0.575, 0.565, 1.000);
constexpr auto InOutSine     = Bezier(0.445, 0.050, 0.550, 0.950);
constexpr auto InExpo        = Bezier(0.950, 0.050, 0.795, 0.035);
constexpr auto OutExpo       = Bezier(0.190, 1.000, 0.220, 1.000);
constexpr auto InOutExpo     = Bezier(1.000, 0.000, 0.000, 1.000);
constexpr auto InElastic     = Bezier(0.600, -0.280, 0.735, 0.045);
constexpr auto OutElastic    = Bezier(0.175, 0.885, 0.320, 1.275);
constexpr auto InOutElastic  = Bezier(0.680, -0.550, 0.265, 1.550);

}

This will require some changes to the code but for the better. Still allows custom curves.

My Lerp overload proposal for Upp/Core is also designed to use this type of functions. They work well.

@Trilec What do you think?

Copy link
Copy Markdown
Owner

@Trilec Trilec Sep 7, 2025

Choose a reason for hiding this comment

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

sorry I'm missing these , I'm still getting used to the whole Github...what I posted in the discussion:

Thanks —I've made the changes (Updated GitHub).
I’d like to keep the call-site API callable (not a pointer) to keep usage simple, e.g. Animate(..., Easing::OutQuad());.
Thanks for the constexpr idea — I like it. For this package though I was prioritizing the designer workflow. Cubic-Bézier control points are easy to reason about, and work great with the curve editor in the demo so people can see and tweak the feel. -- I will explore what you're suggesting..

the next step (which I'm hoping I get some traction on) is how we start putting this in chameleon.
I'm starting to look at this next because I'm designing a gallery control, and would like to have this driven by styles.

Comment thread Animation/Animation.cpp
@@ -22,7 +22,7 @@ namespace {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also, not a fun of using global and alternating then through class method. I think g_step_ms and g_fps should be a members of Animation class.

Copy link
Copy Markdown
Owner

@Trilec Trilec Sep 7, 2025

Choose a reason for hiding this comment

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

Yep agreed, a lot of code in there likely an initial set up , currently on re-working with constexpr , raised some memory issues and branching issues, might have been due to slight shift in timing and the scheduler picked up another bug, memory management on this has been extremely challenging, Im putting in your global concern and GUI tests are now ok but Console im reworking driving animation frames via the real U++ event loop (TimeCallback + Ctrl::ProcessEvents), not a fake/manual loop.

Copy link
Copy Markdown
Owner

@Trilec Trilec Sep 8, 2025

Choose a reason for hiding this comment

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

I've pushed all the changes — hardens the Animation scheduler and replaces the console harness with a deterministic, CI-friendly test suite.
Switched easing to constexpr + small refactors surfaced a shutdown‐order issue (exit-time AV) and some timing/branching edge cases that were previously masked by jitter (I suspect ... pain to find).
Dont see where the examples get updated I updated my repo and pushed

@Trilec
Copy link
Copy Markdown
Owner

Trilec commented Sep 13, 2025

Hi,
made some additional bug fixes ,slight renaming of some internals and the addition of a reset().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants