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

Base zoom speed on browser refresh rate #12211

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lukemckinstry
Copy link
Contributor

Description

Bases zoom speed on the render rate of the user's browser.
Added a field in frameState to track the timestamps of the 10 most recent renders.
Then when calculating the zoom distance on each frame, adjust it based on the fps zoom speed to provide consistent experience for browsers with different speeds.
The if the user wants faster or slower zoom, they can still adjust zoomFactor through the public api as before with the same effects.

Issue number and link

#12187

Testing plan

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @lukemckinstry!

✅ We can confirm we have a CLA on file for you.

@lukemckinstry lukemckinstry changed the title Base zoom speed on browser frames per second Base zoom speed on browser refresh rate Sep 23, 2024
@lukemckinstry
Copy link
Contributor Author

posted this for initial review/feedback/discussion @ggetz. I still need to add unit tests.

@javagl
Copy link
Contributor

javagl commented Sep 24, 2024

This looks somewhat similar to what I dumped into a forum thread a while ago. The goal there was also to have an "average frame rate over the last n frames".

In that forum thread, I put this into some FpsTracker class. The reasons may be obvious. The functionality is encapsulated in a class where the name says what the class does (/* An FpsTracker tracks the FPS */ 😁 ). It offers a getAverageFps function directly. It avoids that const timeStamps = object._scene.frameState?.timeStamps; (aka Train Wreck). And it avoids the manual computation (with magic constants like 10 and 30) inside the handleZoom function.

The purpose of that FpsTracker there was to plug it into some MsseUpdater, which was supposed the update the maximum screen space error based on the average FPS. (Fleshing this out into a real feature is still on my TODO list - I think that this could make a whole lot of sense...).

tl;dr : If there was "something" where a function like getAverageFps was offered, then this could probably be re-used in multiple places...

@lukemckinstry
Copy link
Contributor Author

I like your idea to create a FpsTracker class to make this more reusable. I will work on this change and update this PR when it is ready.

@ggetz
Copy link
Contributor

ggetz commented Sep 24, 2024

I like the idea of isolating the average FPS logic in one place. The update itself should probably still happen in Scene.js though, (as @lukemckinstry just implemented).

@@ -162,6 +163,7 @@ function Scene(options) {
this._frameState.scene3DOnly = defaultValue(options.scene3DOnly, false);
this._removeCreditContainer = !hasCreditContainer;
this._creditContainer = creditContainer;
this._fpsTracker = new FpsTracker();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are accessing this from another class, can we expose this as fpsTracker with a getter? It can be marked as @private so it doesn't show up in the developer API documentation.

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