-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Update Viewer on preUpdate instead of clock tick #7069
Conversation
Thanks for the pull request @lilleyse!
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
This seems okay with me. I'll let @mramato make the call on this though |
Looks like you have some test failures. I've actually wanted to explore making this change ever since we added the pre/post render events. I'm honestly not that worried about it (because as you said, the tick/preRender always get called back to back in our own internal render loop). That being said, we should still hammer on this before merging, so I'll take a look at it now. |
Source/Widgets/Viewer/Viewer.js
Outdated
Viewer.prototype._onTick = function(clock) { | ||
var time = clock.currentTime; | ||
|
||
Viewer.prototype._onUpdate = function(scene, time) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name this _onPreUpdate
just so it's more obvious.
Renamed the function and fixed the tests (had the commit locally but forgot to push - it shows up above the comments). |
Thanks @lilleyse. The build failed for seemingly unrelated reasons that looks like they might be related to external urls. |
This changes the viewer to be updated on preUpdate rather than clock tick from this idea in #7204.
This allows entity changes to be reflected in the pick pass.
I'm a little hesitant about making a core change like this but it seems mostly safe as
clock.tick()
andscene.render()
are called right after another in CesiumWidget.this branch vs. master