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

Fixed rendering issues related to environmentState.renderTranslucentDepthForPick. #9844

Closed
wants to merge 3 commits into from

Conversation

ugnelis
Copy link

@ugnelis ugnelis commented Sep 29, 2021

Solves #9824.

I have tested only cases related to transparent entities and point clouds with shading.

It is possible that this should also have to be fixed for the Cesium VR https://github.com/CesiumGS/cesium/blob/main/Source/Scene/Scene.js#L2925-L2927

@cesium-concierge
Copy link

Thank you so much for the pull request @ugnelis! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ❌ Missing CLA.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

@ugnelis
Copy link
Author

ugnelis commented Oct 5, 2021

Update: this fix brings other issues when viewer.camera.switchToPerspectiveFrustum() / viewer.camera.switchToOrthographicFrustum() is used. I believe the bug lays somewhere here

scene._primitives.update(frameState);
. Right now I am not able to solve this bug as I am not aware why updateAndRenderPrimitives is not included for renderTranslucentDepthForPick = true mode.

@ebogo1
Copy link
Contributor

ebogo1 commented Oct 25, 2021

Thanks @ugnelis - I've confirmed we received your signed CLA - we'll take a look at this as soon as we get a chance!

@lilleyse
Copy link
Contributor

This seems like a good fix to me.

The reason why updateAndRenderPrimitives is not included for renderTranslucentDepthForPick = true is because scene.render() is expected to be called before pickPosition according to the documentation (https://cesium.com/learn/cesiumjs/ref-doc/Scene.html#pickTranslucentDepth). The idea is that the render would have already updated the primitives so we don't need to do it again, but this is a faulty assumption because primitives will push different commands depending whether it's the pick pass or render pass. I think that's why pick works but render doesn't in your workaround: #9824 (comment).

@ugnelis can you restore your cesium fork? I was going to push to this branch but the remote is gone. My changes are here: https://github.com/CesiumGS/cesium/compare/pick-translucent-depth-fix?expand=1.

Update: this fix brings other issues when viewer.camera.switchToPerspectiveFrustum() / viewer.camera.switchToOrthographicFrustum() is used.

I don't see this problem on my branch. Here's the sandcastle I've been using for testing: Local sandcastle

@lilleyse lilleyse mentioned this pull request Dec 21, 2021
2 tasks
@ugnelis
Copy link
Author

ugnelis commented Dec 28, 2021

Hi @lilleyse, I have tried to restore my fork but I couldn't manage to do so successfully: it is missing some of my commits. Sorry about that. I suggest creating a new branch from this one where you can apply your changes

@lilleyse
Copy link
Contributor

lilleyse commented Jan 4, 2022

@ugnelis no problem, I opened a separate PR and included you in CONTRIBUTORS.md: #9991

@lilleyse lilleyse closed this Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants