Skip to content

Commit

Permalink
Platform: make Sdl2Application::setMinimalLoopPeriod() unit-safe.
Browse files Browse the repository at this point in the history
I wanted to add this for GlfwApplication, only to realize the timer there
is a double, in seconds, so accepting *integral* milliseconds there felt
very weird. Let's use the fancy new time types instead.

Also updated the docs to (hopefully) clarify that setSwapInterval() has
to be called in order for the interaction between the two to work
properly.

As usual, the old variant taking untyped milliseconds is a deprecated
alias to this one.
  • Loading branch information
mosra committed Oct 5, 2024
1 parent f7cdd61 commit ba0fbe3
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 24 deletions.
3 changes: 3 additions & 0 deletions doc/changelog.dox
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,9 @@ See also:
performance and a default, use @relativeref{Platform::EmscriptenApplication::GLConfiguration,Flag::PowerPreferenceLowPower}
or @relativeref{Platform::EmscriptenApplication::GLConfiguration,Flag::PowerPreferenceHighPerformance}
instead
- @cpp Platform::Sdl2Application::setMinimalLoopPeriod(UnsignedInt) @ce
taking an untyped millisecond value is deprecated in favor of
@relativeref{Platform::Sdl2Application,setMinimalLoopPeriod(Nanoseconds)}
- @cpp Shaders::DistanceFieldVector @ce, @cpp Shaders::Flat @ce,
@cpp Shaders::Generic @ce, @cpp Shaders::MeshVisualizer2D @ce,
@cpp Shaders::MeshVisualizer3D @ce, @cpp Shaders::Phong @ce,
Expand Down
39 changes: 27 additions & 12 deletions src/Magnum/Platform/Sdl2Application.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@
#include "Magnum/Math/ConfigurationValue.h"
#include "Magnum/Math/FunctionsBatch.h"
#include "Magnum/Math/Range.h"
#ifndef CORRADE_TARGET_EMSCRIPTEN
#include "Magnum/Math/Time.h"
#endif
#include "Magnum/Platform/ScreenedApplication.hpp"
#include "Magnum/Platform/Implementation/DpiScaling.h"

