Improve documentation and readability in Scene and depth-related code #12188

jjhembd · 2024-09-11T00:15:19Z

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 · 2024-09-11T00:15:39Z

Thank you for the pull request, @jjhembd!

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

ggetz · 2024-09-11T13:10:33Z

@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...)

packages/engine/Source/Scene/PostProcessStage.js

packages/engine/Source/Scene/Scene.js

jjhembd · 2024-09-17T03:16:10Z

@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 · 2024-09-17T03:37:39Z

@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 · 2024-09-17T11:43:36Z

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 · 2024-09-17T12:57:19Z

@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 · 2024-09-17T16:01:22Z

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 · 2024-09-17T17:02:39Z

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.

ggetz

A few comments. Many are not necessary to merge the PR, but I figured I'd point them out. :)

ggetz · 2024-09-18T17:19:56Z

packages/engine/Specs/Scene/TranslucentTileClassificationSpec.js

-    function executeCommand(command, scene, context, passState) {
-      command.execute(context, passState);
+    function executeCommand(command, scene, passState) {
+      command.execute(scene._context, passState);


Since this is external to the class, let's use the getter: scene.context.

ggetz · 2024-09-19T13:45:09Z

packages/engine/Source/Core/PerspectiveFrustum.js

+
+  //>>includeStart('debug', pragmas.debug);
+  if (frustum.fov < 0 || frustum.fov >= Math.PI) {
+    throw new DeveloperError("fov must be in the range [0, PI).");


No changes strictly needed for the sake of this PR, but keep in mind that Check has some functions to help with number ranges like this.

ggetz · 2024-09-19T13:48:10Z

packages/engine/Source/Scene/DerivedCommand.js

+
+  if (!writesDepthOrDiscards && !usesLogDepth) {
+    const source =
+      "void main() \n" + "{ \n" + "    out_FragColor = vec4(1.0); \n" + "} \n";


Also, while strictly not needed for this PR, keep in mind that template strings can help with multiline blocks.

Yes, some of this was a little messy. I went ahead and updated to template strings in this file.

ggetz · 2024-09-19T20:49:04Z

packages/engine/Source/Scene/Scene.js

  const hasLogDepthDerivedCommands = defined(derivedCommands.logDepth);
  const hasHdrCommands = defined(derivedCommands.hdr);
  const hasDerivedCommands = defined(derivedCommands.originalCommand);
  const needsLogDepthDerivedCommands =
    useLogDepth && !hasLogDepthDerivedCommands;
  const needsHdrCommands = useHdr && !hasHdrCommands;
+  // PERFORMANCE_IDEA: if (!useLogDepth && !useHdr), we may be able to save a call to updateDerivedCommands.


This may be up for debate, but I'm generally not a fan of leaving TODO comments like this in the code. It's too easy to forget about them and then they never get implemented. Do you think this will indeed be useful for a future developer?

ggetz · 2024-09-19T20:49:58Z

packages/engine/Source/Scene/Scene.js

+/**
+ * Get the central body occluder for the scene.
+ *
+ * TODO: The occluder is the top-level globe. When we add


Similar comment about TODO comments in the code. Would this be more helpful if it lived in an issue instead?

In this case, I couldn't find any discussion about "support for multiple central bodies," so I don't think an issue would be appropriate.
I changed the comment to simply describe the assumptions made by this method, so that if someone does want to discuss multiple bodies, they will understand what to change.

ggetz · 2024-09-19T20:52:12Z

packages/engine/Source/Scene/Scene.js

+ * based on its bounding volume.
+ *
+ * @param {DrawCommand} [command] The draw command
+ * @param {CullingVolume} cullingVolume The culling volume of the current Scene.


The order of "optional, required, optional" parameters is non-ideal. Again, not strictly required for this PR, but it would be best if we ordered the signature with 'required, optional, optional".

jjhembd · 2024-09-27T03:02:37Z

@ggetz thanks for the review! I think I addressed all your comments.

ggetz · 2024-09-27T13:31:54Z

Great, thanks @jjhembd! To merge in main, you'll want to follow @jjspace's instructions to make things less of a headache.

The last commit before Prettier v3 was used to reformat the code

This commit reformats all code using prettier v3 relative to the pre-prettier-v3 tag

jjhembd · 2024-09-27T19:45:58Z

@ggetz merge is done!

ggetz · 2024-09-27T20:26:07Z

Looks good! Thanks @jjhembd!

Jeshurun Hembd added 12 commits September 10, 2024 12:46

Update PostProcessStageCollection to follow Coding Guide

875e0a8

Clean up DerivedCommand

52da7fd

More cleanup in Scene, View, PostProcessStage, GlobeDepth

4daa78e

Cleanup PostProcessStage, Scene, add docs in View

faa07f7

Edit docs related to depth buffers

2d6ea50

Fix spec failure

ad274f8

Simplify executeCommand signature in Scene

e36f1cf

Clarify docs and calculations for projection matrices

22bf29b

Document more private functions in Scene

05d9864

Update Framebuffer to follow current CodingGuide

e250d72

Add docs, simplify signatures for depth buffers

854907b

Incorporate ideas from PR 12058

611bcca

jjhembd added cleanup category - doc labels Sep 11, 2024

jjhembd requested a review from javagl September 11, 2024 00:15

jjhembd mentioned this pull request Sep 11, 2024

Break executeCommands into functions #12058

Closed

javagl reviewed Sep 11, 2024

View reviewed changes

packages/engine/Source/Scene/PostProcessStage.js Show resolved Hide resolved

packages/engine/Source/Scene/Scene.js Outdated Show resolved Hide resolved

packages/engine/Source/Scene/Scene.js Show resolved Hide resolved

Address TODO comment

0350152

jjhembd mentioned this pull request Sep 17, 2024

Update Coding Guide with notes on destructuring assignment #12196

Open

ggetz reviewed Sep 19, 2024

View reviewed changes

ggetz self-assigned this Sep 20, 2024

PR feedback

556c767

Jeshurun Hembd added 2 commits September 26, 2024 22:59

Merge branch 'main' into scene-cleanup

8389281

Remove PERFORMANCE_IDEA comment in Scene

a5e0059

Jeshurun Hembd added 3 commits September 27, 2024 12:03

Merge tag 'pre-prettier-v3' into scene-cleanup-merge

72821f4

The last commit before Prettier v3 was used to reformat the code

Merge tag 'post-prettier-v3' into scene-cleanup-merge

9b5c4e3

This commit reformats all code using prettier v3 relative to the pre-prettier-v3 tag

Merge branch 'main' into scene-cleanup-merge

0316474

jjhembd mentioned this pull request Sep 27, 2024

Update prettier to v3 #12206

Merged

6 tasks

ggetz approved these changes Sep 27, 2024

View reviewed changes

ggetz merged commit 1101847 into main Sep 27, 2024
9 checks passed

ggetz deleted the scene-cleanup branch September 27, 2024 20:26

This was referenced Dec 20, 2024

Depth Test Failure / Z-fighting #12337

Closed

Fix intermittent z-fighting issue in DepthPlaneVS #12385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve documentation and readability in Scene and depth-related code #12188

Improve documentation and readability in Scene and depth-related code #12188

jjhembd commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Sep 11, 2024

ggetz commented Sep 11, 2024

javagl left a comment •

edited

Loading

jjhembd commented Sep 17, 2024

jjhembd commented Sep 17, 2024

javagl commented Sep 17, 2024 •

edited

Loading

jjhembd commented Sep 17, 2024

javagl commented Sep 17, 2024

jjhembd commented Sep 17, 2024

ggetz left a comment •

edited

Loading

ggetz Sep 18, 2024

ggetz Sep 19, 2024

ggetz Sep 19, 2024

jjhembd Sep 27, 2024

ggetz Sep 19, 2024

ggetz Sep 19, 2024

jjhembd Sep 27, 2024

ggetz Sep 19, 2024

jjhembd commented Sep 27, 2024

ggetz commented Sep 27, 2024

jjhembd commented Sep 27, 2024

ggetz commented Sep 27, 2024

Improve documentation and readability in Scene and depth-related code #12188

Improve documentation and readability in Scene and depth-related code #12188

Conversation

jjhembd commented Sep 11, 2024 • edited Loading

Description

Issue number and link

Testing plan

Author checklist

github-actions bot commented Sep 11, 2024

ggetz commented Sep 11, 2024

javagl left a comment • edited Loading

Choose a reason for hiding this comment

jjhembd commented Sep 17, 2024

jjhembd commented Sep 17, 2024

javagl commented Sep 17, 2024 • edited Loading

jjhembd commented Sep 17, 2024

javagl commented Sep 17, 2024

jjhembd commented Sep 17, 2024

ggetz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjhembd commented Sep 27, 2024

ggetz commented Sep 27, 2024

jjhembd commented Sep 27, 2024

ggetz commented Sep 27, 2024

jjhembd commented Sep 11, 2024 •

edited

Loading

javagl left a comment •

edited

Loading

javagl commented Sep 17, 2024 •

edited

Loading

ggetz left a comment •

edited

Loading