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

Change blending options in attempt to fix overlapping billboard fringes. #5066

Merged
merged 6 commits into from
Mar 16, 2017

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Mar 2, 2017

If this works, it should fix #5007. Hopefully it whacks this mole without other ones popping up.

To see old behavior, compare GeoJSON in 1.27 to GeoJSON in 1.30. The newer version is on the right:

badblending1 27_1 30

This branch offers this:

maybefixedblending

If you look closely at the 1.27 shot, you can see in the "rounded" corners of the billboard where the old bug was "punching holes" in the ones behind, making the corners appear more squared off. In the 1.30 shot, everything is messed up, but the "punching holes" bug at least is gone.

On this branch, this demo appears more orderly. There's a slight issue remaining, where low-alpha fringes of the inset icons punch holes in the background color of the same icon. This may actually be a bug with the alpha values coming out of the PinBuilder, since the output billboard should be calling for opaque fragments there in the interior, regardless of the inset graphic. Edit: Issue #5097, fixed in #5099.

Anyway, I tested this with label backgrounds, and with non-backgrounded labels. It seems to mostly work. There's some strangeness where a non-background label that sticks into terrain will have its opaque fragments occluded at a slightly different depth from the translucent fragments. Edit: Issue #5083. But mostly I think this is a big improvement.

@emackey
Copy link
Contributor Author

emackey commented Mar 2, 2017

Here's an example of what I'm talking about with labels sticking into mountains. I know the text looks crowded and bad, but there are not supposed to be any holes in the glyphs.

Cesium 1.27 is too old to load this format of gist. Cesium 1.28 needs to be manually set to "Natural Earth II" base map, but then it works, and it looks perfect:

Cesium 1.30 exhibits the label depth strangeness, which means it is not introduced by this branch:

bigbadlabels

So, it looks like this is a separate, but likely related, issue.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 8, 2017

So, it looks like this is a separate, but likely related, issue.

Agreed. Can you submit a separate issue for this?

@@ -1465,7 +1465,8 @@ define([
this._rsTranslucent = RenderState.fromCache({
depthTest : {
enabled : true,
func : WebGLConstants.LEQUAL // Allows label glyphs and billboards to overlap.
// Allows label glyphs and billboards to overlap.
func : ((this._blendOption === BlendOption.TRANSLUCENT) ? WebGLConstants.LEQUAL : WebGLConstants.LESS)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this._blendOption === BlendOption.TRANSLUCENT really mean? Does it mean "this billboard collection is for a label collection with backgrounds?" If so, can we just asked the label collection or have the label collection pass in a private flag so this code is more obvious?

Copy link
Contributor Author

@emackey emackey Mar 8, 2017

Choose a reason for hiding this comment

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

Good eye, but no. this._blendOption === BlendOption.TRANSLUCENT means that the entire billboard is being written in the translucent pass, without an opaque pass. You'll notice on #L1471 that we already use the same test to determine whether or not to write to the depth buffer. This is because, when the opaque pass is NOT in use, we rely on the depth buffer to save us from OIT making everything blend with everything, even if it means "punching holes" in things. When the 2-pass system is in use, we're avoiding the "punching holes" bug by using the depth buffer only in the opaque pass and not the translucent pass.

So, the difference between single-pass and 2-pass controls a difference in strategy of how we use the depth buffer, and thus should also control the depth test function. In single-pass mode we use LEQUAL because the depth buffer is in use and we need to overlap fragments at the same depth. In 2-pass mode we use LESS to avoid the fringe overlap problem.

Basically, with this change, single-pass mode has returned to the behavior from before the "punching holes" fix, making it fully backwards-compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, can you improve the Allows label glyphs and billboards to overlap comment to better explain this?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 8, 2017

Also, update CHANGES.md.

@emackey
Copy link
Contributor Author

emackey commented Mar 8, 2017

New issue #5083 submitted for the label depth problem mentioned above.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 8, 2017

Does anyone else want to look at this? @bagnell?

@emackey
Copy link
Contributor Author

emackey commented Mar 10, 2017

Updated.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 10, 2017

OK with me. Does anyone else want to test this out?

I labeled this next release so we get it into master soon.

@emackey
Copy link
Contributor Author

emackey commented Mar 13, 2017

Now that #5099 was merged to master, and I merged master in here, this fix is looking better than any of the screenshots posted originally.

betterblendingfix

@bagnell
Copy link
Contributor

bagnell commented Mar 13, 2017

This looks good to me.

@emackey
Copy link
Contributor Author

emackey commented Mar 15, 2017

This is ready.

@bagnell bagnell merged commit b58120d into master Mar 16, 2017
@bagnell bagnell deleted the blending-badness branch March 16, 2017 20:14
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.

Problem with overlapping billboards
3 participants