Expand All @@ -78,6 +81,7 @@ extern "C" {
namespace Magnum { namespace Platform {

using namespace Containers::Literals;
using namespace Math::Literals;

namespace {

Expand Down Expand Up @@ -126,9 +130,6 @@ Sdl2Application::Sdl2Application(const Arguments& arguments, const Configuration
#endif

Sdl2Application::Sdl2Application(const Arguments& arguments, NoCreateT):
#ifndef CORRADE_TARGET_EMSCRIPTEN
_minimalLoopPeriod{0},
#endif
_flags{Flag::Redraw}
{
Utility::Arguments args{Implementation::windowScalingArguments()};
Expand Down Expand Up @@ -826,6 +827,20 @@ bool Sdl2Application::setSwapInterval(const Int interval) {
return true;
}

#ifndef CORRADE_TARGET_EMSCRIPTEN
void Sdl2Application::setMinimalLoopPeriod(const Nanoseconds time) {
CORRADE_ASSERT(time >= 0_nsec,
"Platform::Sdl2Application::setMinimalLoopPeriod(): expected non-negative time, got" << time, );
_minimalLoopPeriodMilliseconds = Long(time)/1000000;
}

#ifdef MAGNUM_BUILD_DEPRECATED
void Sdl2Application::setMinimalLoopPeriod(const UnsignedInt milliseconds) {
_minimalLoopPeriodMilliseconds = milliseconds;
}
#endif
#endif

void Sdl2Application::redraw() { _flags |= Flag::Redraw; }

Sdl2Application::~Sdl2Application() {
Expand Down Expand Up @@ -890,7 +905,7 @@ bool Sdl2Application::mainLoopIteration() {
#endif

#ifndef CORRADE_TARGET_EMSCRIPTEN
const UnsignedInt timeBefore = _minimalLoopPeriod ? SDL_GetTicks() : 0;
const Nanoseconds timeBefore = _minimalLoopPeriodMilliseconds ? SDL_GetTicks()*1.0_msec : Nanoseconds{};
#endif

#ifdef CORRADE_TARGET_EMSCRIPTEN
Expand Down Expand Up @@ -1032,10 +1047,10 @@ bool Sdl2Application::mainLoopIteration() {

#ifndef CORRADE_TARGET_EMSCRIPTEN
/* If VSync is not enabled, delay to prevent CPU hogging (if set) */
if(!(_flags & Flag::VSyncEnabled) && _minimalLoopPeriod) {
const UnsignedInt loopTime = SDL_GetTicks() - timeBefore;
if(loopTime < _minimalLoopPeriod)
SDL_Delay(_minimalLoopPeriod - loopTime);
if(!(_flags & Flag::VSyncEnabled) && _minimalLoopPeriodMilliseconds) {
const Nanoseconds loopTime = SDL_GetTicks()*1.0_msec - timeBefore;
if(loopTime < _minimalLoopPeriodMilliseconds*1.0_msec)
SDL_Delay(_minimalLoopPeriodMilliseconds - loopTime/1.0_msec);
}
#endif

Expand All @@ -1044,10 +1059,10 @@ bool Sdl2Application::mainLoopIteration() {

#ifndef CORRADE_TARGET_EMSCRIPTEN
/* If not drawing anything, delay to prevent CPU hogging (if set) */
if(_minimalLoopPeriod) {
const UnsignedInt loopTime = SDL_GetTicks() - timeBefore;
if(loopTime < _minimalLoopPeriod)
SDL_Delay(_minimalLoopPeriod - loopTime);
if(_minimalLoopPeriodMilliseconds) {
const Nanoseconds loopTime = SDL_GetTicks()*1.0_msec - timeBefore;
if(loopTime < _minimalLoopPeriodMilliseconds*1.0_msec)
SDL_Delay(_minimalLoopPeriodMilliseconds - loopTime/1.0_msec);
}

/* Then, if the tick event doesn't need to be called periodically, wait
Expand Down
40 changes: 29 additions & 11 deletions src/Magnum/Platform/Sdl2Application.h
Original file line number Diff line number Diff line change
Expand Up @@ -897,18 +897,35 @@ class Sdl2Application {
#ifndef CORRADE_TARGET_EMSCRIPTEN
/**
* @brief Set minimal loop period
*
* This setting reduces the main loop frequency in case VSync is
* not/cannot be enabled or no drawing is done. Default is @cpp 0 @ce
* (i.e. looping at maximum frequency). If the application is drawing
* on the screen and VSync is enabled, this setting is ignored.
* @m_since_latest
*
* This setting reduces the main loop frequency in case
* @ref setSwapInterval() wasn't called at all, was called with
* @cpp 0 @ce, the call failed, or no drawing is done and just
* @ref tickEvent() is being executed. The @p time is expected to be
* non-negative, default is @cpp 0_nsec @ce (i.e., looping at maximum
* frequency). If the application is drawing on the screen and VSync
* was enabled by calling @ref setSwapInterval(), this setting is
* ignored.
*
* Note that SDL timer resolution is just milliseconds, so anything
* below @cpp 1.0_msec @ce will behave the same as @cpp 0_nsec @ce. As
* the VSync default is driver-dependent, @ref setSwapInterval() has to
* be explicitly called to make the interaction between the two work
* correctly.
* @note Not available in @ref CORRADE_TARGET_EMSCRIPTEN "Emscripten",
* the browser is managing the frequency instead.
* @see @ref setSwapInterval()
*/
void setMinimalLoopPeriod(UnsignedInt milliseconds) {
_minimalLoopPeriod = milliseconds;
}
void setMinimalLoopPeriod(Nanoseconds time);

#ifdef MAGNUM_BUILD_DEPRECATED
/**
* @brief @copybrief setMinimalLoopPeriod(Nanoseconds)
* @m_deprecated_since_latest Use @ref setMinimalLoopPeriod(Nanoseconds),
* which prevents unit mismatch errors, instead.
*/
CORRADE_DEPRECATED("use setMinimalLoopPeriod(Nanoseconds) instead") void setMinimalLoopPeriod(UnsignedInt milliseconds);
#endif
#endif

/**
Expand Down Expand Up @@ -1204,7 +1221,7 @@ class Sdl2Application {
* If implemented, this function is called periodically after
* processing all input events and before draw event even though there
* might be no input events and redraw is not requested. Useful e.g.
* for asynchronous task polling. Use @ref setMinimalLoopPeriod()/
* for asynchronous task polling. Use @ref setMinimalLoopPeriod() /
* @ref setSwapInterval() to control main loop frequency.
*
* If this implementation gets called from its @cpp override @ce, it
Expand Down Expand Up @@ -1253,7 +1270,8 @@ class Sdl2Application {

#ifndef CORRADE_TARGET_EMSCRIPTEN
SDL_Window* _window{};
UnsignedInt _minimalLoopPeriod;
/* Not using Nanoseconds as that would require including Time.h */
UnsignedInt _minimalLoopPeriodMilliseconds{};
#else
SDL_Surface* _surface{};
Vector2i _lastKnownCanvasSize;
Expand Down
2 changes: 1 addition & 1 deletion src/Magnum/Platform/Test/Sdl2ApplicationTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ struct Sdl2ApplicationTest: Platform::Application {
when redrawing constantly. */
#if 0
void tickEvent() override {
setMinimalLoopPeriod(250);
setMinimalLoopPeriod(250.0_msec);
Debug{} << "tick event:" << Seconds{SDL_GetTicks()*1.0_msec};
}
#endif
Expand Down

0 comments on commit ba0fbe3

Please sign in to comment.