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

ofFpsCounter with more precision using straight std::chrono #7966

Merged
merged 17 commits into from
Jul 16, 2024

Conversation

dimitre
Copy link
Member

@dimitre dimitre commented May 16, 2024

ofFpsCounter with more precision using straight std::chrono when possible.
filterAlpha is being calculated with double.
the removal of some includes shows other files were depending of the includes included here, so now they are more explicit.

this PR:

  • return better precision in ofFpsCounter
  • fixes one error where getLastFrameFilteredNanos is equal to getLastFrameNanos
  • Simplify headers inclusion in other files.

@dimitre dimitre marked this pull request as draft May 16, 2024 15:54
@dimitre dimitre marked this pull request as ready for review May 17, 2024 16:15
@dimitre
Copy link
Member Author

dimitre commented May 17, 2024

Finally! ready to go @ofTheo

@dimitre dimitre changed the title [WIP] ofFpsCounter with more precision using straight std::chrono ofFpsCounter with more precision using straight std::chrono May 20, 2024
@dimitre dimitre requested a review from NickHardeman May 21, 2024 00:19
@ofTheo
Copy link
Member

ofTheo commented May 22, 2024

oh cool @dimitre - going to really hammer it this week!
be great to get some extra eyes on this too as its pretty core to a lot of things :)

@artificiel @2bbb @NickHardeman

@dimitre
Copy link
Member Author

dimitre commented May 22, 2024

Nice. It is a direct replacement line by line, and keeping the same logic from original fps counter.
Using std::chrono allows the same code for all platforms, with the maximum precision possible allowed by the time resolution in a specific platform.
As it was possible to remove ofUtils.h include I had to add back includes in lots of other OF parts, which is good.
The most direct the dependencies are, less things for preprocessing and compiling.

@dimitre dimitre requested a review from danoli3 May 28, 2024 16:49
@NickHardeman
Copy link
Contributor

@dimitre, I just did a simple test with setting the fps of the app and recalculating via ofFpsCounter and it looked good to me.

While looking at the PR, I noticed these namespace lines at the top of ofTimeFps.h

using namespace std::chrono;
using namespace std::chrono_literals;

I think the appropriate std::chrono's should be prepended to the vars in the .h file and the above lines moved to ofTimeFps.cpp. What do you think? Otherwise the using namespace std::chrono; and using namespace std::chrono_literals; is global.

std::chrono::time_point<std::chrono::steady_clock> wakeTime;
std::chrono::time_point<std::chrono::steady_clock> lastWakeTime;

@dimitre
Copy link
Member Author

dimitre commented May 30, 2024

Thank you Nick, good catch. I'll remove this from .h files

@artificiel
Copy link
Contributor

this PR circumvents ofTime, which has platform #ifdefs etc (I think that pre-dates std::chrono?).

could the work here expand into simplifying the std::chrono in ofTime itself? so the timing functions internals are all based on the same core, in the sense that ofGetElapsedTimef() itself gets "upgraded"?

@dimitre
Copy link
Member Author

dimitre commented Jun 1, 2024

@artificiel good idea, ofTime can be updated so all internal operations are done using std::chrono.
I still think this one can be done with straight chrono as it will reduce .h includes

Copy link
Member

@danoli3 danoli3 left a comment

Choose a reason for hiding this comment

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

Awesome keen to try this out

@danoli3
Copy link
Member

danoli3 commented Jul 16, 2024

Yeah if you want to wrap it with the ofTime stuff now or later up to you

@dimitre
Copy link
Member Author

dimitre commented Jul 16, 2024

@danoli3 it is just personal opinion but I don't see why using ofTime in 2024.

@danoli3
Copy link
Member

danoli3 commented Jul 16, 2024

okay if we passing on all targets lets get it in

@danoli3
Copy link
Member

danoli3 commented Jul 16, 2024

could you rebase it

@dimitre
Copy link
Member Author

dimitre commented Jul 16, 2024

Rebased. No conflicts here

@dimitre
Copy link
Member Author

dimitre commented Jul 16, 2024

@danoli3 outstanding work on the core, thanks for that.
I'll be back in OF Core / PG things late this month

@danoli3 danoli3 merged commit d47d87e into openframeworks:master Jul 16, 2024
16 checks passed
@dimitre dimitre deleted the fpscounter branch July 16, 2024 21:45
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.

None yet

5 participants