Property Table Textures#13155
Property Table Textures#13155mzschwartz5 wants to merge 8 commits intoproperty-texture-type-supportfrom
Conversation
|
Thank you for the pull request, @mzschwartz5! ✅ We can confirm we have a CLA on file for you. |
9dad7d3 to
bba586e
Compare
|
|
||
| const propertiesArray = Object.entries(propertyTexture.properties).filter( | ||
| ([id, property]) => property.isGpuCompatible(), | ||
| ([id, property]) => property.classProperty.isGpuCompatible(), |
There was a problem hiding this comment.
I'm aware this function call is missing an argument - it gets fixed in #13163
There was a problem hiding this comment.
The changes to this file are just me moving functions from PropertyTextureProperty.js to a lower level class so that they can be used by property table properties as well.
There was a problem hiding this comment.
These changes constitute the meat of this PR. There has been significant discussion and investigation as to where to create the texture for the property table.
Ultimately, I decided on doing it in this parse function, and doing it synchronously. We can always revisit this if it proves to be an issue, but it's certainly the most straightforward way to do it, and it seems to work well (in my limited testing).
It does have some implications: the PropertyTable stores the texture (in contrast to other glTF resources, which are typically stored on the relevant glTF loaders), and thus some new methods are needed to clean up resources.
There was a problem hiding this comment.
This approach feels pretty clean to me.
There was a problem hiding this comment.
These unit tests are really just moved from the other file, nothing super new here.
lilleyse
left a comment
There was a problem hiding this comment.
Looks good @mzschwartz5, just a couple comments from me. I appreciate the inline comments.
| PropertyTable.prototype.destroy = function () { | ||
| if (defined(this._texture)) { | ||
| this._texture.destroy(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
destroyObject is typically called in destroy functions. Example from BatchTable.js:
| PropertyTable.prototype.destroy = function () { | |
| if (defined(this._texture)) { | |
| this._texture.destroy(); | |
| } | |
| }; | |
| PropertyTable.prototype.destroy = function () { | |
| this._texture = this._texture && this._texture.destroy(); | |
| return destroyObject(this); | |
| }; |
| pixelFormat: PixelFormat.RGBA, | ||
| pixelDatatype: PixelDatatype.UNSIGNED_BYTE, |
There was a problem hiding this comment.
I see how RGBA/UNSIGNED_BYTE makes integration with #13135 easier, but RGBA/FLOAT would allow for accessing larger elements, e.g. VEC2/3/4 of FLOAT.
I don't want this to hold up the PR, so maybe just update the compatibility matrix to list unsupported type/componentType
| * | ||
| * @private | ||
| */ | ||
| StructuralMetadata.prototype.destroy = function () { |
|
As discussed offline, this sandcastle is significantly slower on this branch due to property table texture creation. Need to investigate further. import * as Cesium from "cesium";
// A demo showing how to isolate elements in an architectural design by CategoryID
// Snowdon Towers sample data courtesy of Autodesk Revit.
const viewer = new Cesium.Viewer("cesiumContainer");
// Set to 1 PM Philadelphia time in UTC
viewer.clock.currentTime = Cesium.JulianDate.fromIso8601(
"2024-11-22T18:00:00Z",
);
// Set the initial camera view
viewer.camera.setView({
destination: Cesium.Cartesian3.fromDegrees(-79.886626, 40.021649, 235.65),
orientation: {
heading: 0,
pitch: Cesium.Math.toRadians(-20),
roll: 0,
},
});
// Add the architectural tileset
let archTileset;
try {
archTileset = await Cesium.Cesium3DTileset.fromIonAssetId(2887123);
viewer.scene.primitives.add(archTileset);
} catch (error) {
console.log(`Error loading tileset: ${error}`);
}
// Add the site tileset
try {
const tileset = await Cesium.Cesium3DTileset.fromIonAssetId(2887129);
viewer.scene.primitives.add(tileset);
} catch (error) {
console.log(`Error loading tileset: ${error}`);
} |
|
@javagl interesting... I was testing on #13163 and assumed this PR was the problem but that seems not to be the case.
Strange, because the sandcastle doesn't even use custom shaders. |
|
Interesting indeed...
Quickly skimming over the base state, there now are these property table infos. The number of properties does not seem to be excessively large. But... a wild guess what could very well cause the slowdown: It will create a new shader for each different set of properties. (For different numbers of properties, and if that is applicable: Even for every different order of properties!). From quickly debugstepping through that, the number varies wildly, between 2 and ~50. So it might very well be that it has to compile and link dozens of shaders, with each of them being ~"considerably complex" for itself (because it's the model shader), and with additional (considerable) complexity due to all the metadata stuff. This will 💥 the |
This would surprise me, but I'll have to look into it more closely. I believe I made it so that every property in a property table gets put in the shader, whether or not a given model actually uses it (and use the default value if it's unused)... so for there to be a lot of different shaders being compiled and linked here, it would have to mean a lot of different property tables. Maybe I'm wrong about what I implemented though - in which case, the solution is simple (do what I thought I was already doing). |
|
Disclaimer: I have not investigated the resulting shader code. I only saw that the pipeline stage was called with all these different sets of property values (and I was actually in a call at that time, so didn't check how many shaders it actually did build). If all that should already go into a single shader, then the answer to the question why |
|
Looking closer, I think you're right Marco. In an example I debugged, each gltf primitive specifies a subset of properties from the table it references. The texture and shader only use those referenced properties. Which means a different texture and shader for each gltf primitive, potentially. That's no good... each property table should be uploaded once, in its entirety. And each primitive referencing that table should have a shader that has access to all properties, even if it only uses some (the rest will just be default values). This will require a little rethinking about how I load the tables as textures. Right now, it's happening per gltf primitive - but I think it's more of a per-tileset operation. (But I don't think there's a single place in a tileset where all property tables are defined... so... is this even possible?) |
The closest one could again be a schema that is "pulled up" into the tileset and defines some sort of holistic view on the metadata of all glTFs that may appear. Otherwise, it could happen at any point in time that a new tile with a glTF is loaded that uses a metadata schema that was never used before. One rough idea for a workaround could be to have some place where all the properties are collected, and the shader builder mechanism only kicks in and regenerates (and assigns) a new shader when a new property is encountered. But I don't have enough context to say how feasible that sounds... |


**Depends on #13135**
This PR is a step in adding support for representing property tables from the Structural Metadata glTF extension on the GPU - namely, for use in Custom Shaders.
Currently, property tables are only resident on the CPU. They can be used for selection and declarative styling, but not for custom shaders. The linked issue below goes into some of the challenges and work involved in using a property table on the GPU.
This PR adds functionality to glTF loading that uploads property table properties as GPU textures. The next PR will augment metadata shader code generation to allow use of these textures.
Issue number and link
#13124
Testing plan
Since this is only half of the functionality, there's no good way to test.
Testing should be mostly limited to regression testing. Here are some relevant sandcastles to smoke test (ensure parity between prod and local version):
Custom shaders with property textures
Legacy batch tables
Test with the EXT_mesh_features and EXT_structural_metadata 3D-tiles / glTF samples
Note: some issues may exist that may have been solved in the next PR. For instance, I didn't know at the time that
componentTypeis undefined forENUMtype properties - which instead usevalueTypeto specify bytewidth. Many instances ofcomponentTypechange tovalueTypein the next PR.Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change