novelrt / NovelRT

A cross-platform 2D game engine accompanied by a strong toolset for visual novels.
MIT License
184 stars 42 forks source link

Improved interop for StepTimer with a new Event C API #486

Closed DynamicField closed 1 year ago

DynamicField commented 2 years ago

Currently, the interop implementation for NovelRT::Timing::StepTimer is broken. Here are some problematic snippets:

NrtResult Nrt_StepTimer_create(uint32_t targetFrameRate, double maxSecondDelta, NrtStepTimerHandle* output)
{
    // null-check omitted
    NovelRT::Timing::StepTimer timer = NovelRT::Timing::StepTimer(targetFrameRate, maxSecondDelta); // It's in the stack...
    *output = reinterpret_cast<NrtStepTimerHandle>(&timer); // Returning a pointer to the stack?!?
    return NRT_SUCCESS;
}

NrtTimestamp Nrt_StepTimer_getTargetElapsedTime(NrtStepTimerHandle timer)
{
    NovelRT::Timing::Timestamp* time = new NovelRT::Timing::Timestamp(0); // Allocating an int64 to the heap
    *time = reinterpret_cast<NovelRT::Timing::StepTimer&>(timer).getTargetElapsedTime(); // That reference-cast seems wrong
    return reinterpret_cast<NrtTimestamp&>(*time); // That NrtTimestamp is now pointing to undestroyable heap memory!
}

NrtResult Nrt_StepTimer_setTargetElapsedTime(NrtStepTimerHandle timer, NrtTimestamp target)
{
    // null-check omitted
    NovelRT::Timing::StepTimer time = reinterpret_cast<NovelRT::Timing::StepTimer&>(timer); // Casting a handle as a reference
    time.setTargetElapsedTime(NovelRT::Timing::Timestamp(target)); // Segfault!  
    return NRT_SUCCESS;
}

Probably that StepTimer could previously fit in int64_t, which would explain the reference-casts. Anyway, that code does not work. That PR is going to fix this, and implements APIs for NrtUtilitiesEvent and NrtUtilitiesEventWithTimestamp to make fully usage of the StepTimer. Hopefully, C maniacs will now experience the niche pleasure of staring at a timer ticking excessively fast.

Additions and renovations

Implementation remarks

In the NrtEvent.cpp file, there are templated "generic" versions of methods and a EventImplementationDetails struct to easily add more event types in the future in necessary. (It's macro-free!)

DynamicField commented 1 year ago

Almost ready to merge! (let's wait for CI, and please squash though)

capnkenny commented 1 year ago

No squash. We'd like to preserve history as-is (even if there's large gaps in history).

Good stuff!