👍

Let's add a type for this struct.

Something like:

interface EnvironmentMapAndSkybox {
  environmentMap: WebGLRenderTarget,
  skybox?: WebGLRenderTargetCube
}

It seems like these cache checks ought to be hoisted above the conditionals where we load so that we only do the check once, and otherwise load + populate the cache.

One pattern I like a lot is to populate caches with a promise for the value (rather than the value itself). This allows us to streamline the async flow of the method a bit. For example:

let skyboxLoads: Promise<WebGLRenderTargetCube|null> = Promise.resolve(null);
let skybox: WebGLRenderTargetCube|null = null;

// ...

if (!!skyboxUrl) {
  if (!skyboxCache.has(skyboxUrl) {
    skyboxCache.set(skyboxUrl, this.loadEquirectAsCubeMap(
        skyboxUrl,
        progressTracker ? progressTracker.beginActivity() : () => {}));
  }
  skyboxLoads = targetCache.get(skyboxUrl);
}

// ...

skybox = await skyboxLoads;

This approach also helps a lot if another element wants to load the same map but only starts trying to in the async time while it has already started loading but hasn't finished yet. The second loader will see the actively loading item in cache and await on its promise (avoiding a race condition that would result in a double load).

It's good to have addressed this edge case (loading the same URL for environment vs skybox). However, I think it would be more robust to just have two caches (one for environment, and one for skybox). It's not likely but certainly possible that :env will collide with a real URL, and we could avoid worrying about this problem entirely by maintaining a separate cache.

Sidenote: this whole simultaneous loading arrangement between skybox and envmap screams for a higher level abstraction to save us from repeating ourselves all over the place. No need to do that now, just wanted to note it.

This dualistic quality where the environment map may be PMREM'd or may not be adds an interesting complication. You would need to amend the skybox example I commented about above to do something like this when populating the cache in the environment map case:

environmentCache.set(environmentUrl, this.loadEquirectAsCubeMap(
        environmentUrl,
        progressTracker ? progressTracker.beginActivity() : () => {})
    // Chain the promise so that the cached async work includes the PMREM pass:
    .then((environmentMap: WebGLRenderTarget) => this.pmremPass(environmentMap.texture)));

That would actually end up looking a lot cleaner than what we had before this PR was proposed...