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

Day/Night lighting broken in 2D #4122

Closed
hpinkos opened this issue Jul 14, 2016 · 13 comments
Closed

Day/Night lighting broken in 2D #4122

hpinkos opened this issue Jul 14, 2016 · 13 comments

Comments

@hpinkos
Copy link
Contributor

hpinkos commented Jul 14, 2016

It only
orks sometimes depending on where the camera is
day-night-2d

viewer.scene.globe.enableLighting = true;
@hpinkos
Copy link
Contributor Author

hpinkos commented Jul 18, 2016

@jeffwhitty
Copy link

+1, I'm seeing this with the November release. Found this while searching before logging a duplicate.

I noticed if I pan up and down it seems to happen as well as side to side, but I haven't spotted a consistent factor to narrow down the cause.

@cfairchi
Copy link

Still seeing this issue in 1.34. Any ETA on a fix?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 29, 2017

@ggetz could you please try to triage Cesium releases to see if this previously worked and find the commit that broke this?

@sspeaks
Copy link

sspeaks commented Sep 20, 2017

I've found this issue to be introduced post version 1.21.0. Is there any ETA on a fix for this?

@hpinkos
Copy link
Contributor Author

hpinkos commented Sep 20, 2017

Thanks for tracking that down @sspeaks! This is marked high priority so we hope to have time to look at it soon, but our team is spread pretty thin right now. If anyone has time to look into it, we are always happy to review a pull request with a fix!

@emackey
Copy link
Contributor

emackey commented Sep 20, 2017

I ran git bisect on this. It was introduced in either f924c16 or its parent commit 041ee30 (these are both work-in-progress commits). The parent of those commits b590677 did not display the problem.

These commits were merged to the old models-2D branch as part of #3933, and that branch was later merged to master.

(For my own reference, what follows below is the Sandcastle snippet that was bisected)

var img = Cesium.createDefaultImageryProviderViewModels();
var len = img.length;

var viewer = new Cesium.Viewer('cesiumContainer', {
    imageryProviderViewModels: img,
    selectedImageryProviderViewModel: img[len - 1]
});
viewer.scene.globe.enableLighting = true;
viewer.scene.morphTo2D(0);

@ghost
Copy link

ghost commented Nov 30, 2017

I must admit I am kind of ignorant of frustums and 3d graphics, so take this with a grain of salt.

if (!is2D) {
     curNear = Math.max(near, Math.pow(farToNearRatio, m) * near);
     curFar = Math.min(far, farToNearRatio * curNear);
 } else {
     curNear = Math.min(far - nearToFarDistance2D, near + m * nearToFarDistance2D);
     curFar = Math.min(far, curNear + nearToFarDistance2D);
}

This bit of code seems to be the culprit in the more recent version of Scene.js.

If you use the code in the if (!is2D) block for all cases(including 2d), it seems to eliminate the problem. Similarly, increasing nearToFarDistance2D from 1.76e6 to 1.76e7 also fixes the issue.

//i.e. Just use this instead of the if/else
curNear = Math.max(near, Math.pow(farToNearRatio, m) * near);
curFar = Math.min(far, farToNearRatio * curNear);

I'm sure either of those fixes breaks countless other things though. I would be happy to try and fix this with a PR, but I fear my lack of knowledge on the subject would waste your time with reviews.

@hpinkos
Copy link
Contributor Author

hpinkos commented Nov 30, 2017

Thanks @sspeaks610 ! @bagnell what do you think?

@hpinkos
Copy link
Contributor Author

hpinkos commented Mar 3, 2018

Also reported by @craigp1231 in #6303

@ggetz
Copy link
Contributor

ggetz commented Aug 7, 2018

@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/d/msg/cesium-dev/vD3SiO5V6U8/-mMOU-bgCgAJ
https://groups.google.com/forum/#!topic/cesium-dev/-eSNYd_9xQk

If this issue affects any of these threads, please post a comment like the following:

The issue at #4122 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https:/AnalyticalGraphicsInc/cesium.


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

🌍 🌎 🌏

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 8, 2018

@sspeaks610 @cfairchi @jeffwhitty This was just fixed and will be included in the Cesium 1.49 release available September 3rd. Thanks!

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

No branches or pull requests

8 participants