Moved Viewer functionality to CesiumWidget class #12202

jfayot · 2024-09-18T10:57:22Z

Description

This is a proposal to move Viewer functionalities not related to widgets to CesiumWidget class as proposed in #11967.

Issue number and link

Fixes #11967

Testing plan

Updated ViewerSpec.js
Updated CesiumWidgetSpec.js
Updated Cesium Widget sandcastle

I kept duplicate tests in ViewerSpec, may be some can be removed as they are implemented in CesiumWidgetSpec now?

Author checklist

I have submitted a Contributor License Agreement
I have added my name to CONTRIBUTORS.md
I have updated CHANGES.md with a short summary of my change
I have added or updated unit tests to ensure consistent code coverage
I have updated the inline documentation, and included code examples where relevant
I have performed a self-review of my code

github-actions · 2024-09-18T10:57:44Z

Thank you for the pull request, @jfayot!

✅ We can confirm we have a CLA on file for you.

ggetz · 2024-09-18T13:11:01Z

@jjspace Could you please take a pass on this PR and see if this is a direction we'd like to go API-wise? Please be mindful of any breaking changes, and deprecate or preserve functionality where we can.

jfayot · 2024-09-20T15:17:58Z

@jjspace any update on this one? 👀

jjspace

Thanks @jfayot, this is looking good. sorry for the delay, github doesn't make it very easy to review diffs for things that move between files... I had a few initial comments

I kept duplicate tests in ViewerSpec, may be some can be removed as they are implemented in CesiumWidgetSpec now?

Generally we don't write specs that test "dependencies" of an object. I would say any tests that are for viewer functions that are now just passed through to widget functions should be removed.

packages/widgets/Source/Viewer/Viewer.js

packages/engine/Source/Widget/CesiumWidget.js

packages/widgets/Source/Viewer/Viewer.js

packages/engine/Source/Widget/CesiumWidget.js

jfayot · 2024-09-25T09:09:10Z

packages/widgets/Source/Viewer/Viewer.js

@@ -1887,24 +1754,11 @@ Viewer.prototype._dataSourceRemoved = function (
 Viewer.prototype._onTick = function (clock) {
  const time = clock.currentTime;

-  const isUpdated = this._dataSourceDisplay.update(time);
-  if (this._allowDataSourcesToSuspendAnimation) {
+  const isUpdated = this._cesiumWidget.dataSourceDisplay.ready;


I don't know if you noticed this change, but I'm not quite confident with it, and I have no idea how to do it differently. What do you think?

I do remember thinking something about this seemed wrong but I forgot to loop back to it. I think it might be the case that this can be skipped because the onTick for the CesiumWidget checks the same thing and stops the same clock object if the data source display doesn't update? I also don't 100% understand what these functions are doing though so I might be wrong. Maybe we should even be initializing the clock differently in the Viewer now?

@ggetz would you be able to take a look specifically at this chunk of code/functionality?

From what I understand, this block's intent is to temporarily stop the animation if something is still buffering into the dataSource. Since ClockViewModel only pass through the Clock's canAnimate property, and the clock is already updated in CesiumWidget, I first thought that this was no more needed into the Viewer.
To be sure, I did try to duplicate the test "suspends animation by dataSources if allowed" from CesiumWidget spec to Viewer spec (replacing widget.clock by viewer.clockViewModel) and removed the mentioned code block: the test failed!
I guess this is because the ClockViewModel properties are not synchronized with Clock's properties unless ClockViewModel.synchronize() is called. Therefore, in the case where dataSource.update() would return false, you would end up with clock.canAnimate set to false, and clockViewModel.canAnimate still equal to true.
May be the clockViewModel is being synchronized later but I can't be certain!
I left the "suspends animation by dataSources if allowed" test commented in the Viewer spec if you want to play with it...

Yes, I think this is a sensible change. Just as @jfayot stated, we need to update the clockViewModel, which CesiumWidget does not have a reference to.

jjspace

Thanks for the changes @jfayot, I think this is getting pretty close. Just had a few more small comments.

I'm going to try and do a bit more testing tomorrow. For my sanity given that the Viewer and CesiumWidget are basically the "core classes" of CesiumJS and used by every project I'd feel more comfortable with a second review pass, maybe @ggetz or @lukemckinstry can take a look through?

packages/widgets/Source/Viewer/Viewer.js

packages/engine/Source/Widget/CesiumWidget.js

packages/widgets/Source/Viewer/Viewer.js

jjspace · 2024-09-26T21:30:41Z

packages/widgets/Source/Viewer/Viewer.js

@@ -1887,24 +1754,11 @@ Viewer.prototype._dataSourceRemoved = function (
 Viewer.prototype._onTick = function (clock) {
  const time = clock.currentTime;

-  const isUpdated = this._dataSourceDisplay.update(time);
-  if (this._allowDataSourcesToSuspendAnimation) {
+  const isUpdated = this._cesiumWidget.dataSourceDisplay.ready;


I do remember thinking something about this seemed wrong but I forgot to loop back to it. I think it might be the case that this can be skipped because the onTick for the CesiumWidget checks the same thing and stops the same clock object if the data source display doesn't update? I also don't 100% understand what these functions are doing though so I might be wrong. Maybe we should even be initializing the clock differently in the Viewer now?

@ggetz would you be able to take a look specifically at this chunk of code/functionality?

packages/widgets/Source/Viewer/Viewer.js

packages/engine/Source/Widget/CesiumWidget.js

jfayot · 2024-09-27T04:45:18Z

Hi @jjspace ! I'll be off for one week starting from now with no connectivity at all...
I'll catch up on oct 7th
See you soon

lukemckinstry · 2024-10-02T13:25:31Z

Started some initial high level testing, on sandcastles like this https://sandcastle.cesium.com/?src=GeoJSON%20simplestyle.html when using this branch should we be able to replace Cesium.Viewer with Cesium.CesiumWidget?

jjspace · 2024-10-02T14:42:42Z

when using this branch should we be able to replace Cesium.Viewer with Cesium.CesiumWidget

@lukemckinstry The basic answer is yes. After this PR the API for CesiumWidget and Viewer should be the same so they can be exchanged except for where code interacts with the actual widgets that are added by the Viewer like the timeline and animation controls.

Swapping it in that one you linked works fine for me, were you seeing an issue? I had tried on a handful of sandcastles already but if you see specific issues please share them so we can take a look.

lukemckinstry · 2024-10-02T14:53:30Z

Swapping it in that one you linked works fine for me, were you seeing an issue? I had tried on a handful of sandcastles already but if you see specific issues please share them so we can take a look.

I did see an issue in that sandcastle, but it is working fine now 👍, so perhaps I set it up wrong

jfayot · 2024-10-07T12:05:06Z

Hi @jjspace, I'm back 😁
I've addressed most of your comments I think, but there are still some pending points...
Tell me what should be the next step!

ggetz · 2024-10-23T13:04:36Z

@jjspace I follow up on the questions you had!

@jfayot and @jjspace A quick reminder to make sure CHANGES.md is updated to reflect any new properties or functionality available on the CesiumWidget class. Thanks!

jjspace · 2024-10-24T20:44:40Z

Awesome, thanks @ggetz!

@jfayot Can you please merge in main to make sure there are no more conflicts? Then update the CHANGES.md as Gabby mentioned. I think most of the changes can probably go under a main bullet point of something like "move Viewer functionality to CesiumWidget to increase usability".

Overall with our previous reviews I think this is really close. I'll take another look once the branch is updated!

…get's property

jfayot · 2024-10-25T10:18:22Z

@jjspace @ggetz, I've rebased my branch and updated the CHANGES.md. I also tried to improve the way canAnimate is updated in the _onTick using a callback. The test "suspends animation by dataSources if allowed" was duplicated in both Viewer and CesiumWidget in order to test it in both context. Hope this is fine for you!

jjspace

Sorry for the delay on this @jfayot. All looks good to me now, thank you for the changes!

ggetz assigned jjspace Sep 18, 2024

jjspace reviewed Sep 24, 2024

View reviewed changes

jfayot commented Sep 25, 2024

View reviewed changes

jfayot force-pushed the feat_11967 branch from 9a27f05 to 35a1067 Compare September 25, 2024 09:21

jjspace reviewed Sep 26, 2024

View reviewed changes

jfayot force-pushed the feat_11967 branch from eed3697 to d699551 Compare October 7, 2024 12:46

jfayot added 5 commits October 25, 2024 09:24

feat: moved Viewer functionality to CesiumWidget class

66512be

fix: fixed jsdoc

34212ad

fix: link allowDataSourcesToSuspendAnimation Viewer's property to Wid…

e266f41

…get's property

fix: updated following review

98617ad

fix: fixes following comments

b552364

jfayot force-pushed the feat_11967 branch from d699551 to b552364 Compare October 25, 2024 07:25

jfayot added 2 commits October 25, 2024 11:00

doc: updated CHANGES.md

e27ac2e

fix: improved canAnimate update using callback

32e9050

jjspace added 2 commits October 30, 2024 12:47

adjust changes.md

05410bf

Merge remote-tracking branch 'origin/main' into feat_11967

bb0d73d

jjspace approved these changes Oct 30, 2024

View reviewed changes

jjspace merged commit aa6a1c7 into CesiumGS:main Oct 30, 2024
4 checks passed

jjspace mentioned this pull request Nov 1, 2024

Update Viewer timeline when data sources change #12284

Merged

6 tasks

ggetz mentioned this pull request Nov 14, 2024

TypeError on setting the Viewer selectedEntity #12303

Open

jfayot mentioned this pull request Dec 4, 2024

Severe performance drop since 1.123 #12356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved Viewer functionality to CesiumWidget class #12202

Moved Viewer functionality to CesiumWidget class #12202

jfayot commented Sep 18, 2024 •

edited by jjspace

Loading

github-actions bot commented Sep 18, 2024

ggetz commented Sep 18, 2024

jfayot commented Sep 20, 2024

jjspace left a comment

jfayot Sep 25, 2024

jjspace Sep 26, 2024

jfayot Oct 7, 2024 •

edited

Loading

ggetz Oct 23, 2024

jjspace left a comment

jjspace Sep 26, 2024

jfayot commented Sep 27, 2024

lukemckinstry commented Oct 2, 2024 •

edited

Loading

jjspace commented Oct 2, 2024 •

edited

Loading

lukemckinstry commented Oct 2, 2024

jfayot commented Oct 7, 2024

ggetz commented Oct 23, 2024

jjspace commented Oct 24, 2024

jfayot commented Oct 25, 2024

jjspace left a comment

Moved Viewer functionality to CesiumWidget class #12202

Moved Viewer functionality to CesiumWidget class #12202

Conversation

jfayot commented Sep 18, 2024 • edited by jjspace Loading

Description

Issue number and link

Testing plan

Author checklist

github-actions bot commented Sep 18, 2024

ggetz commented Sep 18, 2024

jfayot commented Sep 20, 2024

jjspace left a comment

Choose a reason for hiding this comment

jfayot Sep 25, 2024

Choose a reason for hiding this comment

jjspace Sep 26, 2024

Choose a reason for hiding this comment

jfayot Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

ggetz Oct 23, 2024

Choose a reason for hiding this comment

jjspace left a comment

Choose a reason for hiding this comment

jjspace Sep 26, 2024

Choose a reason for hiding this comment

jfayot commented Sep 27, 2024

lukemckinstry commented Oct 2, 2024 • edited Loading

jjspace commented Oct 2, 2024 • edited Loading

lukemckinstry commented Oct 2, 2024

jfayot commented Oct 7, 2024

ggetz commented Oct 23, 2024

jjspace commented Oct 24, 2024

jfayot commented Oct 25, 2024

jjspace left a comment

Choose a reason for hiding this comment

jfayot commented Sep 18, 2024 •

edited by jjspace

Loading

jfayot Oct 7, 2024 •

edited

Loading

lukemckinstry commented Oct 2, 2024 •

edited

Loading

jjspace commented Oct 2, 2024 •

edited

Loading