Improve documentation and readability in Scene and depth-related code

[jjhembd]

Description

This PR is a first step towards improved maintainability in some of the core rendering code, especially code that touches depth buffers. The focus is on classes like Scene, View, GlobeDepth, Framebuffer, and DerivedCommand, with minor documentation updates in connected classes.

Main changes include:

• Added documentation for private functions
• Simplified function signatures (to be more documentable)
• Broke executeCommands in Scene into smaller functions, borrowing ideas (and some code!) from Break executeCommands into functions #12058
• Cleaned code to better follow the Coding Guide, especially this point:

Declare variables where they are first used.

Issue number and link

Testing plan

• Run all specs locally
• Spot check Sandcastles

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, @jjhembd!

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

[ggetz]

@javagl Would love to get your input on this.

Also a general comment that it would be helpful to prioritize getting this PR merged quickly to avoid merge conflicts as it touches core parts of the codebase.

[javagl]

First, a big 👍: I'm strongly convinced that there should be (and have been) more cleanup passes like this. Otherwise, the effect is commonly referred to as https://en.wikipedia.org/wiki/Lava_flow_(programming) . People are afraid of touching code (that it hard to read and fully understand), out of the fear of breaking things. And whatever they are going to do instead: It can not improve the situation. It can only make it worse. (Now iterate that...)

In terms of the review, a general disclaimer: When ~"code that needs to be cleaned up" eventually is cleaned up, it can be really hard to look at the change alone and be 100% sure that it does not introduce changes in the behavior (including bugs). But multiple reviews and tests should make "serious" bugs less likely.

---

I did look over the changes via the commits, the "Files changed" as a whole, and for some of them, at the new state only (locally in VSCode).

• Some commits seem to only address the "Declare variables where they are first used." guideline. That's good. These nearly-global variables also bugged me. The let always raises the questions: Will it change? And where? And how? We might even consider allocating some time/cycles to just do this, mechanically, as part of maintenance (orchestrated properly, to avoid merge conflicts)
• Other commits did further structural cleanups. Some of them may be subjective.
  • I'm usually not using the const { destructuring, assignment } = syntax;, but ... it's concise and clean, so hard to argue against.
  • I also prefer "early returns" when they are used as "watchdogs" at the beginning of a function, and decrease the indentation level, and have seen that this was done in a few places
  • Pulling out repetitive.parts.of.property.accesses seems uncontoversial
  • Similarly, pulling out common blocks into functions that have names and documentation
• I've been staring at ad274f8 for quite a while. And I hesitate to ask 😆...

There are some minor inlined comments, but nothing critical, I guess.

---

I also compiled and started that state locally, and tried out some of the Sandcastles. (I did not try all of them, and all possible interactions, but ... focussed on some of the "rendering-heavy" ones with sophisticated features, like Depth-Of-Field and whatnot). I did not notice any issues.

---

(I also ran the tests locally, and some of them did fail, but I'm currently having some network connection issues, so that doesn't mean anything for this PR...)

---

Unrelated to this PR, but maybe as sorts of follow-ups:

