BufferPrimitive: Add 'alpha' support#13384
Conversation
|
Thank you for the pull request, @donmccurdy! ✅ We can confirm we have a CLA on file for you. |
| typedArray: collection._positionView, | ||
| context, | ||
| usage: BufferUsage.DYNAMIC_DRAW, | ||
| usage: BufferUsage.STATIC_DRAW, |
There was a problem hiding this comment.
Unrelated - I've set all buffer attributes to STATIC_DRAW, just to keep things consistent for now. In the future I'd like to make this an option the user can control, if we can measure a difference in performance, which I haven't managed to do (at least on my machine) yet.
| * @readonly | ||
| * @ignore | ||
| */ | ||
| this._blendOption = options.blendOption ?? BlendOption.TRANSLUCENT; |
There was a problem hiding this comment.
I chose TRANSLUCENT as the default to reduce aliasing — if aliasing were less of an issue, and/or if the default canvas resolution in CesiumJS matched the display (#13336) then OPAQUE would probably make more sense as the default.
There was a problem hiding this comment.
BillboardCollection and PointPrimitiveCollection default to OPAQUE_AND_TRANSLUCENT. That might be the better option long term though it does require splitting the rendering into two commands, an opaque command and a translucent command.
There was a problem hiding this comment.
Great PR! Few questions:
- Translucent mode should submit the the pass to
Pass.TRANSLUCENTright? I seerenderBufferPointCollection.jsand polylines, polygons still using:
pass: Pass.OPAQUE,
Is there any sample test cases for me to test it as well?
- When
blendOptionis TRANSLUCENT, the render state doesn't setdepthMask: false. This means transparent primitives still write to the depth buffer, I'm not sure but do you think will this affect anything? PointPrimitiveCollectionexposesblendOptionas a plain public property, allowing users to read and update it at runtime. For now,BufferPrimitiveCollectiononly stores_blendOptionas a private field. Not sure if we need to have getter or setter for this (maybe no need?).
Good catch, thanks! Yes, probably we should derive the pass from the blend option. Done.
The sandcastle snippet above is an easy test, or any of our existing vector tilesets can be tested by using styles like this: tileset.style = new Cesium.Cesium3DTileStyle({
color: "color('white', 0.25)",
pointSize: 4,
pointOutlineWidth: 2,
pointOutlineColor: "color('purple', 0.25)",
});I'm not aware that it's possible to change the blend option on a tileset currently.
Good question. It definitely could affect things, but I don't think I'd want to derive the depth write setting directly from the blend option. I think we could expose depth buffer settings on the collection later, though, if that becomes necessary.
I'd be OK with letting users change this setting later, but no immediate use case for it, so I'm inclined to wait and keep things simple for now. /cc @lilleyse just FYI, in case any of this feels like it may be inconsistent with CesiumJS user expectations? |
FWIW, cesium/packages/engine/Source/Scene/PointPrimitiveCollection.js Lines 1003 to 1033 in d81e43d |
|
Thanks @lilleyse! My only hesitation is that I'd recalled alpha blending helping to mitigate aliasing recently, for otherwise opaque geometry, related to #13336. I should probably make a reproduction to confirm that use case, but if so, then disabling depthMask when alpha blending is enabled would make it difficult to use that approach for opaque geometry. Not feeling firmly decided on this either way. Related to the "Create a Sandcastle comparing BufferPoint/Polyline/Polygon classes to existing CesiumJS APIs..." task in #13444. |
Description
Changes:
blendOptionparameter in BufferPrimitiveCollection constructorsmaterial.color.alphain BufferPrimitiveCollection subclassesmaterial.outlineColor.alphain BufferPointCollectionTasks:
collection.blendOptionper-collection optionIssue number and link
Testing plan
Unit tests added.
Sandcastle:
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my changeAI acknowledgment
If yes, I used the following Tools(s) and/or Service(s):
If yes, I used the following Model(s):
PR Dependency Tree
This tree was auto-generated by Charcoal