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

requestRenderMode Bug on resize #6812

Closed
knackjason opened this issue Jul 17, 2018 · 13 comments
Closed

requestRenderMode Bug on resize #6812

knackjason opened this issue Jul 17, 2018 · 13 comments

Comments

@knackjason
Copy link

I believe I've found a bug related to requestRenderMode in Firefox. I asked about it in the forums (https://groups.google.com/forum/#!topic/cesium-dev/cHR1qyjMKeo) and a community member suggested I file a bug.

Essentially, Cesium doesn't stop the update/render process in Firefox until you interact (move, zoom) with the globe after the page initially loads. You can see this happen by visiting the Sandcastle demo at https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=Scene%20Rendering%20Performance.html. In Chrome, the FPS flatline to "N/A" within a few seconds after page load. The FPS never flatline after the page load in Firefox, however, until you move the globe or zoom in.

Cesium: 1.4.7
Browser: Firefox 61.0.1
OS: CentOS 7.5

@ggetz
Copy link
Contributor

ggetz commented Jul 18, 2018

Thanks @knackjason for the report!

I'm using Firefox 61.0 and Cesium 1.47 I'm not able to replicate (the FPS go to N/A within a second or so), so this may take more investigation.

If you're able to debug, I would watch to see if any functions are getting added to the FrameState.afterRender array, as that's likely why the render loop is continually running.

@knackjason
Copy link
Author

@ggetz what OS are you using? I tried Firefox 61 on Windows and also saw the FPS go to N/A within a second. This is not happening for me in Linux (CentOS 7.5), so maybe it's more of a bug with Firefox on Linux.

I'm happy to help debug but am also brand new to Cesium. Can you point me in the direction of how I'd watch the FrameState.afterRender array?

@hpinkos
Copy link
Contributor

hpinkos commented Jul 18, 2018

@mramato can you try this out on your linux machine?

@mramato
Copy link
Contributor

mramato commented Jul 18, 2018

I can reproduce this on Windows/Linux with either Chrome or Firefox.

  1. Dock the browser to one side of the screen
  2. Load the demo (everything is fine)
  3. Maximum the browser (it will keep rendering forever)

@knackjason
Copy link
Author

Going off of what @mramato said, I'm also noticing that Cesium will start rendering forever in an unmaximized window after any kind of browser resize (whether it's maximizing or just increasing/decreasing with the mouse).

@mramato mramato changed the title requestRenderMode Bug in Firefox requestRenderMode Bug on resize Jul 18, 2018
@mramato
Copy link
Contributor

mramato commented Jul 18, 2018

I updated the title and marked this as next release since it seems like a pretty bad break for requestRenderMode.

@mramato
Copy link
Contributor

mramato commented Jul 18, 2018

Thanks again for reporting @knackjason!

@knackjason
Copy link
Author

No problem!

@lukesanantonio
Copy link
Contributor

lukesanantonio commented Jul 18, 2018

I can confirm this behavior in both Chrome and Firefox on my mac.

I couldn't pin down a commit with git bisect (full log here) but I did learn some things:

  • Everything is good up until 7c46e11 (inclusive),
  • e331d4c broke requestRenderMode entirely - rendering never stops, resize or not (this is why I had to skip a bunch of commits while bisecting),
  • 22ecbe6 is the first commit that exhibits the current behavior.

I don't think I know enough to debug this any further but based on those commits @bagnell might know.

@lukesanantonio
Copy link
Contributor

lukesanantonio commented Jul 18, 2018

Actually I think I've tracked this down. The camera frustum changes because the viewport resizes but the checkForCameraUpdates function doesn't register the change. this._frustumChanged always evaluates to true because this._cameraClone isn't updated (see Scene.js#L3313).

EDIT: e331d4c is when this._frustumChanged was introduced, 22ecbe6 is when it was changed to use .equals instead of !==.

lukesanantonio pushed a commit to lukesanantonio/cesium that referenced this issue Jul 19, 2018
Before this change, the user could update the frustum but not the camera
by resizing the viewer. The Scene would continue to render even when
requestRenderMode was enabled because it recognized the frustum had
changed, but would not update the cached camera to mark the change as
handled. This commit changes cameraEqual (in Scene.js) to take into
account the camera frustum.

Fixes CesiumGS#6812
lukesanantonio pushed a commit to lukesanantonio/cesium that referenced this issue Jul 19, 2018
Before this change, the user could update the frustum but not the camera
by resizing the viewer. The Scene would continue to render even when
requestRenderMode was enabled because it recognized the frustum had
changed, but would not update the cached camera to mark the change as
handled. This commit changes cameraEqual (in Scene.js) to take into
account the camera frustum.

Fixes CesiumGS#6812
lukesanantonio pushed a commit to lukesanantonio/cesium that referenced this issue Jul 19, 2018
Before this change, the user could update the frustum but not the camera
by resizing the viewer. The Scene would continue to render even when
requestRenderMode was enabled because it recognized the frustum had
changed, but would not update the cached camera to mark the change as
handled. This commit changes cameraEqual (in Scene.js) to take into
account the camera frustum.

Fixes CesiumGS#6812
lukesanantonio added a commit to lukesanantonio/cesium that referenced this issue Jul 26, 2018
lukesanantonio added a commit to lukesanantonio/cesium that referenced this issue Jul 26, 2018
@mramato
Copy link
Contributor

mramato commented Jul 30, 2018

Just a reminder that the release is this Wednesday. We should really try and get all showstopper issues addressed by the end of today.

@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/cHR1qyjMKeo

If this issue affects any of these threads, please post a comment like the following:

The issue at #6812 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@knackjason
Copy link
Author

Thanks, @lukesanantonio and everyone else for the fix! I'm looking forward to getting it on Wednesday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants