Don't allow picking on the sunPostProcess framebuffer.

[emackey]

Fixes #5478.

Likely fixes #5402.

/cc @bagnell @AnneWoepse

@bagnell I'm curious about the if block I'm modifying here. It has two else if clauses which could possibly both be false. What happens if all three paths go false at the same time? Seems like the framebuffer would not be assigned, would it have a reasonable default?

[pjcozzi]

Please update CHANGES.md

[emackey]

@bagnell Also, it's concerning that picking would "stay broken" even after this code switched back to using the globeDepth framebuffer. Is switching framebuffers here somehow unsafe for picking?

[bagnell]

What happens if all three paths go false at the same time? Seems like the framebuffer would not be assigned, would it have a reasonable default?

It will default to whatever the pass state framebuffer is. When picking, it will be the picking framebuffer. If the pass state framebuffer is undefined, it'll render to the default framebuffer (directly to canvas).

Also, it's concerning that picking would "stay broken" even after this code switched back to using the globeDepth framebuffer. Is switching framebuffers here somehow unsafe for picking?

Yes, during the pick pass it should never switch framebuffers. All of the post processing and globe depth copying should be disabled when picking.

[bagnell]

I'm not sure this is the right fix. The condition should always evaluate to false when picking. All of the environment should not render and all post processing should be disabled when frameState.pick is true.

[emackey]

Maybe the environment isn't being updated correctly? The commit that caused this (a9a275f) changes the location of the environment update.

[emackey]

And, notably, the offending commit didn't touch the line I've changed here. It remotely changed the results of this if check.

[emackey]

All of the post processing and globe depth copying should be disabled when picking.

During my testing, I added console.log statements to the various if clauses here, and the globe depth one (the middle one) does get executed when the Sun post process filter isn't in effect. Maybe this whole section of code needs stronger protection from picking.

[bagnell]

Maybe the environment isn't being updated correctly?

I think it might be updating correctly but out of order? If you look at Sun.update you'll see this block of code:

if (!frameState.passes.render) {
    return undefined;
}

That would cause environmentState.isSunVisible to be false in that condition.

During my testing, I added console.log statements to the various if clauses here, and the globe depth one (the middle one) does get executed when the Sun post process filter isn't in effect. Maybe this whole section of code needs stronger protection from picking.

That's strange, if you look several lines up you'll see this:
var useGlobeDepthFramebuffer = environmentState.useGlobeDepthFramebuffer = !picking && defined(scene._globeDepth);

[emackey]

That's strange

Oh, I didn't limit my console logging to just the pick pass, fixed now. So yes, in master, the pick pass chooses either Sun bloom (the first clause, when the Sun is visible in the frame), or it falls past all three conditionals to leave the framebuffer alone (skipping over globe depth and fxaa).

This means that once the Sun comes into view, the pick framebuffer is permanently set to the Sun Bloom post-process one, permanently breaking picking even after the Sun goes away, which is the observed behavior.

[bagnell]

Ah, OK, I found the real issue. The environment state is never updated on a pick. We simply need to call updateEnvironment in Scene.pick right before updateAndExecuteCommands. Do you you want to submit the fix?

[emackey]

Actually I added the call in two places, is this OK?

[bagnell]

Yup, thanks @emackey!