-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Billboard transparency #4886
Conversation
…ransparent fragments.
Conflicts: Source/Scene/BillboardCollection.js
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) { |
There was a problem hiding this comment.
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({ |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
Add unit tests. Checking the number of commands returned by |
Just those comments. |
@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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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"
Just those comments. |
@pjcozzi This is ready. |
Amazing. |
I still have the problem. billboards img set a png, the transparency part is disappear; |
@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. |
@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? |
Fixes #2130.