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

Billboard transparency #4886

Merged
merged 17 commits into from
Jan 26, 2017
Merged

Billboard transparency #4886

merged 17 commits into from
Jan 26, 2017

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Jan 19, 2017

Fixes #2130.

@emackey
Copy link
Contributor

emackey commented Jan 20, 2017

Of course, it should be noted that this hurts performance for apps that use large numbers of billboards and points, because they are now drawn 2-pass instead of single-pass. But the visual improvement may be worth the cost.

@mramato
Copy link
Contributor

mramato commented Jan 20, 2017

Of course, it should be noted that this hurts performance for apps that use large numbers of billboards and points, because they are now drawn 2-pass instead of single-pass. But the visual improvement may be worth the cost.

Feedback has shown that it is absolutely worth the cost. Visual quality improvements is something that is asked on the forum on a constant basis.

@@ -1502,21 +1519,22 @@ define([
vaLength = va.length;

colorList.length = vaLength;
for (j = 0; j < vaLength; ++j) {
for (j = 0; j < vaLength * 2; ++j) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but extract * 2 from the loop.

@@ -1482,11 +1483,27 @@ define([
vs.defines.push('DISTANCE_DISPLAY_CONDITION');
}

fs = new ShaderSource({
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in the point cloud, shouldn't the render state be different for each pass? Opaque writes depth; translucent does not.

vec4 color = texture2D(u_atlas, v_textureCoordinates) * vertexColor;
if (color.a == 0.0)

#ifdef RENDER_FOR_PICK
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment here and to the point shader that explains this? For example: fully transparent parts are not pickable, and the billboard/point is rendered twice: the opaque pass discards translucent fragments, and the translucent pass discards opaque ones.

}

command.primitiveType = PrimitiveType.POINTS;
command.pass = (j % 2 === 0) ? Pass.OPAQUE : Pass.TRANSLUCENT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make a public enum with the following properties:

  • OPAQUE
  • TRANSLUCENT
  • OPAQUE_AND_TRANSLUCENT

Billboard, label, and point collections can expose this property (get/set ideally) and default to OPAQUE_AND_TRANSLUCENT. Make sure the reference doc clearly defines the performance vs. visual quality tradeoffs.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 21, 2017

Add unit tests. Checking the number of commands returned by update and their Pass would be good.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 21, 2017

Just those comments.

@bagnell
Copy link
Contributor Author

bagnell commented Jan 26, 2017

@pjcozzi This is ready for another look.

@@ -133,6 +135,9 @@ define([
* @param {Matrix4} [options.modelMatrix=Matrix4.IDENTITY] The 4x4 transformation matrix that transforms each billboard from model to world coordinates.
* @param {Boolean} [options.debugShowBoundingVolume=false] For debugging only. Determines if this primitive's commands' bounding spheres are shown.
* @param {Scene} [options.scene] Must be passed in for billboards that use the height reference property or will be depth tested against the globe.
* @param {BillboardRenderTechnique} [options.renderTechnique=BillboardRenderTechnique.OPAQUE_AND_TRANSLUCENT] The billboard rendering technique. The default
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be exposed on LabelCollection as well?

I don't think users would use the opaque version, but they would likely use the translucent one for a performance boost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that they would always be both, but that isn't true anymore with the label backgrounds. I'll add it.

*
* @exports BillboardRenderTechnique
*/
var BillboardRenderTechnique = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this applies to points and labels as well, I don't think it should have Billboard in the name. I am also hesitant about having technique in the name since in glTF and computer graphics, this isn't really a technique in the material sense.

Perhaps throughout, this is better named BlendOption (too confusing with blend mode? probably not at this level of the API), ImageBlendOption, RenderPasses (again, too similar to the renderer?), or CompositeMode? Ignore the implementation and think about what would sound reasonable to the end user.

@@ -34,6 +34,7 @@ Change Log
* `TerrainProvider` now optionally exposes an `availability` property that can be used to query the terrain level that is available at a location or in a rectangle. Currently only `CesiumTerrainProvider` exposes this property.
* Added `sampleTerrainMostDetailed` to sample the height of an array of positions using the best available terrain data at each point. This requires a `TerrainProvider` with the `availability` property.
* Added 2D and Columbus View support for models using the RTC extension or whose vertices are in WGS84 coordinates. [#4922](https://github.com/AnalyticalGraphicsInc/cesium/pull/4922)
* Transparent parts of billboards, labels, and points no longer overwrite parts of the scene behind them. [#4886](https://github.com/AnalyticalGraphicsInc/cesium/pull/4886)
Copy link
Contributor

Choose a reason for hiding this comment

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

Update this.

/**
* The billboard rendering technique. The default is used for rendering both opaque and translucent billboards.
* However, if either all of the billboards are completely opaque or all are completely translucent,
* setting the technique to BillboardRenderTechnique.OPAQUE or BillboardRenderTechnique.TRANSLUCENT can improve performance by 2x.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and throughout, perhaps "up to 2x"

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 26, 2017

Just those comments.

@bagnell
Copy link
Contributor Author

bagnell commented Jan 26, 2017

@pjcozzi This is ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 26, 2017

Amazing.

@Petter-li
Copy link

I still have the problem. billboards img set a png, the transparency part is disappear;
version:"cesium": "^1.55.0";
can u guys told me how to fix it. Thanks.

@BoJIbFbI4
Copy link

hi, same for 1.71. I'm wanna change alpha for pins that not selected for a little bit, but no matter to pin's alpha, if polygon has alpha - pins will be faded
polygon with alpha:
image
image
polygon w/o alpha:
image
image

@OmarShehata
Copy link
Contributor

@BoJIbFbI4 let's move the discussion to the Cesium community forum: https://community.cesium.com/. Please include a Sandcastle example that reproduces the issue in your question, see: https://community.cesium.com/t/how-to-share-custom-sandcastle-examples/9828.

@BoJIbFbI4
Copy link

@OmarShehata, hi. Thnx for the quick response! For me build the 99% same behavior example will take unforgivable amount of time. Maybe you know that 'fixed' issue, or can give me some advice?

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.

Transparent billboards/points not working as expected.
7 participants