• The Coding Guide does not say anyhing about "destructuring assignment". Is this preferred or discouraged, or does it not matter?
• One of the more obscure abominations of what is considered to be "JavaScript code" is that for (const key in obj) { if (!obj.hasOwnProperty(key)) { ... } }. Should the Coding Guide recommend for (const key of Object.keys(obj)) instead? (I'd say: YES!)
• Some commits removed unused parameters. Maybe this should be checked via linting? (VSCode points it out, at least...)

[jjhembd]

@javagl I think I addressed all your feedback. I also ran the end-to-end tests, comparing all the Sandcastle screenshots against the current main branch. No problems were detected.

[jjhembd]

@javagl for your more general comments,

• The test failure fixed by ad274f8 stumped me too! But that commit is reverting to the code as it was before this PR.
• Destructuring assignment is not mentioned in the Coding Guide: good point! I opened Update Coding Guide with notes on destructuring assignment #12196 to discuss further.
• The pattern for (const key in obj) { if (!obj.hasOwnProperty(key)) { ... } } was discussed here. The hesitation to change it arises from the knowledge that for...of is known to be slower than for (let i = 0; i < length; i++). I haven't yet seen a benchmark comparing for...of vs. for...in, especially when that Object.hasOwnProperty() call is factored in. Perhaps we need to generate these benchmarks ourselves. Or perhaps the current pattern is an artifact of some prehistoric premature optimization that we should abandon. But I think that is out of scope for this PR.
• Unused parameters could be checked via eslint, but I think it might require too many exceptions. For example, we force all the *PipelineStages to have the same signature for their process method, so we can loop over them without needing to know what kind of stages are in the pipeline at every given moment. But this necessarily means that some pipeline stages will not use all the arguments--see AlphaPipelineStage.

[javagl]

The test failure fixed by ad274f8 stumped me too!

It may be a detail, but ... you know, I'm a strickler. To be clear: Unless I'm overlooking something obvious, the code change in this commit should not have an effect. The behavior should be the same in both cases. Everybody could (and should be able to!) throw in such a change into a PR, as part of a cleanup, and it should not cause randomly and inexplicably cause the build to break.

Do you happen to know (whether the test failure was really related to that, and not just a random fluke, and) what exactly the failure was? (It may not be worth spending additional time for that, but it caused quite a 🤨 for me...)

---

Unused parameters could be checked via eslint, but I think it might require too many exceptions.

I guess that this is somewhat related to the concept of an "interface" that does not exist on the same level as it does in a proper(ly) typed language. There probably are many places where parameters are passed in, just to match an "interface". There probably are solutions for that. (Maybe something like letting the parameter name start with an _underscore so that eslint does not complain about it being unused).
Regardless of that, one could consider enabling this rule, once, locally, and cleaning up all unused parameters that can be cleaned up, and then disabling it again, for the CI. Not really important, though...

---

Beyond the scope of (and somewhat unrealated to) this PR, but a few short notes:

The pattern ... was discussed #10486 (comment).

The pattern of
for (const key in myObject) ... hasOwnProperty .. const value = myObject[key];
has the obscure part of the hasOwnProperty call, and the additional overhead of another "map lookup" via myObject[key].

In terms of alternatives (or rather: possible improvements), there are many things to consider. Just quick bullet points:

• Object.keys(...) creates an array. This may be prohibitively expensive and generate a lot of garbage. Who knows...
• The for (key in object) pattern might internally also create an array. Or it boils down to some Iterable construct. Who knows...
• When only the values are required, then Object.values(....) may be beneficial. It avoids the lookup (but still creates an array)

One important question are modifications. I just did a quick test. The behavior is not tooo surprising, but ... subtle.

The runModify.... functions iterate over keys/values in different ways, and they all add/modify a property.

• runModifyA iterates over the properties "the old way", and adds a property. Will this be picked up in the iteration?
  • No. That's OK. But it suggests that the set of keys also is "stored" somewhere, like an array...
• runModifyA and runModifyB iterate over the properties, and change the value of a property. Will the modified value be visible during the iteration?
  • Yes, of course
• runModifyC iterates over the Object.values. Will changes in the values be picked up during the iteration?
  • No, of course not! The Object.values creates a "snapshot" of the array when the iteration starts

Now, breaking that down to machine code and examining the performance- and memory implications for objects with 10, 1000, 100000 properties is a different story, but I was curious about the basics. You never know...

function Example() {
  this.propertyA = "A";
  this.propertyB = true;
  this.propertyC = 123;
}

function runModifyA(example) {
  for (const key in example) {
    if (example.hasOwnProperty(key)) {
      const value = example[key];
      console.log(value);
      if (value === true) {
        example.propertyC = "CHANGED_VALUE";
        example.propertyD = "ADDED_VALUE";
      }
    }
  }
}

function runModifyB(example) {
  for (const key of Object.keys(example)) {
    const value = example[key];
    console.log(value);
    if (value === true) {
      example.propertyC = "CHANGED_VALUE";
      example.propertyD = "ADDED_VALUE";
    }
  }
}

function runModifyC(example) {
  for (const value of Object.values(example)) {
    console.log(value);
    if (value === true) {
      example.propertyC = "CHANGED_VALUE";
      example.propertyD = "ADDED_VALUE";
    }
  }
}

function print(example) {
  for (const value of Object.values(example)) {
    console.log(value);
  }
}

const exampleA = new Example();
const exampleB = new Example();
const exampleC = new Example();

console.log("runModifyA");
runModifyA(exampleA);
console.log("\n");

console.log("result:");
print(exampleA);
console.log("\n");
console.log("\n");

console.log("runModifyB");
runModifyB(exampleB);
console.log("\n");

console.log("result:");
print(exampleB);
console.log("\n");
console.log("\n");

console.log("runModifyC");
runModifyC(exampleC);
console.log("\n");

console.log("result:");
print(exampleC);
console.log("\n");
console.log("\n");

[jjhembd]

@javagl

Do you happen to know (whether the test failure was really related to that, and not just a random fluke, and) what exactly the failure was?

Here is the spec that breaks when that last frustumSplits assignment is pulled out of the loop:

1) disables frustum culling
     Scene/Model/Model frustum culling
     TypeError: Cannot read properties of undefined (reading 'far')
    at View.createPotentiallyVisibleSet (packages/engine/Source/Scene/View.js:440:69 <- Build/CesiumUnminified/Cesium.js:213675:72)

For completeness, here is the change I make to get that error. In View, starting at line 434,

  for (let j = 0; j < numFrustums; ++j) {
    frustumSplits[j] = frustumCommandsList[j].near;
    //if (j === numFrustums - 1) {
    //  frustumSplits[j + 1] = frustumCommandsList[j].far;
    //}
  }
  frustumSplits[numFrustums] = frustumCommandsList[numFrustums - 1].far;

The failure (and the fix) have been 100% repeatable for me over many runs, so I don't think it's a random fluke.

Your runModify tests might give a clue? Perhaps some hidden state of frustumCommandsList is stored somewhere for the loop, and changes after modifying previous elements? But I haven't been able to track it down.

[javagl]

The reason for why that commit made sense was (of course, embarrassingly obvious in hindsight): For the case of numFrustums === 0, it accessed the frustumCommandList[-1].
(With the version that works, the frustumSplits are a 1-element array that only contains undefined, which ... may not make sense either, but apparently, these frustumSplits are only used for the DebugCameraPrimitive, so this may usually not cause an issue...)

[jjhembd]

Thanks, that makes sense @javagl! Basically if numFrustums == 0, we don't even execute the loop so we avoid the problem.

@ggetz do you want to take a second pass at this? I think it's ready to go.