Deprecate ready/readyPromise for terrainProviders

[ggetz]

Part of #10909

Since we're getting rid of the readyPromise pattern, it's preferable to roll out the deprecations across the API as part of one release in order to reduce noise. I've created a staging branch to target some smaller PRs like this into.

This PR depreciates the readyPromise pattern for terrain providers. I set the deprecation period for two releases from the next one (1.104) since these are fairly high-tough areas of the API. A few other API changes consequently fall out of this deprecation, notably:

• ready and readyPromise have been deprecated, but remain working until they are completely removed.
• Various properties no longer throw an exception since there's no need to wait for ready
• Where applicable, url or the relevant resource-related options have been deprecated from constructors. In their place, there are new fromUrl functions, which are asynchronous functions that resolve to a new instance of the provider after successfully requesting the required resources. This function call throws any relevant errors rather than logging it to the console or raising an error event.
• Viewer and CesiumWidget parameters: These will now need to take promises which resolve to the terrainProvider if we'd like to keep the same setup flow
• createWorldTerrain will need to be deprecated, as it can no longer work synchronously. I went with createWorldTerrainAsync but I'm open to naming suggestions.

Some finer points of the implementation here include:

• Generally, a move towards using async/await syntax for flatter and more readable code
• Adds a new async version of the toThrowDeveloperError matcher for better async testing flow
• Making use of a builder pattern (encapsulated within the modules, not part of the public API) in order to manage a more complex object construction
• I don't think the baseLayerPicker and Cesium Viewer app changes are the cleanest they can be, though it's probably best to address that once the imagery provider version of these changes are in.
• It looks like in order for typescript to pass, we cannot remove the url option from the the constructor options typedef without throwing an error. Unless there is a workaround I don't know of, we'll need to note to remember to remove the property when the depreciation warnings are removed.

Finally, as a consequence of these changes, we'll definitely need to do a pass through website tutorials and ensure we're using the updated APIs. I've made the Sandcastle changes here already.

TODO:

• Cleanup typescript errors
• Make sure sandcastles with dropdowns have selected their first item
• scene.pickPosition seems wildly off in http://localhost:8080/Apps/Sandcastle/index.html?src=Drawing%20on%20Terrain.html&label=All
• I3S Error http://localhost:8080/Apps/Sandcastle/index.html?src=I3S%20IntegratedMesh%20Layer.html&label=All
• Put back removed unit tests

Outside of scope for this PR:

• Pass over async code for Sandcastles (and do we want to make the invoked wrapper function async). In particular, do a pass for race conditions when doing things like switching providers.
• depthTestAgainstTerrain setting via BaseLayerPicker? Deprecate ready/readyPromise for terrainProviders #11033 (comment)
• Finalize deprecation versions and open issues
• fromUrl / Resource naming
• Open forum thread to discuss API changes

[cesium-concierge]

Thanks for the pull request @ggetz!

• ✔️ Signed CLA found.
• ❔ 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.

[ggetz]

@mramato Would you be able to give this a pass?

[mramato]

@ggetz Will do. Thanks!

[mramato]

• Adds a new async version of the toThrowDeveloperError matcher for better async testing flow

I haven't looked at the code yet, but modern jasmine allows you to do stuff like:

expectAsync(promimse).toBeRejectedWithError(DeveloperError);

or even

expectAsync(promimse).toBeRejectedWithError(DeveloperError, "Exact error message");

or

expectAsync(promimse).toBeRejectedWithError(DeveloperError, /regex_message_check/);

I feel that conventions in Cesium like toThrowDeveloperError are data and custom for no longer valid reasons. This isn't the PR to start changing that, but it would be nice to update the code base to start to move towards modern jasmine.

[mramato]

• It looks like in order for typescript to pass, we cannot remove the url option from the the constructor options typedef without throwing an error. Unless there is a workaround I don't know of, we'll need to note to remember to remove the property when the depreciation warnings are removed.

Can you clarify what you mean by this. Are you talking about leaving the deprecated url property in the constructor options JSDoc so that TypeScript still includes it?

[mramato]

Just to confirm, provider would always be set before and now it isn't? Or was this a bug? I see we made a lot of the parameters optional, which technically isn't a breaking change; but since these changed weren't mentioned in the PR description or in CHANGES, I figured I'd double check.

[ggetz]

Some of these (message, x, y, level) were already treated as optional, so the doc comment was updated for accuracy.

provider and event were previously being passed, and this make it optional so that we can log errors without having already created the provider instance.

[mramato]

I don't have a better suggestion, but did we consider the fact that CesiumTerrainProvider.fromUrl is kind of an odd name since it also takes a Resource instance? I ask because it doesn't seem super intuitive to have code like:

const terrain = await CesiumTerrainProvider.fromUrl(IonResource.fromAssetId(1234));

Perhaps this should continue to work and in some future PR we can add CesiumTerrainProvider.fromIonAssetId as a helper function that would simplify the above and make the code more self documenting?

[mramato]

