Fix post-processing crash when disabling and re-enabling stages

[j9liu]

Fixes #9204, also fixes #9528.

The crashes were happening because the texture cache wouldn't create framebuffers if no stages were marked as active, despite the fact that they are enabled. It will now create framebuffers if at least one stage is enabled.

I ran the unit tests and looked through all of the post-processing Sandcastles, and I don't believe this change introduced any other bugs.

cc: @lilleyse @ebogo1

[cesium-concierge]

Thanks for the pull request @j9liu!

• ✔️ 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.
• Works (or fails gracefully) in IE11.

[lilleyse]

I can confirm the fix works but it might be worth looking into the alternate approach I mentioned in #9204 (comment). I was curious what would happen so I hacked together a quick version of it and was surprised to see that it fixed the original issue and a totally separate issue #8345 with the full screen flash. I didn't really dive deep into why it fixed that but maybe it has something to do with the framebuffer and active stages being slightly out of sync with each other, like a one frame delay?

[lilleyse]

Also, please add a test to PostProcessStageCollectionSpec.js

[j9liu]

I can confirm the fix works but it might be worth looking into the alternate approach I mentioned in #9204 (comment). I was curious what would happen so I hacked together a quick version of it and was surprised to see that it fixed the original issue and a totally separate issue #8345 with the full screen flash. I didn't really dive deep into why it fixed that but maybe it has something to do with the framebuffer and active stages being slightly out of sync with each other, like a one frame delay?

I tried to implement that fix, but the extra update call actually causes some of the unit tests to fail. I'll start looking into the full screen flash issue to find a specific fix for that.

EDIT: I'll attach the failed tests for reference. Surprisingly, the same crash is happening in one of them, even though the extra update is supposed to take care of it?

[j9liu]

maybe it has something to do with the framebuffer and active stages being slightly out of sync with each other, like a one frame delay?

It turned out to be something like that: PostProcessStageCollection.prototype.execute only executes active stages, so it skipped over the ones that were enabled but not active. The yellow screen flash was happening in the Sandcastle because it would execute the Silhouette stage, but not the next enabled one, so the pure yellow was the result of an incomplete pass. I added a call to check if the stages could be marked active before it tried to execute the pipeline, this should fix #8345.

[lilleyse]

I tried to implement that fix, but the extra update call actually causes some of the unit tests to fail. I'll start looking into the full screen flash issue to find a specific fix for that.

Hm I'm not seeing those errors so maybe there was a difference in how we implemented it. Here's the code I added to the end of PostProcessStageCollection.prototype.update:

  count = 0;
  for (i = 0; i < length; ++i) {
    stage = stages[i];
    if (stage.ready && stage.enabled && stage._isSupported(context)) {
      count++;
    }
  }

  activeStagesChanged = count !== activeStages.length;
  if (activeStagesChanged) {
    this.update(context, useLogDepth, useHdr);
  }

Ultimately we arrived at similar fixes because both fixes call update a second time to update the active stages, just in different locations. See if unit tests pass for you with that code, I prefer it mainly because it's less changes overall and more self contained.

[j9liu]

@lilleyse - that fix worked. I stripped my changes and added your fix instead. I also got rid of the lastLength variable (the one I renamed lastActiveLength) because it was being used for the same purpose as activeStagesChanged.

Do you want me to do anything about the tonemapping issue I mentioned over Slack?

While writing the tests for PostProcessStageCollection I realized that the tonemapping aspect of it never resets after each test, so when I put some new tests after the tonemapping tests, they were still being incorrectly tonemapped. But it doesn't look like there's an option to disable tonemapping in general -- can I add one?
3:37
I'm not sure the intended functionality -- it looks like PostProcessStageCollection defaults to this ToneMapper.ACES setting, not sure if that's supposed to be "normal" coloring because even when I try to use this setting the test is still being tonemapped. Is it okay for there to be a lack of a tonemapper or is that wrong?

[lilleyse]

Tonemapping is only applied when scene.highDynamicRange = true so as long as the tonemapping tests set this back to false the other tests should run fine.

I think the problem is that those tests are not removing the primitive they add to the scene so it interferes with subsequent tests. If you add scene.primitives.removeAll(); in afterEach it should fix any problems in the future.

[j9liu]

@lilleyse - done! I temporarily moved my tests after the tonemapping ones just to be sure the scene was resetting and they passed.

[lilleyse]

Thanks @j9liu!