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

Make polygons with an image material render as translucent #8349

Closed
wants to merge 1 commit into from

Conversation

OmarShehata
Copy link
Contributor

This came up on the forum.

Here's a PNG with a transparent background:

Cesium

If you add this as a material to a polygon, you get a black background:

image

Code:

var viewer = new Cesium.Viewer('cesiumContainer');

viewer.entities.add({
    polygon : {
        hierarchy : Cesium.Cartesian3.fromDegreesArray([-93.42146226782502, 40.966453300249995, -93.41909064452037, 40.966453300249995, -93.41909064452037, 40.96882492355465, -93.42146226782502, 40.96882492355465]),
        material : "../images/Cesium_Logo_overlay.png",
        height: 100
    }
});


viewer.zoomTo(viewer.entities);

But if you comment out the height so it's clamped, it works fine:

image

This inconsistency is because the ground primitives batch will override the translucency property of a material:

https://github.com/AnalyticalGraphicsInc/cesium/blob/dceac54c0e96bcbd3f07e9632746c5e0e05a1025/Source/DataSources/StaticGroundGeometryPerMaterialBatch.js#L123-L126

Whereas the regular primitive batch will not:

https://github.com/AnalyticalGraphicsInc/cesium/blob/dceac54c0e96bcbd3f07e9632746c5e0e05a1025/Source/DataSources/StaticGeometryPerMaterialBatch.js#L131-L134

This PR updates the regular primitive batch to behave the same as the ground primitive one. In retrospect, I'm not sure if this is the right solution. The real problem is that the material declares it is not translucent, because the color's alpha channel is 1:

https://github.com/AnalyticalGraphicsInc/cesium/blob/dceac54c0e96bcbd3f07e9632746c5e0e05a1025/Source/Scene/Material.js#L1102-L1119

But I think for an image material, the behavior should be translucent by default, since you would expect an image with transparency to render as transparent. If we make the change in Material.js then the batches won't need to override this.

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@@ -130,7 +130,7 @@ import Property from './Property.js';
geometryInstances : geometries,
appearance : new this.appearanceType({
material : this.material,
translucent : this.material.isTranslucent(),
// omitting translucent property to override it
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming we do take this change, a link back to this PR for details and/or a more expanding explanation would be helpful

@mramato
Copy link
Contributor

mramato commented Nov 2, 2019

The main issue here is the inconsistency between groundprimitive/primitive, but there's more going on. material in your example is an instance of ImageMaterialProperty, which has a transparent property. So in master you can do:

var viewer = new Cesium.Viewer('cesiumContainer');

viewer.entities.add({
    polygon : {
        hierarchy : Cesium.Cartesian3.fromDegreesArray([-93.42146226782502, 40.966453300249995, -93.41909064452037, 40.966453300249995, -93.41909064452037, 40.96882492355465, -93.42146226782502, 40.96882492355465]),
        material : new Cesium.ImageMaterialProperty({ 
            image:"../images/Cesium_Logo_overlay.png",
            transparent: true
        }),
        height: 100
    }
});

viewer.zoomTo(viewer.entities)

and get the behavior you want.

Cesium historically defaulted to transparent false because transparent true causes issues if the image is no transparent. We definitely need to look into the big picture more before we do anything.

@OmarShehata
Copy link
Contributor Author

Thanks for the context. I think it's OK to have the user supply the transparent: true parameter, and the only thing we need to fix here is the inconsistency. That code was originally written by @likangning93 but here's where the Dan says that the "render states are overridden for ground primitives":

#6393 (comment)

I just tried this to confirm. It's not actually possible to make a GroundPrimitive NOT translucent. So I think there need not be any code fix here, other than to document the translucent and closed options have no effect when the polygon is clamped?

@cesium-concierge
Copy link

Thanks again for your contribution @OmarShehata!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

2 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @OmarShehata!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @OmarShehata!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 26, 2020

@mramato @OmarShehata what's the status of this PR?

@mramato
Copy link
Contributor

mramato commented Feb 26, 2020

We can probably just close this PR. Sounds like there may be a non-documented limitations in GroundPrimitive that perhaps @OmarShehata can open an issue for if he agrees.

@hpinkos hpinkos closed this Feb 26, 2020
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.

4 participants