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

Model - support noData and required from EXT_mesh_features #9887

Closed
ptrgags opened this issue Oct 22, 2021 · 24 comments
Closed

Model - support noData and required from EXT_mesh_features #9887

ptrgags opened this issue Oct 22, 2021 · 24 comments

Comments

@ptrgags
Copy link
Contributor

ptrgags commented Oct 22, 2021

One of the schema changes from EXT_mesh_features is to remove default and optional from the schema and replace them with noData and required. The CesiumJS implementation still needs to be updated for this.

This was out of scope for #9880 since it has wider reaching implications. I want to wait on this until getting metadata working in Custom Shaders

@ptrgags
Copy link
Contributor Author

ptrgags commented Dec 1, 2021

Some more details:

  • Update parsing logic
    • we might need a boolean flag to distinguish EXT_feature_metadata from EXT_mesh_features, because the default behavior is different. the former defaults to properties being required, the latter defaults to properties being optional.
    • noData is not the same as the older defaultValue, as the former treats the valid no data value like a null value, while the latter replaces missing columns with a constant value.
  • CPU Styling/Picking - both make use of getProperty() -- this just needs to be updated to return undefined for noData values.
  • Custom Shaders noData -- should the noData value be passed in to the shader and the user is responsible for interpreting it? I suppose you could use NaN, but this could lead to unexpected behavior if the user isn't aware of it. However, this decision can be deferred until we add metadata properties to custom shaders.

@lilleyse
Copy link
Contributor

lilleyse commented Dec 1, 2021

should the noData value be passed in to the shader and the user is responsible for interpreting it?

Yeah I think that's a good path to start

@lilleyse
Copy link
Contributor

@ptrgags could you post a new summary of the changes required here? Some things have changed in the spec and implementation since - notably defaultValue was added back to the spec and property textures and property attributes were added to custom shaders (which means we can actually start implementing this).

Afterwards we can get noData working for voxels: #10253

@ptrgags
Copy link
Contributor Author

ptrgags commented Jun 22, 2022

Looks like a few details have already been accounted for:

  • Scene/MetadataTableProperty handles the parsing of noData/default for CPU metadata.
  • CPU Styling/Picking - looks like MetadataTableProperty.get()/.set() already handles noData and default in the return value
  • Scene/PropertyTextureProperty - This doesn't explicitly store the default/noData values, but this._classProperty has this information

Remaining details:

  • Scene/PropertyTextureProperty - This doesn't explicitly store the default/noData values, but this._classProperty has this information. These properties could be exposed via getters if needed. Similarly for Scene/PropertyAttributeProperty.
  • Scene/ModelExperimental/MetadataPipelineStage - the code that processes property attributes and property textures needs to be updated.
    • In addition to the struct fields metadata.<propertyName>, we'd need another field to store the noData value. Maybe something like metadata.<propertyName>_noData? This way the user can check for this value and do something different.
    • As for default... I'm not sure the best implementation is. There's potentially 2 parts:
      • If a property has a defaultValue but the property attribute doesn't exist at all, then the result is a constant defaultValue. it could be done by adding a line like this to the shader: (the default would have to be formatted as a valid GLSL type if we want to avoid allocating uniforms.
      metadata.propertyName = default;
      
      • On the other hand, if you have both a default and a noData sentinel, you'd want something like:
      metadata.propertyName = metadata.propertyName == metadata.propertyName_noData ? metadata.defaultValue : metadata.propertyName;
      
      Not sure the performance implications here (not sure if ternary operators lead to divergence)
      • Another option is to just provide metadata.<propertyName>_default and let the user decide what to do with it. Doesn't sound very ergonomic for the custom shader author though, sounds like the user would have to add a lot of boilerplate if statements like the above.
  • Update Documentation/CustomShaderGuide/README.md to document any changes made.

Future:

@lilleyse
Copy link
Contributor

not sure if ternary operators lead to divergence

Might depend on the compiler and complexity of the expressions. czm_branchFreeTernary could be used instead.

@lilleyse
Copy link
Contributor

Hm this brings up bigger questions about the custom shaders API. We also need to consider min, max, and statistics.

What about sub-structs for these special properties? Only problem is it could collide with actual property names.

metadata.noData.propertyName
metadata.default.propertyName
metadata.min.propertyName
metadata.max.propertyName
metadata.statistics.propertyName.min
metadata.statistics.propertyName.max
metadata.statistics.propertyName.occurrences

... other statistics properties including user-defined properties...

Related areas of the spec:

The voxels branch already does it this way for statistics.

@lilleyse
Copy link
Contributor

It seems like this issue could be split into two PRs:

  • Add noData, default, min, max, statistics to the custom shader API
  • Handle noData and default internally when accessing property values

@jjhembd
Copy link
Contributor

jjhembd commented Jul 5, 2022

What about sub-structs for these special properties? Only problem is it could collide with actual property names.

Two options to avoid the naming conflict:

1. Move the property value to metadata.propertyName.value

metadata.propertyName.value
metadata.propertyName.noData
metadata.propertyName.default
  ...
