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

Expose 3D Tiles feature attributes #71

Closed
kring opened this issue Dec 8, 2020 · 18 comments · Fixed by #281
Closed

Expose 3D Tiles feature attributes #71

kring opened this issue Dec 8, 2020 · 18 comments · Fixed by #281
Assignees
Labels
enhancement New feature or request

Comments

@kring
Copy link
Member

kring commented Dec 8, 2020

Because tiles are "sent" to the renderer as glTFs, we should consider encoding feature attributes the way they [are / will be] encoded in 3D Tiles Next.

@kring kring added this to the Initial Release milestone Dec 8, 2020
@kring kring removed this from the Initial Release milestone Feb 25, 2021
@kring kring added the enhancement New feature or request label Apr 27, 2021
@kring kring added this to To do in Cesium for Unreal Apr 28, 2021
@kring
Copy link
Member Author

kring commented May 13, 2021

3D Tiles Next will use the the EXT_feature_metadata extension embedded in the tile glTF to encode feature metadata. So we'd like to use this extension to communicate feature data coming from cesium-native as well. That means converting b3dm batch tables to EXT_feature_metadata on load. There are some challenges to that:

  • EXT_feature_metadata only supports binary properties, not JSON ones. So batch table JSON properties need to be converted to binary.
  • EXT_feature_metadata requires that a given property have the same type across all features. The b3dm batch table is more flexible when encoded in JSON. So we'll have to convert all values to a common representation when converting the batch table. i.e. convert mixed-type properties to strings.
  • EXT_feature_metadata does not allow a property to have a "null" value for a particular property. So here, too, we'll need to convert nulls to zero-length strings.
  • EXT_feature_metadata does not support anything like the 3DTILES_batch_table_hierarchy extension. So the hierarchy will need to be flattened into parallel arrays. This will likely introduce nulls, which will in turn require types to be widened to strings.

As EXT_feature_metadata evolves, or as further extensions with more capability are added, we can revisit these concessions. But this will likely be "good enough" for most tilesets.

@kring
Copy link
Member Author

kring commented May 13, 2021

This code from CesiumJS may be useful: https://github.com/CesiumGS/cesium/blob/master/Source/Scene/parseBatchTable.js

@kring kring moved this from To do to In progress in Cesium for Unreal May 17, 2021
@kring kring self-assigned this May 17, 2021
@baothientran
Copy link
Contributor

baothientran commented May 17, 2021

Current progress from @kring:
https://github.com/CesiumGS/cesium-unreal-samples/tree/picking
https://github.com/CesiumGS/cesium-unreal/tree/picking
https://github.com/CesiumGS/cesium-native/tree/feature-metadata-extension

On the native side, the TODOs are:

  • Add support for binary b3dm batch tables
  • Add support for string and array metadata
  • Add support for the 3D Tiles batch table hierarchy extension

On the unreal side, TODOs are:

  • upport all the metadata types (currently only int8 and float64 are supported)
  • Handle float64 properly; slightly tricky cause glTF doesn't have float64 accessors.
  • Figure out how we can package the actual picking functionality into something more useful for our users. Currently it's a dodgy part of the level blueprint in cesium-unreal-samples.

@kring kring assigned baothientran and unassigned kring May 19, 2021
@baothientran
Copy link
Contributor

baothientran commented May 24, 2021

@kring the common API to access both b3dm and EXT_feature_metadata is in the https://github.com/CesiumGS/cesium-native/tree/feature-metadata-api branch. This is a high level API to access both batch table and gltf EXT_feature_metadata without the need to transcoding from one format to the other.

The high level design is that we have an abstract Metadata class and 5 PropertyView classes. The Metadata is mostly just a simple map from a property name to a PropertyView object. For PropertyView class, we have PrimitivePropertyView to represent scalar, StringPropertyView to represent string, ArrayPropertyView to represent array (this should work for fixed and dynamic array), JsonPropertyView to represent json, and StructPropertyView for struct (this is mostly a std::map<std::string, std::unique_ptr<PropertyView>>). Those PropertyView classes represents the whole collection of instances similar to how batch table work. For example, in the batch table, if ID is a collection of Unsigned Int of instances, then we have a PrimitivePropertyView to represent the array of Unsigned Int similar to that.

So to bridge the difference between batch table and EXT_feature_metadata, we would need to implement those 5 properties for each format, and then on Unreal side, it will only deal with those high level API rather than using the batch table directly. Similar for EXT_feature_metadata. To transcoding from one to the other, one can create Transcoder class and only deal with those high level API rather than the format itself. Please let me know if we should go ahead with it

@baothientran
Copy link
Contributor

