BufferPrimitiveCollection: Material API#13313
Conversation
|
Thank you for the pull request, @donmccurdy! ✅ We can confirm we have a CLA on file for you. |
There was a problem hiding this comment.
It looks like BufferPrimitiveCollection.byteLength is not currently used anywhere in the render path, 3d tiles cache logic, or similar runtime systems?
Anyway, for this PR, should we still update BufferPrimitiveCollection.byteLength in BufferPrimitiveCollection.js to include this._materialView.byteLength?
| * @property {Color} [color=Color.WHITE] | ||
| * @property {BufferPolylineMaterial} [material=BufferPolylineMaterial.DEFAULT_MATERIAL] | ||
| * @property {TypedArray} [positions] | ||
| * @property {number} [width=1] |
There was a problem hiding this comment.
result.width = options.width ?? 1; has already been removed, please remember update the docs accordingly.
Also, the JSDoc/examples for BufferPointCollection, BufferPolylineCollection, and BufferPolygonCollection still reference the old color initialization and setColor() workflow. Since this PR moves styling to material classes, those examples should we update to use material / setMaterial() instead?
There was a problem hiding this comment.
Good catch – I think I've updated all the documentation now.
I'd like for the type checker to either catch errors like the unused width, or to extend the BufferPrimitiveMaterialOptions so we don't have to duplicate so much here, but I think that's currently blocked by #10455.
…ollection.byteLength
That's correct, I believe
Good call, I'd forgotten to add the new buffer to |
| /** | ||
| * Internal-only, supported in Vector Tiles with 3D Tiles 2.0. | ||
| * @memberof Cesium3DTileStyle.prototype | ||
| * @type {StyleExpression} | ||
| * @ignore | ||
| */ | ||
| lineWidth: { | ||
| get: function () { | ||
| return this._lineWidth; | ||
| }, | ||
| set: function (value) { | ||
| this._lineWidth = getExpression(this, value); | ||
| this._style.lineWidth = getJsonFromExpression(this._lineWidth); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I'm taking the liberty of adding a lineWidth property to Cesium3DTileStyle.js here, for alpha testing. That said... it isn't implemented in any stable part of the 3D Tiles spec, including the older vector tiles format, so I've omitted it from published types and documentation with the @ignore JSDoc tag above.
| showSizeAndColorArray[i * 3 + 1] = material.size; | ||
| showSizeAndColorArray[i * 3 + 2] = AttributeCompression.encodeRGB8( | ||
| material.color, | ||
| ); | ||
|
|
||
| outlineWidthAndOutlineColorArray[i * 2] = 0; // TODO: Material API. | ||
| outlineWidthAndOutlineColorArray[i * 2] = material.outlineWidth; | ||
| outlineWidthAndOutlineColorArray[i * 2 + 1] = | ||
| AttributeCompression.encodeRGB8(Color.WHITE); // TODO: Material API. | ||
| AttributeCompression.encodeRGB8(material.outlineColor); |
There was a problem hiding this comment.
I would prefer for the material to take care of packing itself into vertex attributes, rather than making the renderer know about that, but ... for now I think that gets into too many questions of how we'll bridge the draped / non-draped rendering code paths, and if/when we want to try to consolidate materials with existing vector data types in Cesium.js.
danielzhong
left a comment
There was a problem hiding this comment.
Approved! All my review comments have been addressed, and it looks good to go on my end.
|
It looks like there are some test errors that need to be fixed before merging. @donmccurdy |
…vecollection-materials BufferPrimitiveCollection: Material API
16b5f0e to
5cd8ba3
Compare
Description
Material API for BufferPoint, BufferPolyline, and BufferPolygon collections.
As discussed recently, putting style-related properties directly on BufferPrimitive objects doesn't scale well to supporting multiple renderers and rendering styles, so I'm splitting these properties off into new, packable material classes.
Why new material classes? Unlike the BufferPrimitive classes — where breaking from existing APIs was really necessary for our performance goals — I don't think there's any fundamental reason that BufferPrimitiveMaterial "needs" to be distinct from existing material types like
PolylineOutlineMaterialandPolylineDashMaterial. But (1) working out how to share existing materials is non-trivial, (2) existing materials don't appear to cover points and polygons, and (3) our timeline is somewhat tight. So I think that designing a "common" material system to be shared by existing and Buffer- vector collections can be left as a future consideration.Features
My intention would be to support color / outlineColor / outlineWidth for all three topologies "soon", but this seems like enough to start testing. Transparency and polygon offset will also be useful. Dashed outlines and arrows might be good additions later.
In a small change from earlier BufferPoint iterations, I've renamed 'pixelSize' to 'size'. This is meant to better match the 'pointSize' naming in Cesium3DTileStyle, and the
line.widthproperty which doesn't include a 'pixel' prefix, at the cost of no longer matching the naming convention of PointGraphics.Example
Note that after calling
setMaterial(...), no reference to the BufferPrimitiveMaterial instance is held, and any subsequent changes to the material will have no effect untilsetMaterial(...)is called again. The collection simply updates the relevant buffers on each call.Preview
material-api.webm
Issue number and link
Testing plan
See attached unit tests.
Sandcastle example
Author checklist
CONTRIBUTORS.mdI have updatedCHANGES.mdwith a short summary of my changePR Dependency Tree
This tree was auto-generated by Charcoal