metadata.propertyName.statistics.mean
  ...

This would be a breaking change. Current applications get the property value directly as metadata.propertyName

2. Add a new top-level property to the Vertex/FragmentInput struct

struct VertexInput {
    // Processed attribute values.
    Attributes attributes;
    // Feature IDs/Batch IDs.
    FeatureIds featureIds;
    // Metadata property values.
    Metadata metadata;
    // Information about metadata properties.
    MetadataClassInfo metadataClassInfo;
};

The per-vertex or per-fragment property values would still be accessed as metadata.propertyName. Constant values defined in the schema would be accessed from metadataClassInfo as follows:

metadataClassInfo.propertyName.noData
metadataClassInfo.propertyName.default
  ...
metadataClassInfo.propertyName.statistics.median
  ...

@ptrgags
Copy link
Contributor Author

ptrgags commented Jul 5, 2022

@jjhembd I feel a bit undecided here, but leaning towards the second option since it keeps the basic value access simple (vsInput.metadata.propertyName) rather than nesting it deeper in .value. Though it perhaps is a bit weird to keep one value separate from the rest of the struct.

Side note, overall this input struct feels like it is getting cumbersome to work with given how long the names become. vsInput.metadataClassInfo.propertyName.statistics.median... If we ever qualified property names (see #10085) it would be 7 levels deep in the worst case: vsInput.metadataClassInfo.schemaName.className.propertyName.statistics.median... not ergonomic for writing shaders.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jul 6, 2022

Just jotting down one note from a conversation with @jjhembd, instead of nesting the statistics inside the MetadataClassInfo struct, maybe there could be a separate MetadataStatistics struct so it would be fsInput.metadataStatistics.propertyName.min? this would save a level of nesting.

Less nesting is better not just for ergonomics for the user, but also makes the GLSL code generation easier (fewer dynamic structs to generate)

@jjhembd
Copy link
Contributor

jjhembd commented Jul 23, 2022

#10520 brought properties from the metadata Class Property into the shader.

The next step is to bring in Property Statistics. In some ways, this is conceptually similar to what we did in #10520. But there are 3 additional complexities:

1. Property types are more complicated
The properties min, max, median, and sum can be assumed to be of the same type as the property value. But properties mean, standardDeviation, and variance are inherently float values. We therefore need a type conversion function, to set up float mean, float standardDeviation, float variance for int property values, and vec2 mean for ivec2 property values, etc.

2. Values of statistics are harder to retrieve
Class Properties were available by simply exposing the private members from Scene/PropertyAttributeProperty and Scene/PropertyTextureProperty. I have not yet been able to trace back to where Property Statistics can be retrieved.

3. enum types require special handling
enum metadata will not have min, max, mean, median, sum, standardDeviation, or variance properties in the statistics. Instead, this type of metadata has an occurrences property. In the shader, the occurrences type will be an array of integers, with values corresponding to the number of times the value at that index in the enum occurs in the dataset. I will need a little more time to understand some nuances in the spec.

These factors will affect the time estimate for the next PR. I will follow up with a couple questions.

@jjhembd
Copy link
Contributor

jjhembd commented Jul 23, 2022

@ptrgags, can you point me to where I can find answers to these 2 questions?

  1. Where can I retrieve values for Property Statistics?
  2. What does this phrase mean, in the spec for occurrences?
For fixed-length arrays, this is an array of component-wise occurrences.

Do we have enums with array values?

@ggetz ggetz moved this from Issue/PR closed to In Progress in CesiumJS Issue/PR backlog Jul 25, 2022
@ptrgags
Copy link
Contributor Author

ptrgags commented Jul 25, 2022

  1. Note that statistics only exist in 3DTILES_metadata (the 3D Tiles extension), not the glTF extension. You can access it through tileset.metadataExtension.statistics, that just gives you an object that matches the statistics schema
  2. This is the number of occurrences for enum values, see here: https://github.com/CesiumGS/3d-tiles/blob/f9ad4e1a4c753b91262c63121b3124f38ffe49ff/extensions/3DTILES_metadata/schema/statistics.class.property.schema.json#L69-L87

Do we have enums with array values?

No, but we have arrays of enum values:

propertyEnumArray: {
  type: "ENUM",
  array: true,
  count: 3
  enumType: "myEnum",
},

// so the statistics should be an array with occurrences for index 0, index 1, index 2...

@lilleyse
Copy link
Contributor

Note that statistics only exist in 3DTILES_metadata (the 3D Tiles extension), not the glTF extension. You can access it through tileset.metadataExtension.statistics, that just gives you an object that matches the statistics schema

Statistics also exist for 3D Tiles 1.1. @ptrgags is the API the same for both?

@ptrgags
Copy link
Contributor Author

ptrgags commented Jul 25, 2022

Ah yes, sorry for the confusion. Yes, metadataExtension has the same API for both 3DTILES_metadata and 3D Tiles 1.1 (both are parsed in processMetadataExtension() in Cesium3DTileset)

@jjhembd
Copy link
Contributor

jjhembd commented Jul 25, 2022

Thanks @ptrgags! So if I understand you correctly, the glTF files we are currently using in MetadataPipelineStage spec will not have statistics. I will need to:

  1. Add a test loading a tileset in MetadataPipelineStageSpec
  2. Make metadataStatistics struct construction contingent on the presence of statistics in the input

@jjhembd
Copy link
Contributor

jjhembd commented Jul 25, 2022

I'm still trying to trace back the link between renderResources.model in MetadataPipelineStage and the tileset.metadataExtension.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jul 26, 2022

@jjhembd There are some parent pointers that you can use to locate the tileset:

  • renderResources.model is like a parent pointer that points upwards a few levels of abstraction to the ModelExperimental to which this primitive belongs
  • model.content gives you a pointer to the ModelExperimental3DTileContent, but this is only defined for 3D Tiles (if this was an individual model loaded from ModelExperimental.fromGltf(), this will be undefined)
  • content.tileset is a pointer to the containing tileset.
  • from there you can access tileset.metadataExtension.statistics as described above. If it's present, then yes add it to the metadata struct.

These pointers skip over a few levels of abstraction. To recap the full hierarchy here (from root to leaves):

  • Cesium3DTileset
  • One or more levels of Cesium3DTile (a tree navigated via the tile.children array)
  • A Cesium3DTileContent (interface), in this case ModelExperimental3DTileContent
  • ModelExperimental
  • ModelExperimentalSceneGraph and other abstractions
  • the draw command building pipeline, which in this case receives a PrimitiveRenderResources

@ptrgags
Copy link
Contributor Author

ptrgags commented Jul 26, 2022

Also one thing to consider when implementing statistics (and this goes for anything else that comes from tileset/tile/content metadata as well), is that this information is coarser-grained, so as far as the ModelExperimental is concerned, this information is constant. So it could be passed in as either a uniform, or even hard-coded when generating the shader code. It might make sense to do the hard-coding (where reasonable) to avoid taking up more uniform location (plus in WebGL 1 each uniform update has to be done separately = more WebGL API calls)

@jjhembd
Copy link
Contributor

jjhembd commented Jul 27, 2022

As discussed with @ptrgags: we need to load a 3DTileset in MetadataPipelineStageSpec to test statistics. This took me a little while to understand and get working. Some key steps included:

  1. Set scene.camera to look at the tileset data
  2. Load the tileset using Cesium3DTilesTester, with ModelExperimental enabled:
const tilesetOptions = { enableModelExperimental: true };
return Cesium3DTilesTester.loadTileset(
  scene,
  tilesetWithMetadata,
  tilesetOptions
).then(tileset => {  // ...
  1. Retrieve model from tileset.root.content._model, and use it to mock renderResources
  2. Retrieve a sample primitive from model.sceneGraph.components.nodes[0].primitives[0] for the call to MetadataPipelineStage.process

I couldn't find a sample tileset with statistics. So I spent some time understanding the structure of this tileset:

Specs/Data/Cesium3DTiles/Metadata/AllMetadataTypes/tileset_1.1.json

and am now adding a statistics field with fake values for the test. (Let me know if there's a quick way to get actual values for min, max, mean, etc.)

@jjhembd
Copy link
Contributor

jjhembd commented Jul 27, 2022

One upcoming challenge for MetadataPipelineStage: the types as listed in the tileset.json are not always what we need in the shader. For example, a property with "componentType": "UINT16" and "normalized": true will actually be loaded as a float or vecN type. Two possible ways to handle this:

  • Implement something like PropertyTextureProperty.getGlslType for MetadataStatistics fields
  • Use the MetadataStatistics property name to look up the type used by the corresponding PropertyTextureProperty or PropertyAttributeProperty

@lilleyse
Copy link
Contributor

we need to load a 3DTileset in MetadataPipelineStageSpec to test statistics.

Can we mock the tileset like in PickingPipelineStageSpec? Generally I'm not a fan of mocking objects but it might drastically simplify the test.

Or we should step back and think about whether statistics should be passed along to the model so that it doesn't have to reach back to the tileset.

@lilleyse
Copy link
Contributor

Use the MetadataStatistics property name to look up the type used by the corresponding PropertyTextureProperty or PropertyAttributeProperty

This option seems cleaner to me. It can leverage some of the existing code.

@ggetz ggetz moved this from In Progress to In Review in CesiumJS Issue/PR backlog Aug 16, 2022
@j9liu j9liu changed the title ModelExperimental - support noData and required from EXT_mesh_features Model - support noData and required from EXT_mesh_features Sep 1, 2022
@jjhembd
Copy link
Contributor

jjhembd commented Sep 19, 2022

A significant part of what this issue describes has been implemented in #10520 and #10683.

I opened two new issues, #10799 and #10800, to better define the remaining features that have not yet been implemented.

@jjhembd jjhembd closed this as completed Sep 19, 2022
@ggetz ggetz moved this from In Review to Issue/PR closed in CesiumJS Issue/PR backlog Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Issue/PR closed
Development

No branches or pull requests

4 participants