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 polygon offset check for shadows #4559

Merged
merged 6 commits into from
Nov 1, 2016
Merged

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Oct 31, 2016

Yeah...

I think the Chrome 49->50 switch happened right before I introduced this bug so I never noticed that I had accidentally turned polygon offset off.

Now shadow aliasing should be reduced in Firefox. However glPolygonOffset still seems to have almost no effect in Chrome which I opened an issue for here: https://groups.google.com/forum/#!topic/webgl-dev-list/E1dAG65QBhg. Shadowing in Chrome works fine now in version 49 or when ANGLE is disabled in the current version.

@mramato
Copy link
Contributor

mramato commented Oct 31, 2016

Update CHANGES. Do we want this for tomorrow's release? Seems like an obvious fix.

@lilleyse
Copy link
Contributor Author

Update CHANGES. Do we want this for tomorrow's release? Seems like an obvious fix.

Updated. Yes this ought to go in.

@lilleyse lilleyse force-pushed the shadows-polygon-offset branch from ae9f803 to 300191d Compare October 31, 2016 19:31
@@ -185,8 +185,9 @@ define([
this._needsUpdate = true;

// In IE11 polygon offset is not functional.
// TODO : Also disabled for Chrome temporarily. Re-enable once https://groups.google.com/forum/#!topic/webgl-dev-list/E1dAG65QBhg is resolved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Write up a Cesium bug instead of linking to a forum post. (the Cesium bug can include the above link, of course).

If we ever need to do this check in more than one place, we should move the code to FeatureDetection but it's fine here for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, the new issue is here: #4560

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is specifically an ANGLE problem, why are we disabling on Chrome in all cases? We have lots of non-ANGLE Chrome users (Mac/Linux/Android/iOS)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, the check is now Chrome + Windows

@mramato
Copy link
Contributor

mramato commented Nov 1, 2016

Thanks.

@mramato mramato merged commit a7f1c6f into master Nov 1, 2016
@mramato mramato deleted the shadows-polygon-offset branch November 1, 2016 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants