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

Improve fps counter #1886

Merged
merged 9 commits into from
Jan 12, 2025
Merged

Conversation

GabrielNakamoto
Copy link
Contributor

@GabrielNakamoto GabrielNakamoto commented Jan 5, 2025

Accumulate it over a few frames to give an average instead of showing the last frame performance only

  • In vtkF3DUIActor I changed SetFpsValue() to UpdateFpsValue() which will automatically accumulate fps values before setting the display value to the average. The number of frames it waits to average is decided by the static member variable vtkF3DUIActor::FramesToAverage

Updating it only when the whole scene is rendered

  • In the vtkF3DRenderer::Render() function I updated it to check if its information has vtkF3DRenderPass::RENDER_UI_ONLY() set to 1 If it does it calculates the fps value and calls the UI Actor's UpdateFpsValue()

Let me know what you think the number of frames to average should be (currently set to 5).

From building it and using some of the example models it seems that the render UI only is working however I may be wrong.

Seeing this is my first time contributing I'm starting a pull request to get some advice.

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.71%. Comparing base (91310bd) to head (fd30857).
Report is 34 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1886      +/-   ##
==========================================
+ Coverage   95.69%   95.71%   +0.01%     
==========================================
  Files         125      125              
  Lines        9926     9970      +44     
==========================================
+ Hits         9499     9543      +44     
  Misses        427      427              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

This is looking good, wdyt @Meakk ?

@mwestphal mwestphal requested a review from Meakk January 6, 2025 07:02
@GabrielNakamoto
Copy link
Contributor Author

GabrielNakamoto commented Jan 6, 2025

I don't think the last commit works but I don't know why?

…ost recent seconds worth of frames to calculate fps average
Copy link
Member

@Meakk Meakk left a comment

Choose a reason for hiding this comment

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

Great! Thank you for your contribution!

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

please convert all tabs to space

@Meakk
Copy link
Member

Meakk commented Jan 12, 2025

please convert all tabs to space

Why is clang-format not detecting that?

@mwestphal
Copy link
Contributor

please convert all tabs to space

Why is clang-format not detecting that?

no idea, to improve definitely

@GabrielNakamoto
Copy link
Contributor Author

I pushed the style changes

Co-authored-by: Mathieu Westphal <[email protected]>
Comment on lines +1554 to +1556
glEndQuery(GL_TIME_ELAPSED);
GLint elapsed;
glGetQueryObjectiv(this->Timer, GL_QUERY_RESULT, &elapsed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
glEndQuery(GL_TIME_ELAPSED);
GLint elapsed;
glGetQueryObjectiv(this->Timer, GL_QUERY_RESULT, &elapsed);
glEndQuery(GL_TIME_ELAPSED);
GLint elapsed;
glGetQueryObjectiv(this->Timer, GL_QUERY_RESULT, &elapsed);

Comment on lines +1550 to +1551
// Get CPU frame time
double elapsedTime = std::chrono::duration_cast<std::chrono::microseconds>(cpuElapsed).count() * 1e-6;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Get CPU frame time
double elapsedTime = std::chrono::duration_cast<std::chrono::microseconds>(cpuElapsed).count() * 1e-6;
// Get CPU frame time
double elapsedTime = std::chrono::duration_cast<std::chrono::microseconds>(cpuElapsed).count() * 1e-6;

Comment on lines +1558 to +1559
// Get min between CPU frame time and GPU frame time
elapsedTime = std::min(elapsedTime, elapsed * 1e-9);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Get min between CPU frame time and GPU frame time
elapsedTime = std::min(elapsedTime, elapsed * 1e-9);
// Get min between CPU frame time and GPU frame time
elapsedTime = std::min(elapsedTime, elapsed * 1e-9);

#endif

this->UIActor->SetFpsValue(fps);
this->UIActor->UpdateFpsValue(elapsedTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this->UIActor->UpdateFpsValue(elapsedTime);
this->UIActor->UpdateFpsValue(elapsedTime);

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

still incorrect indenting, sorrt about that

@Meakk
Copy link
Member

Meakk commented Jan 12, 2025

Let's merge it. Formatting will be handle in #1912

@Meakk Meakk merged commit b134d54 into f3d-app:master Jan 12, 2025
47 checks passed
@Meakk
Copy link
Member

Meakk commented Jan 12, 2025

Thanks @GabrielNakamoto!

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