I probably won't review the specs too closely (I assume someone working on CesiumJS on a daily basis will also look at this) but don't we normally leave old deprecated tests in to ensure the old functionality still works? And then only remove them once we make the breaking change?

[mramato]

http://localhost:8080/Apps/Sandcastle/index.html?src=I3S%20Feature%20Picking.html seems to be broken?

[mramato]

http://localhost:8080/Apps/Sandcastle/index.html?src=Procedural%20Terrain.html is broken as well (it doesn't load until you pick something from the combo box. i.e. it's flat at the start instead of Noise being rendered.

[mramato]

@ggetz That's all I have for my pass. This looks like it will be a great change and lead to a much cleaner API. (In addition to fixing the race conditions and unit testing issues)

[ggetz]

It looks like in order for typescript to pass, we cannot remove the url option from the the constructor options typedef without throwing an error. Unless there is a workaround I don't know of, we'll need to note to remember to remove the property when the depreciation warnings are removed.
Can you clarify what you mean by this. Are you talking about leaving the deprecated url property in the constructor options JSDoc so that TypeScript still includes it?

We commonly break out the options object for constructors into there own typedef that we defined with JSDoc, ie @typedef {Object} CesiumTerrainProvider.ConstructorOptions, particularly if they are re-used, such as they are in this PR across the constructors and the new fromUrl functions. Attempting to remove the url property from the typedef causes our typescript generation step to throw errors since options.url is still being accessed in the deprecated code paths.

To be clear that this property is deprecated now, I have instead marked the property as optional and added a short note that the property is deprecated. I can also look deeper to see if there is a way to avoid the error if it jumps out as unusual.

I feel that conventions in Cesium like toThrowDeveloperError are data and custom for no longer valid reasons. This isn't the PR to start changing that, but it would be nice to update the code base to start to move towards modern jasmine.

Generally, this PR does start moving towards this convention. The main motivation for a custom toThrowDeveloper matcher now is that it becomes a noop in the release tests. Otherwise in the case of RuntimeErrors, we use expectAsync(promise).toBeRejectedWithError(RuntimeError, "Exact error message");

but don't we normally leave old deprecated tests in to ensure the old functionality still works?

My mistake; I did verify that the old tests were working before changing them to the new APIs. Easy to put them back in.

[ggetz]

@mramato

http://localhost:8080/Apps/Sandcastle/index.html?src=I3S%20Feature%20Picking.html seems to be broken?

How so? Running this locally looks exactly the same as https://sandcastle.cesium.com/?src=3D%20Tiles%20Feature%20Picking.html on my machine.

[mramato]

It wasn't loading the tileset at all for me. I'll try again and confirm.

[ggetz]

Updated to move all the asynchronous calls to the top level for the app level to await and handle errors. As a consequence, a lot of the Sandcastle code is now surrounded by an async function block. I added try/catch to the more minimal examples concentrating on terrain providers. I'd expect more Sandcastle changes to come as we finish up the rest of the ready promise deprecations throughout the codebase, and then we can do a final pass.

[kring]

I apologize in advance for this, because it's going to be pretty annoying, especially this late... but, I don't really think this is the right direction for the API...

With this change, imagery providers, terrain providers, and other things can only be constructed asynchronously (ignoring unlikely scenarios where the caller happens to have all the data needed to construct it synchronously). And there's a fromUrl factory function to do the async construction.

The problem is that this is super hard for users to deal with. Particularly users who don't understand promises or async/await very deeply, which is many (most?) of them. And even for those who do, the asynchronicity is a huge source of complexity and therefore bugs.

If the only (practical) way to construct an ImageryProvider is with a function that returns a promise, then we only have a few patterns that can be used to add that imagery provider to the globe:

Wait for the imagery provider, then add it to the globe

Something like:

const imageryProvider = await WebMapServiceImageryProvider.fromUrl(...);
globe.imageryProviders.add(imageryProvider);

Which looks innocent enough, but now this entire operation is asynchronous. The user just clicked a button to add this WMS layer, let's say. Do we make them look at an hourglass while we complete this async operation? If not, what if the user changes their mind and wants to remove it before the async operation has completed? There are ways to deal with this, but it is super fiddly and easy to get wrong.

Allow a Promise<ImageryProvider> to be added directly

Something like:

globe.imageryProviders.add(WebMapServiceImageryProvider.fromUrl(...));

But now the imageryProviders collection has a mix of real imagery providers and promises for imagery providers. In TypeScript this is awkward in the type system. Without TypeScript its a recipe for runtime errors when someone writes code without remembering that the elements can sometimes be promises.

Make creation of the entire globe async

We can wait for this promise to complete constructing the viewer. The example in the CesiumTerrainProvider docs in this branch suggests this pattern for use with a terrain provder:

