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

Resolving #2153 Added ability to set frame-lock. #2765

Closed
wants to merge 4 commits into from

Conversation

TheoTheDev
Copy link
Contributor

Added ability to increase / decrease max FPS.
By default disabled.
By pressing shift & - -> decrease the limit.
By pressing shift & + -> increase the limit.
Also updated stats FPS with render FPS & loopFPS:
image

  • in console you can see what exact limit was set [after changing the limit].

@TheoTheDev TheoTheDev linked an issue Apr 6, 2022 that may be closed by this pull request
@TheoTheDev TheoTheDev requested review from avaer and 0reo April 6, 2022 05:30
@TheoTheDev TheoTheDev self-assigned this Apr 6, 2022
@TheoTheDev TheoTheDev added the 3d label Apr 6, 2022
@TheoTheDev TheoTheDev added this to the Perofrmance milestone Apr 6, 2022
@TheoTheDev TheoTheDev requested a review from alisaad673 April 6, 2022 20:02
Copy link
Contributor

@0reo 0reo left a comment

Choose a reason for hiding this comment

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

Few readability changes would be good

src/components/app/App.jsx Outdated Show resolved Hide resolved
src/components/app/App.jsx Outdated Show resolved Hide resolved
@TheoTheDev
Copy link
Contributor Author

Added comments as requested + added check is typing inside of input [in this case don't apply].

Copy link
Contributor

@0reo 0reo left a comment

Choose a reason for hiding this comment

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

should be good for testing @alisaad673

@@ -141,6 +141,43 @@ export const App = () => {

}, [] );

useEffect( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a good place to hook in the functionality. It sounds more like a job for ioManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and moved key handling to ioManager.

@alisaad673
Copy link
Contributor

Decreasing and increasing the limit seems to be working fine but render fps don't go be beyond 60 fps even if the limit is set at 100fps. Is it dependent on refresh rate of screen?

bandicam.2022-04-13.00-15-51-603.mp4

@TheoTheDev
Copy link
Contributor Author

Decreasing and increasing the limit seems to be working fine but render fps don't go be beyond 60 fps even if the limit is set at 100fps. Is it dependent on refresh rate of screen?

bandicam.2022-04-13.00-15-51-603.mp4

Usually max refresh rate is 60fps. But if your device has low performance and you get max 40fps for example you can not go beyond this physical limit of computation power of your device.

@avaer
Copy link
Contributor

avaer commented Apr 14, 2022

Usually max refresh rate is 60fps.

This should be determined by the browser and how fast it fires requestAnimationFrame. That is not necessarily 60FPS.

(note that we should not hardcode any values here)

@TheoTheDev
Copy link
Contributor Author

Usually max refresh rate is 60fps.

This should be determined by the browser and how fast it fires requestAnimationFrame. That is not necessarily 60FPS.

(note that we should not hardcode any values here)

so anything to be changed ?
you want to replace 2 * 16.6 with a constant and that's it ?
anything else ?

@@ -71,6 +71,10 @@ export default class Webaverse extends EventTarget {
constructor() {
super();

this.lastRenderTime = 0;
this.renderFrame = 0;
Copy link
Contributor

@avaer avaer Apr 22, 2022

Choose a reason for hiding this comment

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

These are poor variable names. Is it the number of frames? A Boolean? A time?

@@ -570,6 +570,30 @@ ioManager.keydown = e => {
debug.toggle();
break;
}
case 187: { // + & shift [plus - not on num-pad]
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than this keyboard hack, this should be in the settings.

@avaer
Copy link
Contributor

avaer commented Apr 27, 2022

This isn't likely to ship.

@avaer avaer closed this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App: add FPS limiter
4 participants