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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Mar 16, 2017
Merged

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e30f96d
Change blending options in attempt to fix overlapping billboard fringes.
emackey Mar 2, 2017
2c5708f
Refactored test and code comment.
emackey Mar 10, 2017
c2420ac
Merge remote-tracking branch 'origin/master' into blending-badness
emackey Mar 10, 2017
e4c80d6
CHANGES.md
emackey Mar 10, 2017
722c811
Merge remote-tracking branch 'origin/master' into blending-badness
emackey Mar 13, 2017
514857d
Merge remote-tracking branch 'origin/master' into blending-badness
emackey Mar 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Source/Scene/BillboardCollection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1453,7 +1453,7 @@ define([
this._rsOpaque = RenderState.fromCache({
depthTest : {
enabled : true,
func : WebGLConstants.LEQUAL // Allows label glyphs and billboards to overlap.
func : WebGLConstants.LESS
},
depthMask : true
});
Expand All @@ -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 •
edited
Loading

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?

},
depthMask : this._blendOption === BlendOption.TRANSLUCENT,
blending : BlendingState.ALPHA_BLEND
Expand Down