Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix eye dome lighting crashes #9719

Merged
merged 5 commits into from
Aug 13, 2021
Merged

Fix eye dome lighting crashes #9719

merged 5 commits into from
Aug 13, 2021

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Aug 11, 2021

Fixes #9715 (local sandcastle)
Fixes #8818 (local sandcastle)
Fixes #6440 (local sandcastle)

Fixes several issues related issue to eye dome lighting using either destroyed or outdated shaders.

For #9715 the issue was basically:

  • Attenuation was on so the derived command was created
  • Attenuation was turned off (the derived command still exists)
  • The style was changed. A new shader is set and the command becomes dirty. Problem now is that the derived command is still using the old shader.
  • After the next frame the command stops being dirty
  • Then attenuation is enabled again and it doesn't rederive the command because the command isn't dirty so it uses the old shader (either deleted or outdated)

The fix was to check if the derived command was using a different shader, and if so, rederive the command.

This is really just a symptom of the poor design of the derived command system which needs to be redone at some point.

CC @ebogo1 @mramato

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

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

@lilleyse lilleyse requested a review from ebogo1 August 11, 2021 21:17
Copy link
Contributor

@ebogo1 ebogo1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lilleyse - I checked that these changes fix the crash and specs pass locally. Mainly just had a question but otherwise this looks good to me.

derivedCommand.framebuffer = this._framebuffer;
derivedCommand.shaderProgram = getECShaderProgram(
frameState.context,
command.shaderProgram
);
derivedCommand.castShadows = false;
derivedCommand.receiveShadows = false;

if (defined(derivedCommandObject)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I understand this, does the else block only get reached on the first frame before pointCloudProcessor is given a value?

Also it might not help performance, but would it be more precise to wrap both the if and else here with an if (originalShaderProgram !== command.shaderProgram) so that the pointCloudProcessor object only gets updated when necessary? In Sandcastle doing this didn't seem to have any effect (as I'd expect) but maybe it's wrong if it's possible that the command or shader program need to get updated for a reason other than the mismatch we check for..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I understand this, does the else block only get reached on the first frame before pointCloudProcessor is given a value?

Yeah that's what's going on. I reorganized the code a bit so it is hopefully more self-documenting.

It should be ok to set originalShaderProgram every time even if the command is rederived for some other reason besides shader changes.

CHANGES.md Outdated Show resolved Hide resolved
@ebogo1
Copy link
Contributor

ebogo1 commented Aug 13, 2021

Thanks @lilleyse - just in case I made sure everything still works after the last commit but this looks good to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants