Clock should not animate by default #6154
Conversation
|
@ggetz, thanks for the pull request! Maintainers, we have a signed CLA from @ggetz, so you can review this at any time. I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
| @@ -90,6 +90,7 @@ | |||
| selectionIndicator : false, | |||
| shadows : true | |||
| }); | |||
| viewer.clockViewModel.shouldAnimate = true; | |||
There was a problem hiding this comment.
I would recommend changing all of these to simple viewer.clock.shouldAnimate, since it's clearer and doesn't require the timeline/animation widget to be present.
There was a problem hiding this comment.
Also, does this particular example need animation? May be better to just not animate here (and choose a time that shows shadows so they always toggle, which right now only happens if the clock happens to be at a certain time).
| @@ -57,6 +57,7 @@ | |||
| }]; | |||
|
|
|||
| var viewer = new Cesium.Viewer('cesiumContainer'); | |||
| viewer.clockViewModel.shouldAnimate = true;cz, | |||
CHANGES.md
Outdated
| @@ -4,7 +4,7 @@ Change Log | |||
| ### 1.42 - 2018-02-01 | |||
|
|
|||
| * Breaking changes | |||
| * | |||
| * The clock does not animate by default. Set the viewer `ClockViewModel.shouldAnimate` property to `true` to enable animation. | |||
There was a problem hiding this comment.
Let's use viewer.clock.shouldAnimate here instead of "viewer ClockViewModel.shouldAnimate"
|
👍 This also clears up a mistake I keep making where I disable the timeline and animation widgets, but time is still ticking along behind the scenes unbeknownst to me. |
|
This seems like a big breaking change to me, and I'm not sure we should do it so soon. Many time dynamic apps will need to be fixed. I can imagine someone not familiar with |
How many time dynamic apps don't provide a means to control animation though? This just changes the default state to not animating. |
Yes, you can still always just hit "play" on the clock |
|
One example might be if an app is listening to Any apps that involve resources expiring may break. While not widely used or even officially part of the 3D Tiles spec yet, There are also plenty of apps that expect to auto-play, and authors of those apps will be confused why it doesn't start right away anymore. Overall I'm just saying that there may be a lot of unintended consequences related to this change. |
Do you have data to back up this assertion? |
|
We could add a one time developer warning for a few releases? I don't think we can change this functionality without outright breaking it at some point. |
What exactly do you have in mind? Simply writing out to the console that the scene no longer animates by default? I'm not sure that buys us anything. This is a trivial change for a dev to make and will have a bullet point in CHANGES. I honestly don't think it will be a big deal at all. We've made larger breaking changes. |
|
|
Added |
|
Thanks @ggetz, I'll merge as soon as this goes green (assuming no one beat me to it) |
As discussed in #6115 (comment)
Since use cases where the viewer should animate are the exception, not the norm, we should not be animating by default.