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

Make cameraMoveEvent spec more stable #7496

Merged
merged 2 commits into from
Jan 22, 2019
Merged

Make cameraMoveEvent spec more stable #7496

merged 2 commits into from
Jan 22, 2019

Conversation

OmarShehata
Copy link
Contributor

@OmarShehata OmarShehata commented Jan 21, 2019

This is the final step in resolving the commonly occurring test failures on Travis (#7249 (comment)). I will bump this PR to @lilleyse once run a couple times to verify that it no longer breaks.

I figured out the reason why there were two render() calls in that spec, and documented it for posterity. I had to set s.cameraEventWaitTime to -1.0 because View.checkForCameraUpdates does not use Scene time to check this, it uses an absolute timestamp:

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/View.js#L118-L138

So it's very likely that this line:

if (this._cameraStartFired && getTimestamp() - this._cameraMovedTime > scene.cameraEventWaitTime) {

Resolves to getTimestamp() - this._cameraMovedTime equal to 0. So even with scene.cameraEventWaitTime = 0.0 it doesn't run. The reason why I think setting cameraEventWaitTime to negative in this spec is acceptable is because we want it to occur as soon as possible on the next frame, so this will take care of this edge case. Changing this equality to >= should fix it as well but this seems like a safer change to make.

Number of times successfully ran in Travis in a row: 4

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@OmarShehata
Copy link
Contributor Author

I think this is stable @lilleyse .

@mramato
Copy link
Contributor

mramato commented Jan 21, 2019

The reason why I think setting cameraEventWaitTime to negative in this spec is acceptable is because we want it to occur as soon as possible on the next frame, so this will take care of this edge case.

This sounds fine to me, but add a comment in the code stating as such (we use negative time so that it triggers immediately).

Also, so that you are away, jasmine can mock real-world time too, so you can actually test that real-world time stamps behave the way they are supposed to: https://jasmine.github.io/api/2.6/Clock.html Of course with this tests, the negative value is probably the better/simpler approach; but the wall clock mocking is something you (and everyone else) should be aware of.

@mramato
Copy link
Contributor

mramato commented Jan 22, 2019

Thanks @OmarShehata!

@mramato mramato merged commit 191c287 into CesiumGS:master Jan 22, 2019
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