-
Notifications
You must be signed in to change notification settings - Fork 216
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
Allow access to Tileset-level metadata (schema
, schemaUri
, groups
, metadata
)
#709
Conversation
…metadata-take-two
This PR builds on #711 , which builds on #710 , which builds on #706 , which builds on #705 , so I assume that I'm not supposed to really "review" that in all depth (and if I am, just drop me a note). I'm now primarily looking at this to see in how far it addresses #676 . In terms of the core functionality of allowing access to metadata, the current state looks reasonable. With a few caveats: The I could imagine that this is not considered to be in the scope of this PR-chain. And I don't know how much effort it would be to tweak the code generator accordingly. The main reason why I'm mentioning this here (in this "user-oriented" "review") is that this would be a "breaking change" for users. Not a critical one. It would mainly/only change The point of
also bugged me a bit in the experiments that I did in #693 . Right now, I don't see a sensible way for clients to handle this. (
with ~"hybrid" approaches like
or so.
I wonder how users should do that. Forcing users to dive into the depth of the (I think that the approaches of the previous point, i.e. using a Very narrowly focussing on #676 , users will have to...
I think that there could/should be a class that hides all this. This would make accessing the material variants much easier, and (as I said elsewhere) it would also hide possible changes that come from introducing something like a Note: This is also not directly in the scope of this PR. (It could rather be part of an updated state of #693 ). But at some point, there could be classes like this....:
Meaning that users could just call
and have the information that they need. |
Thanks for taking a look, Marco!
Nah I broke this PR out to hopefully make it (and those parts) easier to review. I don't think those other PRs are especially interesting to you, but obviously you're more than welcome to take a look if you like. This is the important one.
Yeah that'd be good! A little tricky and definitely outside the scope of this PR, though. You're right it'll be a breaking change in the future, but a relatively minor one (and we still don't stress about breaking changes in cesium-native too much).
Yeah it's a bit of a hassle. You have to poll. A
Do you have a sense of how common external schemas are? I don't think I've ever seen one, so I was guessing they're rare and no one is too likely to be bothered by this. Loading an external schema should be pretty straightforward, though. Something like this should do the trick, either as a method on the Tileset or as something the user does themselves externally (this is untested and missing some error handling): pAssetAccessor->get(*pMetadata->schemaUri).then([pMetadata](auto&& pRequest) {
SchemaReader reader;
auto result = reader.readFromJson(pRequest->response()->data());
if (result) {
pMetadata->schema = std::move(*result.value);
}
}); If you think this will be common enough to bother with, I'm happy to add a method like this now.
Oops, just my oversight. I've added it. I think it's not strictly needed for types without methods, but a good idea to add anyway.
I definitely agree we can make variant access easier. But I'm currently assuming this tileset-level material variant thing is specific to the company that has requested it, rather than a common thing we should make easy for everyone. If and when there's an official spec for describing tileset-level material variants, it may or may not look like it does today. So I'm hesitant to bake it into our API now. I'd rather provide some code to that one company to help them accomplish their goal for now. In fact, you've already written it in your comment! If we start to see this get traction more widely, then we can incoporate it into the API. I may not be across the full history and status of material variants, though, so let me know if I'm thinking about it wrong. |
I'm thinking about the convenience for the user, and the stability of the API. Compared to things like the possible
or
Sorry, I derailed a bit while writing this part 🤓 :
I had looked at the As a very high-level thought, this refers to thinks like
Very abstractly speaking: I think that whenever there is a state change, one should consider whether it makes sense to offer the possibiltiy to send out notifications about the state change. In some contexts (mainly UIs), that's simply called "MVC". But it specifically refers to state machines that are implemented as such - e.g. the content loading process, where some For the specific RootTile/Metadata question, one could argue that this can be added later, as in
without causing a breaking change (and even if it's breaking - we all love these But often, the future-ness of some return value is "viral" for the call chain, and many places that are currently doing that So even though the question of whether that method should return a Back to topic...
I have no idea how common this is. But I'd like to avoid issues or forum threads like "Help, my data isn't displayed properly", which involve debugging and requesting the source data, only to see that it uses a To put it that way: Many tilesets that include metadata now are dedicatedly created for testing, and there, it's simpler to just inline the schema. But when people are starting to use and create metadata in a much broader sense, then they will use it in every shape and form that is allowed by the specification (and... beyond that, but that's not our problem right now). (One important use case for "external schemas" will actually be in the context of glTF. When you have a tileset with glTFs with complex metadata, you don't want to repeat the schema in each and every glTF. That's different from the tileset metadata, but the case that people want to share the same 3D Metadata schema between glTFs and a tileset (!) does seem like a very reasonable one).
That's true. And the generic access to metadata is the core of this PR. But at the same time, I assume that we'll have to create some convenience infrastructure for accessing metadata, and semantics in particular. This refers to low-level things like the The
The convenience layer drafted in the In that regard, I'd really like to be able to show how they could implement such a class - but for that, I'd have to show them how to handle
Specifically, for the latter:
I would then include the suggested code block (as a (That's the "viral" part of future-ness...) |
I've made some further improvements:
I don't think I can justify much more time polishing this, but let me know if you have any remaining serious concerns @javagl. |
No further substantial comments from my side here. I tried it out with a 'material variants' example, once with an inlined and once with an external schema, and it seems to work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kring looks good to me! Just caught a few small mistakes, but happy to merge once they're fixed + the other PRs are merged to main
.
REQUIRE(loaderResult.pRootTile->getChildren().size() == 1); | ||
auto pRootTile = &loaderResult.pRootTile->getChildren()[0]; | ||
CHECK(pRootTile->isExternalContent()); | ||
CHECK(pRootTile->getChildren().size() == 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this should be required so pRootTile->getChildren().front();
is valid.
CHECK(pRootTile->getChildren().size() == 1); | |
REQUIRE(pRootTile->getChildren().size() == 1); |
REQUIRE(loaderResult.pRootTile->getChildren().size() == 1); | ||
auto pRootTile = &loaderResult.pRootTile->getChildren()[0]; | ||
CHECK(pRootTile->isExternalContent()); | ||
CHECK(pRootTile->getChildren().size() == 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHECK(pRootTile->getChildren().size() == 1); | |
REQUIRE(pRootTile->getChildren().size() == 1); |
* reject. | ||
* | ||
* @return A shared future that resolves to the loaded metadata. Once this | ||
* future resolves, {@link findMetadata} can be used to synchronously obtain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* future resolves, {@link findMetadata} can be used to synchronously obtain | |
* future resolves, {@link getMetadata} can be used to synchronously obtain |
const std::unique_ptr<TileExternalContent>* pRenderContent = | ||
std::get_if<std::unique_ptr<TileExternalContent>>(&this->_contentKind); | ||
if (pRenderContent) { | ||
return pRenderContent->get(); | ||
} | ||
|
||
return nullptr; | ||
} | ||
|
||
TileExternalContent* TileContent::getExternalContent() noexcept { | ||
std::unique_ptr<TileExternalContent>* pRenderContent = | ||
std::get_if<std::unique_ptr<TileExternalContent>>(&this->_contentKind); | ||
if (pRenderContent) { | ||
return pRenderContent->get(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename pRenderContent
to pExternalContent
for clarity?
const std::unique_ptr<TileExternalContent>* pRenderContent = | |
std::get_if<std::unique_ptr<TileExternalContent>>(&this->_contentKind); | |
if (pRenderContent) { | |
return pRenderContent->get(); | |
} | |
return nullptr; | |
} | |
TileExternalContent* TileContent::getExternalContent() noexcept { | |
std::unique_ptr<TileExternalContent>* pRenderContent = | |
std::get_if<std::unique_ptr<TileExternalContent>>(&this->_contentKind); | |
if (pRenderContent) { | |
return pRenderContent->get(); | |
} | |
const std::unique_ptr<TileExternalContent>* pExternalContent = | |
std::get_if<std::unique_ptr<TileExternalContent>>(&this->_contentKind); | |
if (pExternalContent) { | |
return pExternalContent->get(); | |
} | |
return nullptr; | |
} | |
TileExternalContent* TileContent::getExternalContent() noexcept { | |
std::unique_ptr<TileExternalContent>* pExternalContent = | |
std::get_if<std::unique_ptr<TileExternalContent>>(&this->_contentKind); | |
if (pExternalContent) { | |
return pExternalContent->get(); | |
} | |
Thanks @j9liu! This (and the other PRs it depends on) are ready for another look. |
Thanks @kring ! Merging now. |
This is a PR into #711 so merge that first.
Fixes #676
This PR adds the ability access the
schema
,schemaUri
,groups
, andmetadata
properties in a top-level tileset.json as well as in an external tileset's tileset.json. For the top-level tileset.json, this information is stored on the root tile, so it's not available until the root tile exists. It's captured in a new type calledTilesetMetadata
, and the easiest way to obtain it is from theTileset
:The
findMetadata
method optionally takes aTile
. If specified, the method finds the metadata associated with the external (or top-level) tileset that contains the tile.The metadata properties mentioned above are all available on the
TilesetMetadata
instance via the statically-typed Cesium3DTiles classes, likeSchema
andGroupMetadata
. These types use a lot ofJsonValue
instances, though, by necessity.A new class in
Cesium3DTiles
calledMetadataQuery
makes it easier to find properties by semantic. For example:The same technique can be used to access group variants as well.
The above code samples are adapted from a test in
TestTilesetSelectionAlgorithm.cpp
calledAllows access to material variants
that demonstrates (and tests) the ability to access tileset-level material variant information, based on the example in #676. @javagl would you mind taking a look to see if this meets your expectations?Not handled here: external schemas referenced via schemaUri. Users would have to load them manually.