Scene cleanup in preparation for sampling height

[lilleyse]

Contains a lot of the Scene cleanup work in #6934.

For reviewing I recommend reading the commits in order. I did a bunch of cleanup as I tried to fully wrap my head around Scene.js. A lot of the log depth changes were added so that derived commands would not thrash when going between rendering the scene with log depth and then rendering the 1x1 orthographic pass, for the sampleHeight and pickRay functions coming in a later PR.

This doesn't add new functionality to the API so no tests or CHANGES.md update are needed.

@bagnell since almost all of this is in Scene.js can you review? I've been pretty careful not to break anything but it would be good to have another set of eyes for these changes.

[cesium-concierge]

Thanks for the pull request @lilleyse!

• ✔️ Signed CLA found.
• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.
• Works (or fails gracefully) in IE11.

---

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

[bagnell]

There are issues when toggling log depth. For example, open the Picking development Sandcastle example and add this near the top after scene is defined.

Sandcastle.addToggleButton('Log Depth', scene.logarithmicDepthBuffer, function(checked) {
    scene.logarithmicDepthBuffer = checked;
});

Toggling it on and off will make some of the primitives disappear.

[bagnell]

Toggling log depth can also crash. The error was that a vertex array of a command was destroyed.

[bagnell]

Everything else I tested works fine and the code looks good. You can probably fix the errors I posted above with command.dirty = command.dirty || frameState.useLogDepthDirty; in updateDerivedCommands.

I'll merge this right after the next release since this changes a bunch of the scene code and it'll have a lot of time in master.

[lilleyse]

@bagnell I get the same problems in master too.

I think I've figured out why the crashes are happening though. I see it regularly when zoomed in close to the globe and repeatedly toggling log depth by pressing the c key in this demo.

At some point a globe tile's vertex array is destroyed and recreated. The derived commands still reference the destroyed resources. If the pick pass happens before the render pass it gets to this area:

            if (scene.frameState.passes.pick && !defined(command.pickId)) {
                return;
            }

            derivedCommands.depth = DerivedCommand.createDepthOnlyDerivedCommand(scene, command, context, derivedCommands.depth);

and returns without creating the depth command. Later when executing the command

        if (passes.pick || passes.depth) {
            if (passes.pick && !passes.depth && defined(command.derivedCommands.picking)) {
                command = command.derivedCommands.picking.pickCommand;
                command.execute(context, passState);
                return;
            } else if (defined(command.derivedCommands.depth)) {
                command = command.derivedCommands.depth.depthOnlyCommand;
                command.execute(context, passState);
                return;
            }
        }

it sees the depth command exists but crashes because the resources are destroyed.

If the render pass happens before the pick pass the depth command gets recreated and this isn't a problem.

I have one idea for a fix - DrawCommand could have a property saying that a command is a pick command. Classification primitives and anything else that has custom pick commands would set this to true. The code in Scene would look like this

            if (command.isPickCommand) {
                return;
            }

            derivedCommands.depth = DerivedCommand.createDepthOnlyDerivedCommand(scene, command, context, derivedCommands.depth);

So anything that doesn't have custom pick commands, like globe tiles, will always update their depth command regardless of the pass. Does this approach make sense? Or is there something simpler without needing to add a new property.

[bagnell]

That approach seems good to me. Thanks for tracking that down. You can add the fix here or in another PR since I plan on merging this today after the release.

[lilleyse]

@bagnell I'm working on that fix now and will add it to this PR.

[lilleyse]

Fixed the pick command bug, but ran into a more general bug which I think is happening because useLogDepthDirty only cares about the current frame but if a command goes offscreen and log depth changes it won't recreate its derived commands when it goes back on screen. I pushed a fix in cb7f94d but I don't really like it because it recreates derived commands for every log depth switch which is what I was trying to prevent initially. I have another idea that I'm going to try out.

[lilleyse]

@bagnell this is ready now.

I added the pickOnly flag to ClassificationPrimitive, EllipsoidPrimitive, and Vector3DTilePrimitive. I was wondering about GroundPolylinePrimitive but it didn't seem like it needed it. Are there any files I'm missing?

To fix the other derived command bug I check whether a command already has log depth derived commands and recreate those commands even if log depth is currently off.

As part of that reorganization I simplified how cast and receive shadows derived commands are created.

[bagnell]

Are there any files I'm missing?

No, that should be all of them.

This looks good to me. Merging.

[pjcozzi]

@lilleyse given the scope and location of these changes, can you please profile a representative scene or two before and after - and make sure there isn't anything out of the ordinary, e.g., different number of draw commands, rendering the entire scene twice, etc.?

If we didn't already, we should test all the Sandcastle examples, including VR, post processing, etc.

[pjcozzi]

Should this be in Math.js?