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 OIT on machines where it was broken. #2441

Merged
merged 3 commits into from
Feb 2, 2015
Merged

Fix OIT on machines where it was broken. #2441

merged 3 commits into from
Feb 2, 2015

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jan 29, 2015

As pointed out by one of the Chromium developers, we were using an undefined variable in czm_alphaWeight due to a double declaration of the z variable resulting in scoping issues. This change then lead to another issue where textures were too dark, which was fixed by flipping q.w

We also discovered that #2327 is related to czm_alphaWeight and may be an issue with the weighting function. @bagnell is going to look into it separately.

This makes OIT work everywhere and disabling it should no longer be necessary.

As pointed out by one of the Chromium developers, we were using an undefined
variable in `czm_alphaWeight` due to a double declaration of the `z` variable
resuling in scoping issues.  This change then lead to another issue that
where textured were black, which was fixed by flipping `q.w`

We also discovered that #2327 is related to `czm_alphaWeight` and may
be an issue with the weighting function. @bagnell is going to look into it separately.
@mramato
Copy link
Contributor Author

mramato commented Jan 29, 2015

Here's the Chrome issue: https://code.google.com/p/chromium/issues/detail?id=414280

Fixes #2129

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 29, 2015

I will test this on Mac / Intel tomorrow.

@chris-cooper
Copy link
Contributor

Nice! This fixes the issue in Ubuntu 14.04 on a Dell XPS 14 for me. The "Geometry and Appearances" Sandcastle example shows pixel flickering in master but not in the fix-oit branch.

@gberaudo
Copy link
Contributor

It also fixes the issue on my machine. Thank you guys.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 30, 2015

Code looks good. Works on my my MacBook Pro forced to Intel.

@bagnell can you please verify this in scenes with high depth complexity like we did before http://cesiumjs.org/2014/03/14/Weighted-Blended-Order-Independent-Transparency/

@bagnell can you also followup in the Chrome issue to confirm that this is not a GLSL compiler issue that they need to test for (assuming that is the case)?

When we merge this, please post this to the forum and also followup on the few threads where this was discussed.

CC @kenrussell

@mramato
Copy link
Contributor Author

mramato commented Jan 30, 2015

I also tested this on OS X with an Radeon HD6630M and Linux Mint with an Intel HD 4000. Both showed the problem before and both are now fixed.

@kenrussell
Copy link

Please do take a little time and see if you can reduce this into a small test case. It looks like there may be bugs in many GLSL compilers related to shadowing of variables, and it's critically important that we expose these issues so browsers can provide uniform behavior, and so that the underlying bugs may eventually be fixed. Thanks.

@bagnell
Copy link
Contributor

bagnell commented Feb 2, 2015

The weighting function still needs to be investigated, but this fixes the issue and still matches master so I'm merging.

bagnell added a commit that referenced this pull request Feb 2, 2015
Fix OIT on machines where it was broken.
@bagnell bagnell merged commit 8c46dd8 into master Feb 2, 2015
@bagnell bagnell deleted the fix-oit branch February 2, 2015 17:17
@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 2, 2015

@kenrussell we'll see if we can put together a trimmed down test to see if this was 100% our issue or a potential GLSL shadowing issue.

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