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.

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

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

Merged
merged 10 commits into from
Jan 4, 2023

Conversation

DynamicField
Copy link
Contributor

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

  • The StepTimer interop implementation has been mostly rewritten
  • Methods have been added to create Event<> and Event<NovelRT::Timing::Timestamp>, add event listeners, and invoke them in C.

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!)

Copy link
Member

@capnkenny capnkenny left a comment

Choose a reason for hiding this comment

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

Overall seems good IMO, but I'd like to see unit tests to test the templated functionality that was added

Copy link
Member

@RubyNova RubyNova left a comment

Choose a reason for hiding this comment

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

How is the templated implementation working for the C-compliant function signatures here?

# Conflicts:
#	src/NovelRT.Interop/CMakeLists.txt
#	src/NovelRT.Interop/Timing/NrtStepTimer.cpp
@DynamicField
Copy link
Contributor Author

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

@capnkenny
Copy link
Member

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

Good stuff!

@capnkenny capnkenny merged commit 8841070 into novelrt:main Jan 4, 2023
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