@kring I forget to mention, the whole code mostly in CesiumMetadata directory

@kring
Copy link
Member Author

kring commented May 25, 2021

Thanks @baothientran. I took a look at the feature-metadata-api branch, and I guess the main thing I'm unclear on is how this will connect with what we already have. Will there be a new extension to hold a Metadata instance? An additional Metadata property on TileContentLoadResult?

The problem is, I don't really like either of those options.

It seems like a bad idea to have cesium-native specific glTF extensions. Especially when we already have a new extension for metadata that we're in the process of defining, EXT_feature_metadata. It doesn't capture everything in the b3dm batch table with perfect fidelity, of course. If that matters, we should fix the EXT_feature_metadata extension. If it doesn't matter (because we don't think anyone is using the missing features for anything important, or there are acceptable alternative ways to do it), then that should be good enough for cesium-native too.

On the other hand, if we put a Metadata instance in TileContentLoadResult, we're moving away from the ideal scenario where glTF (a well-designed and extensible standard!) is the native language of cesium-native and used to communicate everything about a model/tile to any rendering engines or other applications that use it.

So I think I would still rather see batch table transformed into EXT_feature_metadata. We should provide helpers, along the lines of AccessorView and AccessorWriter to make it easier to read and write feature data stored as EXT_feature_metadata. Ideally, we'd be able to use AccessorView and AccessorWriter directly. The writer would be used by Batched3DModelContent to do the conversion. I think that's putting our engineering effort where it belongs - our new EXT_feature_metadata extension - rather than in creating abstractions over legacy stuff like the b3dm batch table.

@baothientran
Copy link
Contributor

gsl::span is so painfully close to be an ideal interface for an array property type. What it can't represent is the array of string or maybe array of bool (I haven't tested it yet but my gut say it can't because boolean is bit pack together similar std::bitset). For array of string, it typically uses one buffer to store value and two offset buffers to represent array length and string length (https://github.com/CesiumGS/3d-tiles/tree/3d-tiles-next/specification/Metadata/1.0.0#arrays-1). Unfortunately gsl::span doesn't have the concept of offset, so it's not possible without additional allocation. The only solution is to create a custom array view and specialize it for those cases, e.g ArrayView<std::string_view> or ArrayView<bool>

@kring
Copy link
Member Author

kring commented May 27, 2021

I can definitely imagine that some specialization will be needed for AccessorView<std::string_view> and AccessorView<bool>. Is that what you mean?

Unfortunately gsl::span doesn't have the concept of offset, so it's not possible without additional allocation.

I probably don't know what you mean by offset, but it sounds like subspan?

@baothientran
Copy link
Contributor

baothientran commented May 27, 2021

