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

Workaround against SDL_GetPerformanceCounter that sometimes stalls #6114

Closed
wants to merge 1 commit into from

Conversation

pthom
Copy link
Contributor

@pthom pthom commented Jan 23, 2023

This PR covers a quite uncommon corner case with SDL under emscripten.

Consecutive calls to SDL_GetPerformanceCounter() might lead to the same result, which will result in IM_ASSERT(g.IO.DeltaTime > 0.0f) to fail later, inside ImGui::ErrorCheckNewFrameSanityChecks()

This happens only under emscripten, and with specific javascript engines (it is quite common with Safari under macOS, but does not happen with firefox under macOS).

@ocornut
Copy link
Owner

ocornut commented Jan 24, 2023

Hello,

Seems to be the same as #3644
I'd be ok adding the workaround under an #ifdef block, but perhaps it seems more adequate the reuse the previous value? or a very small value? or somehow find way to get high-precision timer? I'll move the discussion to #3644..

Under emscripten, consecutive calls to SDL_GetPerformanceCounter() might
lead to the same result, which will result in IM_ASSERT(g.IO.DeltaTime > 0.0f)
to fail later, inside ImGui::ErrorCheckNewFrameSanityChecks()

We store the last positive DeltaTime in a static variable
and return this if a stall is detected.
@pthom
Copy link
Contributor Author

pthom commented Jan 24, 2023

Hello Omar,

Seems to be the same as #3644

Absolutely!

I added an ifdef, and reused the previous value when a stall is detected.

FYI, this issue happens once every 600 frames or so on Safari/macOS, so that on my side an emscripten app has an average survival time of 10 seconds :-)

@ocornut
Copy link
Owner

ocornut commented Jan 24, 2023

What's your average framerate in that app?
Can you explain/understand why you are getting a zero here?

In your situation (rare/occasional 0.0f) it seems like using the smallest values (e.g. 0.00001f) may be more correct than last DeltaTime.

@pthom
Copy link
Contributor Author

pthom commented Jan 24, 2023

What's your average framerate in that app?

My app average framerate is 60fps.

Can you explain/understand why you are getting a zero here?

I will try to study that. No that easy though, since any slight instrumentation might change the behavior (this bug might disappear depending on the compilation etc. It seems more present when the application is heavy. Eek...)

In your situation (rare/occasional 0.0f) it seems like using the smallest values (e.g. 0.00001f) may be more correct than last DeltaTime.

Right


While I have your attention, during these last months, I discreetly worked on a library around ImGui which group various libraries from the ecosystem, and enables to easily create ImGui applications in C++ and Python.
I did not communicate publicly about it yet (although some people have found it by digging GitHub), but I plan to communicate about it quite soon.

If you have time, here are some links to it:

Cheers!

PS: my apologies if you feel like I tell you about this library too late, I wanted it to be nice looking and a bit battle tested before going further.

@ocornut
Copy link
Owner

ocornut commented Jan 24, 2023

Thanks! I have noticed this in the past few weeks (I have your remote showing in my git ui as I'll try to resume work to facilitate imgui_manual and other things0.

It looks great and will share it now that you've announced it (will also add to wiki etc.)

(PS: consider calling it "Dear ImGui Bundle" when referred to strings/text form, outside of code/namespace ;)

@ocornut
Copy link
Owner

ocornut commented Jan 24, 2023

PS.2: I added it to wiki + made a public post, but feel free to post in Gallery! (and we'll mention it in next Release Notes)

@ocornut
Copy link
Owner

ocornut commented Jan 25, 2023

I have thought about this PR and my conclusions are:

  • We should aim for app and/or backend to provide non-zero delta time (via fixing framerate if known, or smoothing it in extreme cases such as privacy.resistFingerprinting=true.
  • However there is no harm in tolerating zero DeltaTime under Emscripten, which is what I've pushed with 0749061.

Even in the worst case of #3644 (one 0.100 ms, three 0.000 ms) it seems like this would make the issue less prominent.

Thanks for this!

@ocornut ocornut closed this Jan 25, 2023
@pthom
Copy link
Contributor Author

pthom commented Jan 26, 2023

Hello,

However there is no harm in tolerating zero DeltaTime under Emscripten, which is what I've pushed with 0749061.

Thanks! This will simplify my code :-)


PS:
Thanks for twitting about "Dear ImGui Bundle"!

I posted a message about it in the gallery yesterday. I'm a bit worried, because I saw no reaction to this message. I hope it was not perceived as me showing off too much in the wrong place, as it was not my intention at all.

Cheers!

@pthom
Copy link
Contributor Author

pthom commented Feb 13, 2023

Hello,

Just a rather technical information about this issue:

I very much suspect that the fact we sometimes get a timer that stalls is due to mitigation measures against attacks using Spectre. The high precision clock accuracy is reduced by default with WebASM, unless COOP and COEP headers are sent. See https://web.dev/why-coop-coep/

@pthom pthom deleted the SdlEmscriptenClock branch February 13, 2023 07:10
ocornut pushed a commit that referenced this pull request Feb 23, 2023
kjblanchard pushed a commit to kjblanchard/imgui that referenced this pull request May 5, 2023
kjblanchard pushed a commit to kjblanchard/imgui that referenced this pull request May 5, 2023
ocornut added a commit that referenced this pull request Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants