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

Revert 1fda8c0 #6779

Merged
merged 5 commits into from
Jul 8, 2018
Merged

Revert 1fda8c0 #6779

merged 5 commits into from
Jul 8, 2018

Conversation

ProjectBarks
Copy link
Contributor

@ProjectBarks ProjectBarks commented Jul 6, 2018

This should fix a bug that was preventing 3D Tilesets from being occluded when they were not using the depth testing. There is still a outstanding logarithmic depth bug bit this seems connected to an already documented bug.

Related to #6714

@cesium-concierge
Copy link

cesium-concierge commented Jul 6, 2018

Thank you so much for the pull request @ProjectBarks! 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.
  • ✔️ 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.

🌍 🌎 🌏

@lilleyse
Copy link
Contributor

lilleyse commented Jul 6, 2018

The diff doesn't look quite right. The depthPlane.execute(context, passState); line is reverted correctly but the commands are changed. Also make sure to include the changes to DepthPlane.js in this PR.

Also include yourself in CONTRIBUTORS.md and add an entry to the "Fixes" category in CHANGES.md . Thanks!

@hpinkos
Copy link
Contributor

hpinkos commented Jul 6, 2018

@ProjectBarks can you please update your initial comment for some context of why this change is being made and what problem it fixes? And please link any relevant issues that are open. Thanks!

CHANGES.md Outdated
@@ -8,6 +8,7 @@ Change Log

#### Fixes :wrench:
* Fixed bug causing billboards and labels to appear the wrong size when switching scene modes [#6745](https://github.com/AnalyticalGraphicsInc/cesium/issues/6745)
* Fixed occlusion bug for tiles use a depth pane [#6714](https://github.com/AnalyticalGraphicsInc/cesium/issues/6714)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually word this closer to your statement in the PR description. Maybe something like:

Fixed a bug that was preventing 3D Tilesets on the opposite side of the globe from being occluded

for (j = 0; j < length; ++j) {
executeCommand(commands[j], scene, context, passState);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is still a lot extra than what's in the original commit: 1fda8c0

The command execution should not be changed. Maybe it was a bad git revert? You can hand edit to fix.

@ProjectBarks
Copy link
Contributor Author

@lilleyse Should be good

@lilleyse
Copy link
Contributor

lilleyse commented Jul 8, 2018

Looks good now. It fixes the occlusion bug and doesn't seem to have affected other areas. Classification related Sandcastle demos still work.

@lilleyse lilleyse merged commit 3c710c5 into CesiumGS:master Jul 8, 2018
lilleyse added a commit that referenced this pull request Jul 31, 2018
@lilleyse lilleyse mentioned this pull request Jul 31, 2018
hpinkos pushed a commit that referenced this pull request Jul 31, 2018
@hpinkos
Copy link
Contributor

hpinkos commented Jul 31, 2018

@ProjectBarks sorry, but we had to revert these changes in #6866 because it broke pickPosition

@lilleyse opened up an issue here to address the 3D tileset occlusion problem: #6867
Please add any comments you have, thanks =)

@cr8tpro
Copy link

cr8tpro commented Feb 21, 2020

@hpinkos then isn't there any solution to fix occlusion problem?

@OmarShehata
Copy link
Contributor

@cr8tpro it appears that the occlusion problem referenced here #6867 has been fixed. If you're still experiencing an issue can you post a Sandcastle example reproducing the issue in that issue?

If you're unsure you can always ask in the Cesium forum (https://groups.google.com/forum/#!forum/cesium-dev).

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.

6 participants