Fix memory usage for environment map manager

[ggetz]

Description

• Addresses memory leak brought up in this community forum thread
• This is more or less a follow up from Throttle total resources used for environment maps #12320 that hopes to address some lingering memory use concerns
• This would complement, not replace, fixes issue #12356 performance drop due to EnvironmentMap #12358 as it would hope to address lower-level causes of Severe performance drop since 1.123 #12356

Changes include:

• Adjust default number of mipmap levels for the specular maps. This trades off a bit of visual quality for a MUCH smaller overall memory footprint by default.
• More aggressively destroy textures rather than peristing them for future updates.
• Fixes memory leaks.

Issue number and link

Addresses #12356

Testing plan

I found it really helpful to use this webgl memory tool for testing. Thanks to @javagl for pointing it out!

await import("https://greggman.github.io/webgl-memory/webgl-memory.js");

// Create viewer as normal

viewer.scene.debugShowFramesPerSecond = true;
Sandcastle.addToolbarButton("Check memory", function () {
  const ext = viewer.scene.context._gl.getExtension("GMAN_webgl_memory");
  if (ext) {
    // Overall memory info
    const info = ext.getMemoryInfo();
    console.log(info);
    // Every texture, it's size, a stack of where it was created and a stack of where it was last updated.
    const textures = ext.getResourcesInfo(WebGLTexture);
    console.log(textures);
  } else {
    console.log("There was an issue loading the extension");
  }
});

Texture memory results

Scenario	Before (MB)	After (MB)	% change
Procedural IBL	110.334	91.591	-16.99%
Unlit boxes – 5 cubed	2065.118	162.067	-92.15%
without reoloading, back to 1 cube	156.448	128.029	-18.17%

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]

Thank you for the pull request, @ggetz!

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

[jjspace]

@ggetz when I load the Unit box sandcastle locally and change from unlit 1 to unlit 5 back to unlit 1 I get an error that an object can't be destroyed because it already was

[jjspace]

Should this be checking isDestroyed() first? Some places in this PR you do and some you don't. One of these might be the cause of the error I mentioned above.

Also worth noting that this is exactly why I had questions about this pattern #12266

[ggetz]

Throughout this class, I've looked to set any destroyed property back to undefined to avoid the need for the check.

I think #12266 is still worth considering, but I think for the purposed of this PR and to keep the scope small I'd like to avoid addressing it here.

[jjspace]

I will also note that I do see a pretty significant reduction in memory used on my system as a whole when loading the 100 models example from the forum thread locally

[javagl]

As a response to #12320 (comment) :

In that issue, I said "I'll probably try again", so I just tried out the current main state (!), and there, I could still cause the crash with "10 cubed".

And now I tried out this branch, and there, I could not cause that crash. So the main issue seems to be fixed.

I also saw the destroy()/isDestroyed() issue that jjspace mentioned. But I assume that this is just a detail - either of checking isDestroyed at the right place, or of handling this as suggested in #12266

I did not fully think through the code changes. But it seems like to combination of the reduced number of levels (which I think go in exponentially into the memory consumption!) and the approach of "eagerly destroy()ing things" seems to solve this one.

And... since "Testing software" should usually not mean "Checking that it works" but rather "Trying to break it", here's a small outtake from that:

[ggetz]

Thanks @jjspace and @javagl! The error you were seeing should now be resolved. There was an issue with how we were handling canceled commands.

[ggetz]

@jjhembd or @lukemckinstry, could one of you take a final review pass and merge if happy? I think it'd be helpful to get this into main soon given the holidays.

[lukemckinstry]

Test cases look much improved, and I agree with the logic of the fix 👍