-
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
Widget tweaks #870
Merged
Merged
Widget tweaks #870
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
3e12f0b
Widget tweaks
mramato a7f5f2f
Changes after review
mramato 6a08676
Update CHANGES.
mramato 7cb8c63
Fix copy/paste error.
mramato 2ad8f01
Declare members in the constructor.
mramato 37f5049
Cleanup variable initialization order.
mramato 97f3e25
Optimize resizing for Viewer and related widgets.
mramato aaba55b
Hopefully final code changes to make Viewer resizing work again in al…
mramato a2170c7
Fix typo in documentation.
shunter c63b9ea
Moew resize cleanup
mramato 9771e8c
Merge branch 'master' into widgetCleanup
mramato 2bb904c
Ad `onRenderLoopError` events to `Viewer` and `CesiumWidget` for exce…
mramato de46e94
Paint the bike shed blue.
mramato 71d9b57
Remove unused module.
shunter 21b9fcd
Fix missing px when setting style.
shunter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The problem here is that setting this to
false
andtrue
repeatedly inside a single animation frame could kick off multiple render loops. We're assuming that this will stayfalse
long enough that the current loop will fire and fizzle one last time.Instead, we could have a private boolean tracking whether the render loop is active or not, that would get set to false on the next animation frame after the user turns this off. That way we could be certain that the previous loop has ended. The downside is, if the render loop crashed, the value would never get set false, and you could never start a new loop. But, that's the situation master is in right now. Not sure if there's a better plan. I think the official spec of
requestAnimationFrame
returns a handle to the request that can be cancelled directly, but of course we're using polyfills and vendor-prefixed versions of that function that may not follow the spec correctly there.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.
This is a good point, and I'll look to address it in the code before this comes in. It used to be that the official spec for
requestAnimationFrame
had no way to cancel it and Firefox had support but it was non-standard. Looking at the spec on MDN, it looks like we can actually cancel the running request. https://developer.mozilla.org/en-US/docs/Web/API/window.requestAnimationFrameThere 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.
You could just make sure to clear the flag at the very top of render, before doing anything else.
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.
Be careful, our implementation of requestAnimationFrame does not preserve the return value.