The array type has offset buffer to determine the length of the array for each instance. For example:
To represent: instance = [[1, 2, 3, 4], [3, 4], [5, 7, 8, 9]]
The format will be:
value buffer: [1, 2, 3, 4, 3, 4, 5, 7, 8, 9]
offset buffer: [0, 4, 6, 10] (I simplify it here to indicate instance index, but it's actually byte offset)

The offset buffer means that the first element's array will point to from 0th to 3rd values in the value buffer (4 elements)
the second element's array will point to from 4th to 5th value in the value buffer (2 elements)
and so on

Beside array, string type has the same offset buffer as well.

If they are alone like that, gsl::span and std::string_view are enough to represent those data type, but when we have nested array of string, it's ideal to have gsl::span<std::string_view>. However, the specs says that to represent array of string, we will need two offset buffers. Something like:
value buffer: [["asas", "ttasas"], ["aaaaa"], ["thiaaa", "sssss"]]
array offset: [point to array index]
string offset: [point to string index]

@baothientran
Copy link
Contributor

baothientran commented May 27, 2021

The implementation for the property view is in this link https://github.com/CesiumGS/cesium-native/blob/feature-metadata-extension/CesiumGltf/include/CesiumGltf/PropertyAccessorView.h

For the implementation, I decide not to template the whole class PropertyAccessorView. Instead, client will need to use a getter method to retrieve the value from the buffer. Something like this:

PropertyAccessorView property{}; // initialize property view here
if (property.getType() == PropertyType::Int32) {
   for (size_t intanceIdx = 0; instanceIdx < property.numOfInstances(); ++instanceIdx) {
         int32 val = property.get<int32_t>(instanceIdx);
         // do something with the value
   }
}

The only caveat is that if client casts a type that is different with what property.getType() reports, it is undefined behavior. But it may change to return nullptr instead. The only small problem with returning nullptr is we end up with a branching code every time we want to access an instance. Perhaps I should have two method getUnsafe<T> and getSafe<T>

@baothientran
Copy link
Contributor

baothientran commented May 27, 2021

Another thought is that, let's template PropertyAccessorView, e.g PropertyAccessorView<int32_t>. Then we create a MetadataView to capture the whole ModelExt_Feature_Metadata. So the API usage will be something like this:

MetadataView metadata(ModelExt_Feature_Metadata);

if (metadata.getPropertyType("Height") == PropertyType::Int32) {
   PropertyAccessorView<int32_t> heights = metadata.getProperty<int32_t>("Height");

   for (size_t instanceIdx = 0; instanceIdx < heights.size(); ++instanceIdx) {
        int32_t value = heights[instanceIdx];
   }
}

The whole MetadataView is similar to a lightweight manager to manage properties in the model.

Compare to the first design, the second one may be safer to use because we can check if the property is valid before returning the result of the cast to client. That is we can put a check in the function metadata.getProperty<int32_t>("Height"); and return an optional if the property doesn't have a correct type. And the validation only happens at the beginning and not in the loop like in the previous design

Between two designs, please let me know if you have any opinions on any of them

@kring
Copy link
Member Author

kring commented May 27, 2021

@baothientran your second design definitely looks better to me. You can also provide a method to create a PropertyAccessorView and dispatch it to statically-typed callback, rather than requiring the user to manually switch on the type. Take a look at createAccessorView in AccessorView.h. So using it would look something like this:

struct PropertyVisitor {
  void operator()(const PropertyAccessorView<int32_t>& view) {
   for (size_t instanceIdx = 0; instanceIdx < view.size(); ++instanceIdx) {
        int32_t value = view[instanceIdx];
   }
  }

  void operator()(const PropertyAccessorView<std::string_view>& view) {
    // ...
  }
  
  template <typename T>
  void operator()(const PropertyAccessorView<T>& view) {
    // catch all for other types of properties
  }
};

ModelEXT_feature_metadata& metadata = ...;

// We can add properties directly to ModelEXT_feature_metadata by using
// the `toBeInherited` property in glTF.json, so maybe we don't need a separate
// MetadataView class at all?
metadata.createView("Height", PropertyVisitor{});

@baothientran
Copy link
Contributor

baothientran commented Jun 30, 2021

while I implement blueprint library for metadata, I just realize it doesn't support uint64 and double. I'm not sure what to do with those cases. Possible solutions are

  • to cast to int64 and float and have inaccuracy word in the blueprint function name as a warning to the user.
  • wrap double and float in the UStructs and provide Add, Subtract, Multiply, etc between those structs and native type as well. e.g some blueprint functions are like this Add(FCesiumDouble, FCesiumDouble) or Add(FCesiumDouble, float), etc. Also provide Conversion node to cast to them to native type. e.g: ConvertToFloat(FCesiumDouble) -> float. Note that they are only for blueprints. The C++ interface is still normal. The only disadvantage of this is that it takes effort to maintain

@lilleyse
Copy link
Contributor

CesiumJS has the same dilemma for GPU styling and may require lossy fallbacks/warnings CesiumGS/cesium#9572

@baothientran
Copy link
Contributor

ah I see it converts uint64 to float instead of int64. Good point! I guess we can live with the lossy keyword for blueprint for now

@baothientran
Copy link
Contributor

@kring This is probably a minor problem, but I think it's better to discuss here. Currently, to access metadata, we have *View classes to inspect the data. On the Unreal side, I currently wrap those classes in the USTRUCT to be used with the blueprint. However, one problem is that because those classes don't own the metadata storage (e.g gltf buffer), it requires Gltf Model to be around until the tile is evicted from the cache. So it is probably a little wasteful since we can remove the mesh data right away once we are done creating unreal resources. In the case that we need to remove gltf model to free more memory, the metadata need to copy and own the metadata buffers. So between the two options a) keeping the gltf model around and use the view classes and b) has the metadata owns the its buffers, what's your opinion on them? Or maybe a compromise in the future is that we can have both just like we have std::string and std::string_view

@kring
Copy link
Member Author

kring commented Jul 1, 2021

@baothientran we currently don't unload the glTF data even once we've created the Unreal resources, right? Doing so might be a useful optimization, but it definitely raises some problems, so we'd have to approach it cautiously. Unless I've missed something, I don't think we need to worry about it currently. Our solution to that might be impacted by how we make feature metadata available to Materials as well.

@baothientran
Copy link
Contributor

nope we still keep the gltf currently. The above problem is just a small assumption, but it doesn't affect much with the current implementation of metadata on unreal side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

3 participants