Skip to content

Conversation

@Khitiara
Copy link
Contributor

Summary of the PR

Fixes BreakneckSleep in the existing CPU usage PR to store the last tick timestamp and compare to that instead of to when Sleep is called.
The branch still doesnt actually use BreakneckSleep in the window loop but still this is in theory an improvement

Should _lastTick be a property or left as a field? unsure
@Khitiara Khitiara requested a review from a team as a code owner August 15, 2023 04:05
@Perksey
Copy link
Member

Perksey commented Aug 15, 2023

This does not build, but also I’m not sure I agree with the implementation. BreakneckSleep is a readonly struct, being a thin wrapper over a sleep configuration essentially. Adding mutable state like this counteracts that. Moreover, I’d rather data structs like these expose the data therein.

I think maybe we should look at the Sleep method. Maybe add a new TimeSpan parameter called “lastOvershoot” (default it as well) and change the return type to TimeSpan to allow the user to control the tracking of frame time deficits. This type has no concept of a frame time, and it may not necessarily be the case that Sleep is called in sequence/regular intervals. This current implementation would not be appropriate for that generalisation in this case.

There is an outstanding todo on my original PR to add a MaxThrottle property to IViewProperties to integrate BreakneckSleep and allow the user to control how fast they loop.

Copy link
Member

@Perksey Perksey left a comment

Choose a reason for hiding this comment

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

If you wanted to add a dedicated limiter type lmk, otherwise I’m happy to merge this.

@Khitiara
Copy link
Contributor Author

im good for now, can always do another go with a proper limiter type later but im not sure of how best to handle the intricacies there

@Perksey Perksey merged commit 8b21c93 into dotnet:hotfix/fidgity-spinny Aug 18, 2023
@Khitiara Khitiara deleted the hotfix/fidgity-spinny branch August 19, 2023 02:37
@Khitiara Khitiara restored the hotfix/fidgity-spinny branch August 19, 2023 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants