CZML Interval Edge Case Error

[srothst1]

Fixes #9097

Resolves an error related to removing a CZML datasource when the clock interval has a duration of zero.

The ISO documentation conclusively says that a duration can be zero. The documentation also indicates that the amount of intervening time on a time interval is expressed by a duration. Thus, the original suggestion in #9097 to not permit CZML data with clock intervals of duration 0 does not seem to meet ISO standards.

The methods removeAll() and remove() in DataSourceCollection.js both cause the same problem outlined in the issue. When removing a dataSource with an intervening time of 0, the error stems from the line this._dataSourceRemoved.raiseEvent(this, dataSource); in both methods. I looked at the method raiseEvent in Event.js but was not able to find the cause of the error. Assistance on this matter would be appreciated.

This sandcastle demo shows the different test cases as well as the errors.

@ebogo1
@lilleyse

[cesium-concierge]

Thanks for the pull request @srothst1!

• ✔️ 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.
• ❔ Unit tests were not updated.
  • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

[srothst1]

Given the console error: TypeError: this._dataSourceChangedListeners[id] is not a function, I removed the line this._dataSourceChangedListeners[id]() from Viewer.js. This appears to have fixed both errors shown in my sandcastle demo.

[lilleyse]

Removing this._dataSourceChangedListeners[id](); is simply hiding the problem and mostly likely introducing a bug in the process.

See why the listener never got added in the first place. Step through _onDataSourceAdded and you'll notice that this._dataSourceChangedListeners[id] = removalFunc; never gets hit because of an error thrown in one of the preceding calls.

[srothst1]

@lilleyse It seems like this.clockTrackedDataSource = dataSource; is causing the error, however, I am not seeing exactly what it is on the console in Sandcastle.

One quick fix would be to call this._dataSourceChangedListeners[id] = removalFunc; before updating this.clockTrackedDataSource. This would ensure that this._dataSourceChangedListeners[id](); in Viewer.prototype._onDataSourceRemoved has a valid value.

[srothst1]

@lilleyse I believe the simplest solution is to allow for the startTime and the stopTime to be equivalent in Timeline.prototype.zoomTo. This prevents errors from occurring when an empty interval is added and viewer.dataSources.remove(dataSource); or viewer.dataSources.removeAll(dataSource); is called. It also does not seem to negatively impact any other functions.

After updating Timeline.prototype.zoomTo, the empty interval
"interval":"2020-01-01T00:00:00+00:00/2020-01-01T00:00:00+00:00"

yields

which is both intuitive and user-friendly functionality. Here is a sandcastle demo.

[lilleyse]

Now that I've stepped through the timeline code a little bit I'm pretty sure the lessThanOrEquals check is there for good reason.

There's a divide by zero here when this._timeBarSecondsSpan is zero. The timeline code seems to handle the resulting NaN gracefully because it clamps the duration to a minimum duration of 0.01 seconds, but some of the generated css code still uses variables that are NaN. Maybe those NaN's are handled gracefully elsewhere.

Rather than relying on this behavior I think a better solution is to clamp this._timeBarSecondsSpan to the minimum/maximum duration right as it's set in zoomTo. Either that or fix this in Viewer trackDataSourceClock by passing in a stop time that's slightly larger than the start time.

Open to other suggestions as this isn't a system I'm super familiar with.

[srothst1]

@lilleyse From what I can tell, the timeline code and the generated CSS code handle NaN gracefully.

Of your two proposed solutions, I think the second is slightly more intuitive. I implemented it in the most recent commit. When dataSourceClock.startTime is equivalent to dataSourceClock.stopTime, I set dataSourceClock.stopTime to 5 seconds past dataSourceClock.startTime. My choice of 5 seconds is somewhat arbitrary - is there a more appropriate time window that I should use?

Curious to hear your overall thoughts!

[lilleyse]

choice of 5 seconds is somewhat arbitrary - is there a more appropriate time window that I should use?

Ultimately it is arbitrary but CesiumMath.EPSILON2 (0.01) seems like an ok choice because it matches the timelines own internal minimum and is close-ish to zero.

[srothst1]

@lilleyse I just updated Viewer.js. Specifically, I

1. used a scratch variable instead of modifying dataSourceClock.stopTime
2. Added a comment explaining the new if statement.
   Let me know how this looks.

[lilleyse]

Thanks @srothst1, I pushed a couple small commits (one was a small mistake in my example code above). Once CI passes this will be ready to merge.