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

[FPS] Update: Provide more accurate values for frames per second #939

Merged
merged 2 commits into from
Jul 30, 2023

Conversation

tristanbob
Copy link
Contributor

@tristanbob tristanbob commented Jul 17, 2023

  • Uses a sliding window to show exactly the number of frames in the previous second
  • Backwards compatible with the previous extension

Playable game

https://gd.games/victrisgames/load-testing

Game example

GDevelopApp/GDevelop-examples#544

Video

8mb.video-F5t-cJnxWZvi.mp4

"type": "expression"
}
],
"objectGroups": []
},
{
"description": "Change time period between FPS updates. Default: 1 second.",
Copy link
Contributor

@D8H D8H Jul 17, 2023

Choose a reason for hiding this comment

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

This is probably not necessary. People only need to check that their game stays at the maximum FPS possible, they don't need a precise variation speed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some people want to see FPS updates EVERY SINGLE FRAME (which is hard to watch and not very useful). But most people want to see the values averaged over some time period. One second is a good default, but I like to give users control over this.

Comment on lines 59 to 176
"fullName": "Precise FPS (Frames per second)",
"fullName": "Precise FPS (Frames per second) [Deprecated]",
Copy link
Contributor

@D8H D8H Jul 17, 2023

Choose a reason for hiding this comment

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

Why can't the new expression replace this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous expression wasn't really working as intended. I kept this function for backward compatibility, but I would be happy to remove it.

"objectGroups": []
},
{
"description": "Average frames per second (FPS) during the last update period.",
Copy link
Contributor

@D8H D8H Jul 17, 2023

Choose a reason for hiding this comment

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

Suggested change
"description": "Average frames per second (FPS) during the last update period.",
"description": "Average frames per second (FPS) during the last update period. When measuring performance, rendering time should be used instead of FPS because going from 100 to 80 FPS is 2.5 ms difference where going from 40 to 20 FPS is 25 ms difference and FPS takes the waiting time into account.

Copy link
Contributor Author

@tristanbob tristanbob Jul 19, 2023

Choose a reason for hiding this comment

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

This is an interesting commentary, but I don't think it makes sense to live here.

We could consider including another expression that returns TimeToRenderFrame (but that sounds a lot like DeltaTime, assuming timescale = 1).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just to warn users that FPS is not a profiling tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could do a similar "AverageRenderTime" that works similar to FPS (taking an average of the measuring period). Do you think that would be useful?

Copy link
Contributor

@D8H D8H Jul 19, 2023

Choose a reason for hiding this comment

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

No, you can't measure the rendering time with events because it measures the waiting time too.
GDevelop or browser profilers must be used.

Copy link
Contributor

@D8H D8H Jul 19, 2023

Choose a reason for hiding this comment

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

Maybe something like this:

An FPS counter is not a way to profile performance. GDevelop profiler must be used for this usage.

@arthuro555
Copy link
Member

Hmmm I think the implementation could be done better. The UpdatePeriod is bad since the FPS expression does not show the current FPS in real time but the last measured one among other things.

Instead, you could push a current timestamp onto an array every frame, and pop all the timestamps more than 1 second old from the array. That way, the array always contains the list of frames captured within the last second, and you just need to get the length of the array to know the amount of frames within the last second.

For correctness sake, a frame should also not be counted before it has fully finished. when onStepPreEvents runs, there are still objects, behaviors, and rendering that may run and are not counted as part of the frame. GDevelop does not offer a way to run code at the end of the events loop, but luckily we can use JavaScript for that. queueMicrotask will run a function when GDevelop's javascript has finished doing all that it is doing to render a frame and gives control back to the browser. That's the moment where we should record a frame, preferably with performance.now() for getting a precise performance measuring timestamp.

Here is my updated version (game example with updated extension): gdevelop-game(2).zip

@D8H
Copy link
Contributor

D8H commented Jul 19, 2023

Let's try to keep it simple.

The need is only to check that there is no skipped frames. I didn't look at the events but Tristan's solution seems simple and it already fits the need.

Sure, we could use a fancy sliding window, but I won't make it better for users. It will just make it harder to maintain.

@tristanbob tristanbob merged commit e5859f7 into main Jul 30, 2023
1 check passed
@tristanbob
Copy link
Contributor Author

Thanks @arthuro555 and @D8H for your help reviewing this!

@tristanbob tristanbob deleted the fps-update branch July 30, 2023 16:02
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