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

Material variants support #693

Closed
wants to merge 1 commit into from
Closed

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Jul 24, 2023

Addressing #676

Maybe it is possible to split the conceptual questions from some lower-level questions. I'll add a comment with some conceptual questions in the issue.

The lower-level questions:

I tried to find the right place for extracting the (metadata) information from the tileset JSON, and pass it on to the Tileset class. The right place for parsing it may be a bit arbitrary. Since this is currently solely operating on the rapidjson document for the tileset, the magic
MaterialVariants parseMaterialVariants(const rapidjson::Document& tileset)
function can basically be inserted anywhere (currently in an anonymous namespace of the TilesetContentManager). But... this has to be changed for the case that the metadata schema is not contained in the tileset JSON, but referred to via a schemaUri. This is related the the question about the "proper way" for passing this information from the place where it is loaded (or where it has to be loaded in the future) to the Tileset object. Having short descriptions about what a TilesetContentManager actually is (beyond /* A class that manages tileset content */), or the role that a TilesetContentLoaderResult plays (beyond that it is /* The result of loading tileset content */) could be really, really, really helpful.

@javagl
Copy link
Contributor Author

javagl commented Jul 25, 2023

Even though this is only an early draft, it could be good if someone (@kring !?) could have a quick look whether the overall approach is not entirely wrong:

  • There is a plain struct

    struct MaterialVariants {
      std::vector<std::string> tilesetMaterialVariantNames;
      std::vector<std::vector<std::string>> groupsMaterialVariantNames;
    };
    

    Some details TBD here. The main point for now is: It is supposed to be a simple structure that offers a bunch of strings and string arrays that are extracted from metadata

  • An instance of this is offered by the Cesium3DTilesSelection::Tileset class

  • The data is read, directly from the tileset.json, by the TilesetContentManager class - here (in the part that is 'hidden' by GitHub)

  • It is stored in the TilesetContentLoaderResult, which appears to be the only way to 'transport' this information back to the caller (which has to happen on the main thread, via propagateTilesetContentLoaderResult)


One open question is:

The schema is currently assumed to be in the tileset.json. But the tileset.json might also contain a schemaUri, in which case the schema has to be resolved from an external file. I assume that this will involve some juggling with TilesetJsonLoader and the surrounding infrastructure (createLoader, addChildLoader etc...), and I'm not sure whether this conflicts with the current approach of using the TilesetContentManager to "transport" the data that is loaded back to the Tileset instance.


Longer-term points that are related to this PR:

  • The TilesetJsonLoader could eventually be replaced by the Cesium3DTilesLoader. But for now, it is pretty specific and deeply entangled in the loader infrastructure, and can probably not easily be replaced
  • This PR only adds easy, top-level access to the MaterialVariants data. This data is extracted from the metadata. Thus, it should be considered only as a "convenience functionality" that is offered on top of whatever metadata handling classes will exist in the future. Longer term, there might be a Cesium3DMetadata library that contains the auto-generated classes that currently exist only for glTF, but which will also have to exist for 3D Tiles at some point. But regardless of what happens there, "under the hood", the MaterialVariants could still be exposed in the form that is drafted here, with no changes for the clients that consume it.

@cesium-concierge
Copy link

Thanks again for your contribution @javagl!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@javagl
Copy link
Contributor Author

javagl commented Aug 28, 2023

This PR itself has basically become obsolete with #709 : That PR adds generic metadata access. The structures there allow accessing the metadata/schema/class/properties. Based on that, an example of how the material variants could be accessed is shown here:

struct MaterialVariants {
  std::vector<std::string> tilesetMaterialVariantNames;
  std::vector<std::vector<std::string>> groupsMaterialVariantNames;
};

class MaterialVariantsUtilities {

public:
  static CesiumAsync::Future<MaterialVariants>
  fromTileset(Cesium3DTilesSelection::Tileset &tileset) {
    return tileset.loadMetadata().thenImmediately(
        [](auto &&pMetadata) -> MaterialVariants {
          MaterialVariants materialVariants;
          materialVariants.tilesetMaterialVariantNames =
              MaterialVariantsUtilities::findStringPropertyValues(
                  *pMetadata->schema, *pMetadata->metadata,
                  "MATERIAL_VARIANTS");
          for (const Cesium3DTiles::GroupMetadata &group : pMetadata->groups) {

            materialVariants.groupsMaterialVariantNames.push_back(
                MaterialVariantsUtilities::findStringPropertyValues(
                    *pMetadata->schema, group, "MATERIAL_VARIANTS"));
          }
          return materialVariants;
        });
  }

  static void debugPrint(const MaterialVariants &materialVariants) {
    std::cout << "Material variants:" << std::endl;
    std::cout << "  Tileset:" << std::endl;
    const auto &t = materialVariants.tilesetMaterialVariantNames;
    for (size_t i = 0; i < t.size(); i++) {
      std::cout << "    " << t[i] << std::endl;
    }
    std::cout << "  Groups:" << std::endl;
    const auto &gs = materialVariants.groupsMaterialVariantNames;
    for (size_t i = 0; i < gs.size(); i++) {
      std::cout << "  Group " << i << ":" << std::endl;
      const auto &g = gs[i];
      for (size_t j = 0; j < g.size(); j++) {
        std::cout << "    " << g[j] << std::endl;
      }
    }
  }

private:
  static std::vector<std::string>
  findStringPropertyValues(const Cesium3DTiles::Schema &schema,
                           const Cesium3DTiles::MetadataEntity &metadataEntity,
                           const std::string &semantic) {
    std::optional<Cesium3DTiles::FoundMetadataProperty> propertiesWithSemantic =
        Cesium3DTiles::MetadataQuery::findFirstPropertyWithSemantic(
            schema, metadataEntity, semantic);

    const CesiumUtility::JsonValue::Array &propertiesJson =
        propertiesWithSemantic->propertyValue.getArray();
    std::vector<std::string> propertyValues(propertiesJson.size());
    std::transform(propertiesJson.begin(), propertiesJson.end(),
                   propertyValues.begin(),
                   [](const CesiumUtility::JsonValue &value) {
                     return value.getStringOrDefault("");
                   });
    return propertyValues;
  }
};

If someone has ideas about how to let the code in the fromTileset function look a little bit more like

Future<MaterialVariants> fromTileset(...) {
    auto&& future = tileset.loadMetadata();
    return future.thenImmediately(??? MaterialVariantsUtilities::extract ???);
}
...
private: 
    static MaterialVariants extract(TilesetMetadata *metadata) { ... }

then feel free to chime in here. (I'm not sure whether the template infrastructure of the async system even allows that).

In any case, with this utility class, the material variants information can be obtained from a tileset like that:

      MaterialVariantsUtilities::fromTileset(tileset).thenImmediately(
          [](MaterialVariants &&materialVariants) -> void {
            MaterialVariantsUtilities::debugPrint(materialVariants);
          });

(with the caveat that one still has to call some functions that internally cause the pending tasks to be processed and the future to actually be resolved. Maybe examples for this can eventually be derived from the test code that will be added in the context of the other PR).

@javagl javagl closed this Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants