cesium icon indicating copy to clipboard operation
cesium copied to clipboard

Base zoom speed on browser refresh rate

Open lukemckinstry opened this issue 1 year ago • 7 comments

Description

Bases zoom speed on the render rate of the user's browser. Added fields in FrameRateMonitor to track the timestamps of the n 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. 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

https://github.com/CesiumGS/cesium/issues/12187

Testing plan

Author checklist

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

lukemckinstry avatar Sep 23 '24 19:09 lukemckinstry

Thank you for the pull request, @lukemckinstry!

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

github-actions[bot] avatar Sep 23 '24 19:09 github-actions[bot]

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

lukemckinstry avatar Sep 23 '24 19:09 lukemckinstry

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...

javagl avatar Sep 24 '24 12:09 javagl

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.

lukemckinstry avatar Sep 24 '24 18:09 lukemckinstry

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).

ggetz avatar Sep 24 '24 20:09 ggetz

We realized the existing FrameRateMonitor class provides close to the functionality this feature needs. We decided to make some small additions to it rather than create a new class (side notes: All the work up to this commit 331650c67d80b9011ec15a077fa7c8ea6e8b6f6d got re-written, so it may make sense to squash the early commits )

There may be questions as to why we dont get FPS from FrameRateMonitor.lastFramesPerSecond, this should be made clear by the doc updates included in this PR.

lukemckinstry avatar Sep 26 '24 18:09 lukemckinstry

Please excuse the comments on outdated code, I'm not sure how they snuck in there.

ggetz avatar Oct 01 '24 17:10 ggetz

@lukemckinstry I'm going to close this to keep things tidy. If you or anyone else wants to update and finish this out, please feel free to re-open!

ggetz avatar Nov 08 '24 18:11 ggetz