const viewer = new Cesium.Viewer("cesiumContainer", {
  terrainProvider: await Cesium.CesiumTerrainProvider.fromUrl(
    Cesium.IonResource.fromAssetId(3956), {
      requestVertexNormals: true
    });

It's a bit subtle at a glance, but in this pattern the Cesium.Viewer won't be constructed until the terrain provider is constructed asynchronously. Maybe not a huge problem for a terrain provider, but for things that are meant to be layered on top, like overlay imagery layers and some 3D Tilesets, it's a step backwards versus showing the globe more quickly and then adding other data when it's available.

---

Ok, so what's the alternative? I think the right solution is the pattern we eventually landed on in cesium-native: separate the factory from the asynchronously-created provider. In cesium-native, the "imagery provider" is called RasterOverlay. Raster overlays can be constructed synchronously, but they can't provide tiles directly. Instead, someone who wants tiles (usually the rest of cesium-native itself) calls createTileProvider on the raster overlay. This method returns a promise for a RasterOverlayTileProvider.

In some sense this isn't so different from what's implemented here. It's just a factory class that can asynchronously create a tile provider, rather than a factory function that can asynchronously create a tile provider. But in another sense, it makes all the difference. It means that users can easily create new imagery providers, terrain provider, tilesets, etc. and add them to the globe immediately. And when they inspect the appropriate properties on the globe or whatever, they get the same instances back.

Errors are handled by whoever calls createTileProvider, which is usually code written by us so we can deal with the complexity of the asynchronicity. Errors can be surfaced to users as events. Events can also be used to inform users when async operations have completed.

IMO this will be much easier for our users to deal with than async factory functions.

[mramato]

Thanks, @kring. Not that me and you are deeply involved lately, but we built CesiumJS on "disagree and commit" so it's never too late to speak up. 😄 Obviously everyone wants the best approach to win.

Just to keep our eye on the prize. Ultimately the problem here is that CesiumJS has a bunch of unknowable/unawaitable asynchronicity happening under the hood in the "middle" of the stack. That's what leads to problems in #10909 and the fact that Cesium CI just randomly fails these days due to unhandled promises.

This PR (based on conversations I had with Gabby) pushes all of the async code to the top of the stack. Kevin's idea seems to be to go to the complete opposite direction and push it further into the engine and use events to bubble errors back up to the top.

I think both concepts are equally valid and can both be used to achieve the intended goal, making sure all async operations are "knowable" and "awaitable". However I am biased towards the approach this PR originally took.

The problem is that this is super hard for users to deal with. Particularly users who don't understand promises or async/await very deeply, which is many (most?) of them. And even for those who do, the asynchronicity is a huge source of complexity and therefore bugs.

I would want to see some data to back up this assertion. This may have been true 10 years ago, but promises/async/await are now at the core of JS development. Everyone uses them, everyone knows enough about them for them not to be some weird new concept like when we introduced them into CesiumJS' code base. They are bog standard JS.

The user just clicked a button to add this WMS layer, let's say. Do we make them look at an hourglass while we complete this async operation?

Browser apps are async by nature since a lot of operations are reaching back to the server. As an extreme example, every single letter you type into Google Docs makes a network request. Almost all JS apps and frameworks have opinionated ways for dealing with this. Old school hourglasses like you suggest, optimistic + rollback, or some other form of async error handling. Ultimately my point is that async in a web app is not the big lift you seem to be painting it as.

Errors are handled by whoever calls createTileProvider

What is multiple people need to tile provider and it's not clear who will get to it first? Do people create multiple providers rather than sharing a single one? What about when a user is the first one that needs the tile provider? It sounds like now you're pushing the async handling on them anyway.

Errors can be surfaced to users as events.

To me, this seems like it would be more confusing to most users than just using async/await. Ultimately what your suggesting sounds very C++/thick client approach to me. Which works great in those environments but would be less expected in JavaScript.

Also, Kevin's approach sounds like a bigger refactor to me? (though maybe it's not).

In my experience taking other JS apps with this problem and pushing things up to the top of the stack always ended up being a big improvement. The more potential async code you can remove each frame, the better.

Ultimately, it's up to @ggetz and the CesiumJS team to make this decision, as long as apps built on Cesium ensure all async operations are knowable/awaitable, it will be an improvement over what we have now and I'll be happy.

[ggetz]

I'll start with the thought that I'm clearly biased towards the solution which is already in motion... 😆 I'm generally in favor of moving the up the stack to let the user of the library decide what is right for their app.

The problem is that this is super hard for users to deal with. Particularly users who don't understand promises or async/await very deeply, which is many (most?) of them. And even for those who do, the asynchronicity is a huge source of complexity and therefore bugs.

If we do proceed with exposing the async functionality, I do think the onus is on us to make sure we are properly instructing devs how to use the async functionality. So updating our examples to make it clear how to construct any providers, properly handling errors, etc.

From a maintainability standpoint, I don't think we've been great about handling all the errors that may pop up throughout the stack, which has lead to bugs, especially intermittent ones. As #10909 points out, this new structure makes thing much more testable, especially since asynchronous code has become a part of testing libraries like jasmine.

The user just clicked a button to add this WMS layer, let's say. Do we make them look at an hourglass while we complete this async operation? If not, what if the user changes their mind and wants to remove it before the async operation has completed? There are ways to deal with this, but it is super fiddly and easy to get wrong.

I do think handling asynchronous state management is a common paradigm in web development. For example in some of our other apps we've been working on at Cesium, we've been working with Redux where there are helper functions which abstract the recommend approach for handling async requests.

separate the factory from the asynchronously-created provider

It may be worth pointing out (and I may be totally off base with this since I'm not very involved with the C++ world), I do think a C++ and JS implementation will look different when it comes to handling asynchronous code. Promises and async code are a standard part of modern JS, and while std::future exists, I don't think its used as ubiquitously as promises in JS.

If I correctly understand the RasterOverlay and RasterOverlayTileProvider pattern, we would basically want an additional class that lives on top of the existing provider that handles errors and dispatches events, correct? I don't think that precludes the approach here, but it would require that we add that top level RasterOverlay abstraction before shipping this update.

Ultimately, I'm leaning on the side of staying leaner and foregoing that RasterOverlay abstraction, as it will give the user better flexibility in their own apps, and will give us less API area to maintain.

Perhaps in the future, as declarative paradigms are becoming more common in the JS world, we may re-introduce that level of abstraction in the to provide a better interface for event-driven apps, but I'm not sure that's the right choice for the library as it is today.

[jjhembd]

@kring, I agree that the last pattern ("Make creation of the entire globe async") is often not a good one, for the reasons you mentioned:

for things that are meant to be layered on top, like overlay imagery layers and some 3D Tilesets, it's a step backwards versus showing the globe more quickly and then adding other data when it's available.

However, this can still be addressed with the following pattern, lifted from the "Terrain Performance" Sandcastle: (it's another version of your first pattern, "Wait... then add it to the globe".)

        const viewer = new Cesium.Viewer("cesiumContainer", {});

        (async () => {
          try {
            viewer.terrainProvider = await Cesium.createWorldTerrainAsync();
          } catch (error) {
            console.log(error);
          }
        })();

        // ... do more stuff with viewer, while the terrainProvider is still loading ...

Code that depends on terrainProvider actually being loaded, now has to go inside the async function. Previously, it had to go inside terrainProvider.readyPromise.then(...).

From the app developer's point of view, I think either approach requires a similar level of async understanding to code correctly. But the new pattern better handles coding mistakes. The old approach depended on things like this internally:

  if (!this._ready) {
    throw new DeveloperError(
      "requestTileGeometry must not be called before the terrain provider is ready."
    );
  }

and on the hope that we had these errors in all the right places, and they were documented, and the user read all the documentation...

[jjhembd]

@ggetz, sorry if I missed something, but are we planning to clean up more of the .readyPromise usage in the specs in this PR? I understand we still want to test the deprecated methods, but there are unrelated tests like this one in CesiumTerrainProviderSpec

  it("returns reasonable geometric error for various levels", function () {
    returnQuantizedMeshTileJson();

    const provider = new CesiumTerrainProvider({
      url: "made/up/url",
    });

    return provider.readyPromise.then(function () {
      expect(provider.getLevelMaximumGeometricError(0)).toBeGreaterThan(0.0);

Also, the calls to pollToPromise are using the deprecated .ready property

[mramato]

(async () => {

I'll also point out that in modern JS, top-level await means no weird wrapping of async stuff.

[jjhembd]

no weird wrapping of async stuff.

It does look weird, and made me question the new API at first. But on the other hand, without wrapping it, what if the terrain provider hangs? How could we make sure the rest of the app is still usable? The wrapping lets the rest of the function execute in the meantime.

[ggetz]

I understand we still want to test the deprecated methods, but there are unrelated tests like this one in CesiumTerrainProviderSpec

I swung the opposite way to prove that all the correct properties were still being set on created such that these tests continued to work. But based the coding guide (emphasis mine) they should get switched over.

Remove all use of the deprecated API inside Cesium except for unit tests that specifically test the deprecated API

The test will certainly be briefer with the new API, so I'm not against switching it. I'll update.

[kring]

I would want to see some data to back up this assertion. This may have been true 10 years ago, but promises/async/await are now at the core of JS development. Everyone uses them, everyone knows enough about them for them not to be some weird new concept like when we introduced them into CesiumJS' code base. They are bog standard JS.

I used to interview JS developers with a question based around promises. Use async/await if you want, totally open book. I'd even answer any questions you had about promises, their API, how they work, whatever. It was very very rare that someone did well on it. Even people that had experience using async/await and promises seemed to have a very shaky understanding of what they actually did. I'm sure this situation is improving, but the fact remains that this stuff is hard and most of the time you don't need to understand it very deeply to be a competent JS developer.

What is multiple people need to tile provider and it's not clear who will get to it first? Do people create multiple providers rather than sharing a single one? What about when a user is the first one that needs the tile provider? It sounds like now you're pushing the async handling on them anyway.

These questions seem really academic. In virtually all Cesium applications, the only client of tile providers is Cesium itself. Users create them to tell Cesium what to show on the globe, but never "use" them otherwise.

If a user does want a tile provider themselves for some reason, then yes, they need to deal with the asynchronicity. That's a far more advanced scenario than "I want to show my WMS layer on this map."

From a maintainability standpoint, I don't think we've been great about handling all the errors that may pop up throughout the stack, which has lead to bugs, especially intermittent ones. As #10909 points out, this new structure makes thing much more testable, especially since asynchronous code has become a part of testing libraries like jasmine.

I can see your point about it making Cesium easier to test, but I think it's doing that at the cost of being harder to use.

And it doesn't do anything to guarantee errors are handled. Before, there was a readyPromise available as a property, and we forgot to use it. Now the promise is returned by value, but if we don't handle possible promise rejection, we're no better off. In my proposal, only Cesium has to handle the promise rejection in almost all scenarios. Users don't need to worry about it.

It may be worth pointing out (and I may be totally off base with this since I'm not very involved with the C++ world), I do think a C++ and JS implementation will look different when it comes to handling asynchronous code. Promises and async code are a standard part of modern JS, and while std::future exists, I don't think its used as ubiquitously as promises in JS.

This general idea, "C++ is different", was mentioned a few times. But cesium-native's async system is very much modeled on javascript promises. I don't think this is a valid way to dismiss the proposal.

If I correctly understand the RasterOverlay and RasterOverlayTileProvider pattern, we would basically want an additional class that lives on top of the existing provider that handles errors and dispatches events, correct?

No, the RasterOverlay doesn't handle errors and dispatch events. The client does, meaning the ImageryLayerCollection or globe or similar. The RasterOverlay is a configurable factory for a tile provider. Pretty similar to the TerrainProviderBuilder actually.

I think @jjhembd's example illustrates my overall point really well, so let's walk through it... He proposed:

const viewer = new Cesium.Viewer("cesiumContainer", {});

(async () => {
  try {
    viewer.terrainProvider = await Cesium.createWorldTerrainAsync();
  } catch (error) {
    console.log(error);
  }
})();

// ... do more stuff with viewer, while the terrainProvider is still loading ...

Let's shift this to a real-world example. I have an application that's showing a globe (terrain) and a base layer. And my app allows users to enable a WMS overlay on top of that. So my viewer already exists, and I'm writing a function to add the WMS layer, following Jeshurun's pattern and meant to be called in a click handler or whatever. It looks like this:

function addFancyOverlayToTheGlobe(viewer) {
  (async () => {
    try {
      viewer.imagery.add(await WebMapServiceImageryProvider.fromUrl(...));
    } catch (error) {
      console.log(error);
    }
  })();
}

Of course I could make this an async function itself and return a promise, but then the client needs to be willing to deal with it. If this is a simple click handler or similar, the buck needs to stop here.

Now when the user clicks this button, we start the process of adding the WMS overlay, and sometime later it actually happens. The application needs to keep track of this "something is being added to the globe" state in order to show the user that something is happening, because Cesium doesn't know about it; it's hidden in a continuation. If you ask Cesium which WMS layers are on the globe, this one won't show up yet.

Now what if this isn't an "add button", but a checkbox. The user can toggle the overlay on and off. Super common thing, right? We'd need some way of stopping this async operation later. Maybe something like:

function addFancyOverlayToTheGlobe(viewer) {
  (async () => {
    try {
      const provider = await WebMapServiceImageryProvider.fromUrl(...);
      if (overlayIsStillShown()) {
        viewer.imagery.add(overlay);
      }
    } catch (error) {
      console.log(error);
    }
  })();
}

Hmm ok, now what if the user toggles it on, off, then on again before the fromUrl completes?

What if the user does something that causes the viewer to be destroyed before the fromUrl completes?

These are all tractable problems, but they're the sort of thing developers are extremely likely not to think about. And this is a typical, bog standard, simplest sort of thing you can go with CesiumJS scenario here!

The fundamental problem is that users don't (and shouldn't) view imagery provider creation as an async operation that will complete later. They view it as a description to Cesium of what should be shown on their globe. Add an imagery provider to the globe, and it should appear (eventually). Remove the imagery provider, and it should disappear.

And this is what the RasterOverlay system achieves. But let's stick to CesiumJS's terminology to try to be less confusing. Here's what I'm proposing users should be able to do:

// note: Imagery, not ImageryProvider
// This is lightweight and doesn't do any network requests
const fancyOverlay = new WebMapServiceImagery(...);

function addFancyOverlayToTheGlobe(viewer) {
  viewer.imagery.add(fancyOverlay);
}

function removeFancyOverlayFromTheGlobe(viewer) {
  viewer.imagery.remove(fancyOverlay);
}

All of the async creation of a tile provider, as well as the async requesting of tiles, is handled by the Viewer / Globe / ImageryLayerCollection.

If users need to be notified of imagery provider errors, beyond just showing the error to the user (which should be the default), they can subscribe to an event. A simple API can also be used to query the status of the imagery: is it still doing the initial load? are tile requests in progress? did something go wrong?

Writing tests is just as easy as it is in this PR. Call createTileProvider directly (if you need it), and handle the returned promise's resolution or rejection. But the experience for a typical user of CesiumJS is greatly improved.

[ggetz]

The application needs to keep track of this "something is being added to the globe" state in order to show the user that something is happening, because Cesium doesn't know about it; it's hidden in a continuation.

I guess the fundamental question here is whether CesiumJS should be what's tracking the state of your application. IMO no– CesiumJS already is a huge library that does a lot, plus there are plenty of other solutions that developers may want to use here instead.

If users need to be notified of imagery provider errors, beyond just showing the error to the user (which should be the default), they can subscribe to an event. A simple API can also be used to query the status of the imagery: is it still doing the initial load? are tile requests in progress? did something go wrong?

The async functionality that is brought up the stack here is the initial metadata load, not subsequent tile requests. Those continue to use an event and are logged to the console by default. By moving the initial metadata load to the forefront, the developer does need to deal will whether or not the layer was created, which is something that should likely be communicated to the app user directly, not hidden in the console. Or, as you said, they can ignore any errors and they will be in a similar situation as they are now with the readyPromises.

I'm worried about just how much code the proposed alternative would add to keep this pattern consistant for the areas we use ready promises now: Terrain providers, imagery providers, 3D Tiles, models, voxels, etc...

AFAIK, I'm not aware of another 3D JS library that implements a similar pattern where the library attempt to handle the application state. Babylon implements an async factory function for asset loading like we have here. Same with PixiJS. ThreeJS has a callback pattern for their asset loading.

[kring]

I guess the fundamental question here is whether CesiumJS should be what's tracking the state of your application.

I disagree. The fundamental question here is whether this change makes CesiumJS harder to use.

IMO no– CesiumJS already is a huge library that does a lot, plus there are plenty of other solutions that developers may want to use here instead.

I hope I haven't given the impression that I think CesiumJS should be responsible for all of an application's state. That seems a bit of a strawman.

But CesiumJS is certainly not some immediate-mode application where the developer is responsible for managing all loading and then dispatching things to be rendered each frame. It's extremely stateful. Comparing terrain providers, imagery providers, 3D Tilesets, and even geometries to simple "load once" async resources in other engines is not really fair.

But I feel like I'm shouting into the wind here, so how about this... If you want hard evidence that these changes make CesiumJS too hard to use correctly, you need look no further than the Sandcastle code in this branch, where we can easily find an example of it being used incorrectly in exactly the way I'm worried about!

Let's look at the Terrain Sandcastle example. First I'd like to point out that the viewer construction code went from this:

const worldTerrain = Cesium.createWorldTerrain({
  requestWaterMask: true,
  requestVertexNormals: true,
});
const viewer = new Cesium.Viewer("cesiumContainer", {
  terrainProvider: worldTerrain,
});

To this:

const viewer = new Cesium.Viewer("cesiumContainer");
(async () => {
  try {
    viewer.terrainProvider = await Cesium.createWorldTerrainAsync({
      requestWaterMask: true,
      requestVertexNormals: true,
    });
  } catch (error) {
    console.log(error);
  }
})();

Which seems like a huge step back in usability right there. And it's not like it's any safer or handling errors better, either. But let's move on from that and talk about the actual bug.

This Sandcastle example has a dropdown that allows the user to select a terrain provider. The onselect handler for each constructs the appropriate terrain provider, awaits it, and then sets it as the viewer's terrainProvider property. Seems simple enough, right?

Except it has a serious race condition. If I select the VRTheWorldTerrainProvider (which is a slower one from here in Australia), and then very quickly switch to ArcGISTerrainProvider (just because it's the next one up so it's easy to switch to it quickly), the UI will indicate I'm looking at ArcGISTerrainProvider while I'm actually looking at VR The World. I was able to make this happen quite easily within a couple of tries (I confirmed it with the Network tab).

So this is a rather subtle - but potentially very serious! - bug. It didn't happen before this PR. In fact, it would have been quite hard to do it accidentally. Now, with this PR, it's not only extremely easy to do this accidentally (even the world experts in CesiumJS development made this mistake!), but it's also quite difficult to fix. At the very least, the Sandcastle example would get significantly more complicated if we were to fix this.

And this isn't some quirk of Sandcastle. Virtually any CesiumJS app - especially the simpler ones - are likely to run into problems like this.

Now, you don't have to like my solution, but can we at least agree there's a problem?

[mramato]

Which seems like a huge step back in usability right there.

This is a symptom of Sandcastle (which was mentioned up thread as something we should fix) and not a problem elsewhere. Top level await means that the "good" example you posted works in the new API as well, simply by adding await to the call:

const worldTerrain = await Cesium.createWorldTerrainAsync({
  requestWaterMask: true,
  requestVertexNormals: true,
});
const viewer = new Cesium.Viewer("cesiumContainer", {
  terrainProvider: worldTerrain,
});

Also, even without top-level and without this change, it's fairly typical for an application to reach out to a server during initialization, so I don't see how this is much different. Most JS apps already have code dealing with asynchronousness at this level.

I was able to make this happen quite easily within a couple of tries (I confirmed it with the Network tab).

Yes, this is definitely a bug that should be fixed in our widgets. Thanks for bringing it up. This is a branch PR going into the staging branch so there is certainly a list of remaining TODOs that needs to be generated before it goes into main. But the components themselves already have to manage state (like which provider is selected) so I'm not sure why dealing with the async nature suddenly changes everything? As @ggetz mentioned, every modern toolkit out there has async state handling. CesiumJS widgets are definitely not "modern" but I don't see this as a major problem that has technical barriers.

[kring]

I'm not sure what you're talking about, Matt. This isn't a problem with Sandcastle or "our widgets". Sandcastle.addToolbarMenu is a thin wrapper over a standard HTML select element. It's meant to be a convenience, not an abstraction. Yes, I don't doubt you can framework your way out of this bug, but I'm not really sure how you plan to fix it without introducing some abstractions that would be wildly inappropriate in the context of Sandcastle.

But I'm interested to see what you have in mind; please prove me wrong.

I'm writing too much and I'm not sure it's really being read well (I get it, everyone is super busy, Kevin is super noisy), but a couple of really brief things:

1. We need to run this change by the community. Probably not with just terrain providers, because no one cares. But once we have imagery providers following this pattern, we definitely need to let the community try it out before we ship it. This may very well be the biggest breaking change in CesiumJS's history; the community deserves to be involved.

2. I'll try to find some time to prototype the alternate architecture I'm blabbering about, just so you all can see it concretely. IMO it has all the advantages of what's in this PR while not being a breaking change for users (other than users writing their own terrain and imagery providers and that sort of thing), and is in general much easier to use. And it's really not a lot of extra code; very similar to the Builder thing already going on in this PR, actually.

[ggetz]

I agree that we should give the community at heads up about this planned change and listen to potential feedback. We'll open a forum thread shortly. This isn't going out with the February 1st release, though I wouldn't like to delay it much longer.

In the meantime, I think we should proceed with getting this PR into a staging branch. From there, we can make further changes such as the imagery providers and have something more concrete to share with the community. @kring We'll still welcome feedback and comments such as your prototype architecture if you end up having the time.

As I mentioned further up in this PR, I don't expect this PR to be the final iteration through Sandcastle code or our own widgets. I think the comment about onSelect and a few other scenarios are valid, but can be handled and discussed further in a later PR. Also there no need to use a framework to solve these problems. My point was more about how entrenched the async pattern is is the JS ecosystem.

[jjhembd]

Thanks @kring for pointing out the issue with the Terrain Sandcastle.

Loading a new Terrain is inherently async, but the current Cesium behavior stubs in a non-functional provider while a new one is loading. On my machine, with the network throttled, the stub makes the globe disappear for a few seconds. I agree this might be less misleading than continuing to show the previous terrain in the meantime. (A non-blocking "Loading" message might be even better, since the user would still be able to use the rest of the app.)

We already have a TODO item to fix these kinds of issues in Sandcastle. And as you mentioned, imagery providers will provide a better use-case to discuss the new API with the community.

I'll merge this in for now, but please continue the discussion as we work on the imagery providers.

[shunter]

As a casual reader of the discussion here, I think the distinction in my mind is that configuration should be synchronous where possible, even when the actual execution is asynchronous.

Top-level await doesn't really improve the situation because you've just made your configuration be blocked by execution, which leads to the performance consequences Kevin alludes to.

"Configuration" in this context refers to the information needed to create a Viewer (and more broadly set up the rest of the application). In this case the information is "I want to use World Terrain (from a given location or whatever)". You don't need any actual data to be loaded from the server to communicate that, generally speaking. So yeah, as Kevin says above, that essentially boils down to a factory. Maybe a bit more if it's going to keep track of created instances, or report information about their state (e.g. failure state).

I admit I don't really have a stake here these days, but at least I can say you're not "shouting into the wind", Kevin.

[kring]

Well, said, Scott. That's a good way to frame it. Have I mentioned lately that I miss working with you? :)

[kring]

Loading a new Terrain is inherently async, but the current Cesium behavior stubs in a non-functional provider while a new one is loading. On my machine, with the network throttled, the stub makes the globe disappear for a few seconds. I agree this might be less misleading than continuing to show the previous terrain in the meantime.

This is a really interesting point, it definitely gave me pause. But after thinking about it a bit, I want to note two things about it:

1. This effect, "the globe disappears when we switch terrain providers", actually happens after the changes in this PR, too. It just might disappear for a briefer period of time. Because even if the terrain provider itself is ready to go, the globe won't appear until the root tiles are downloaded, and the old globe disappears before that happens in either case.
2. An advanced user can reduce the "no globe" time in the old readyPromise pattern too, making it as short as it is in this PR, if they want to. Something like this (not tested):

const newTerrainProvider = new CesiumTerrainProvider(...);
newTerrainProvider.readyPromise.then(() => {
   viewer.terrainProvider = newTerrainProvider;
});

This comes with all the same dangers as in the forced-async pattern in this PR, of course, but at least in this case it's an advanced user opting in.

We already have a TODO item to fix these kinds of issues in Sandcastle. And as you mentioned, imagery providers will provide a better use-case to discuss the new API with the community.

Ok that's fine but I don't think this is "oh yeah we just need to go back and fix Sandcastle, no big deal." This is exposing a major problem with this pattern, because there is no reasonable solution. Maybe I'm wrong about that, but I think it'd be wise to prove me wrong before spending a lot more time implementing this pattern, because if I'm right we're going to be throwing away a lot of code.

[ggetz]

but I think it'd be wise to prove me wrong before spending a lot more time implementing this pattern, because if I'm right we're going to be throwing away a lot of code.

I think it's a fair point to discuss how developers may use the API overall and what additional steps we might consider taking to simplify it, especially at configuration time as @shunter calls out.

Though if we do choose to make adjustments there, I don't think the will mean throwing away the majority of the code in this branch. readyPromise is still an anti-pattern and the team wants to get rid of it throughout the API.

• This pattern allows us to more easily update tests such that we can get rid of some asynchronous failures as mentioned in Race condition in Cesium3DTileset / Reconsider readyPromise pattern #10909.
• Even assuming we fix all the underlying asynchronous issues in the middle of the stack and the tests, the readyPromise pattern sets a bad precedent as we've seen it become abused in other parts of the code over time.
• Even if we do simplify the most common path, why wouldn't we be able to continue to expose this new async API for users who specifically want to opt into a different control flow? There's a use case for this as mentioned in your above comment.

[ggetz]

@jjhembd @kring

I was thinking through what we could do to simply terrain (and eventually something similar for imagery) provider creation for users who do not want to dive into the async flow, while still preserving the .fromXYZ factory functions for those users who do. While the approach is different then what @kring was suggesting with the RasterOverlay system, here's the start of what I was thinking. Terrain is a new object that handles the async error handling and exposes an error event and a ready event. One plus here is it's more compatible with event-driven UIs then readyPromise was.

This is then handled by Viewer (and eventually the base layer picker and perhaps CesiumWidget). This also restores the behavior that exists in main such that the existing terrain immediately disappears when setting the new Terrain object. (This does make the "placeholder" terrainProvider more explicit by setting it to undefined, but I think this make it clearer what's happening under the hood. @jjhemb mentioned this was a point of confusion for him regarding EllipsoidTerrainProvider vs undefined values for the terrain provider.) This also handles "competing" values, like when setting a terrain provider and then immediately setting another.

One downside of this approach is that (without TypeScript) it's not possible to confirm that a promise resolves to the expected type during object creation. However, there are many other areas of the code where this is the case, and hopefully won't be a concern if/when we move to TypeScript. I believe this wouldn't be an issue with the RasterOverlay approach, but I think the approach here is a bit less drastic of a change to the API overall, which was the intention.

[kring]

That's interesting @ggetz, but it's still a big step back from both the current implementation and my proposed factory (raster overlay) implementation. Specifically:

1. After calling setTerrain, there's no outward appearance on Globe/CesiumWidget/Viewer that anything is happening, and
2. If a user calls setTerrain, and then sets the terrainProvider explicitly, the latter action will eventually get clobbered by the former one. It's a less likely race condition, but still a fairly annoying one.

Also lots of Cesium apps can and should avoid using Viewer, so it's inappropriate to put this logic there IMO. But that's probably easy to fix.

I really don't think the factory is a big lift, especially when we're already creating factories (Builder). There's no reason to rush to weaker designs just to avoid it. I guess I just need to prototype it to demonstrate that.

[ggetz]

After calling setTerrain, there's no outward appearance on Globe/CesiumWidget/Viewer that anything is happening

There should be. That example implements the same "the globe disappears when we switch terrain providers" visual behavior as the existing API. Is there another visual indicator you were looking for that the terrain provider is changing?

If a user calls setTerrain, and then sets the terrainProvider explicitly, the latter action will eventually get clobbered by the former one. It's a less likely race condition, but still a fairly annoying one.

Good point, but this seems like a fairly straightforward fix– Remove the ready event listener if terrainProvider is set directly after setTerrain is called.

Also lots of Cesium apps can and should avoid using Viewer, so it's inappropriate to put this logic there IMO. But that's probably easy to fix.

I agree that most apps should not be using viewer. But as I stated in the comment this does not need to be exclusive to the viewer. We would likely add this functionality to CesiumWidget as well, and in doing so it probably makes sense to move the main logic there. I was hoping this abstraction could live entirely on the Widget layer, but it could be argued this may instead belong in Scene. But as you mention, that's an easy fix.

But I don't see why we shouldn't expose the functionality in Viewer at all though though, especially since it already exposes setting the terrainProvider property.