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

Drafts for extensions validation #230

Merged
merged 23 commits into from
Oct 14, 2022
Merged

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Oct 10, 2022

Addressing #227 : This PR adds a mechanism for adding validators for extensions. Some technical details are described in the issue, and still have to be resolved here.

This is only a basic draft, with support for the 3DTILES_bounding_volume_S2 extension: By default, a tileset with S2 bounding volumes would cause a validation issue, because the bounding volume neither has neither box, region, nor sphere. But with the extensions validation support, it would be possible to register a validator for S2 bounding volumes. The registration is currently drafted like this:

const s2Validator = new BoundingVolumeS2Validator("");
const performDefaultValidation = false;
ExtensionsValidator.register("3DTILES_bounding_volume_S2", s2Validator, performDefaultValidation);

And afterwards, the original issue would no longer be reported. Instead, it would report validation issues for the S2 bounding volumes accordingly - like S2_TOKEN_INVALID or so...

Adding a general support for extensions will require some refactorings of the current validator state. These refactorings should be "small" in terms of the amount of code that has to be changed, but they may affect the interfaces, and therefore several parts of the code. Further technical details will be added here soon.

@javagl
Copy link
Contributor Author

javagl commented Oct 11, 2022

A short brain dump:

Some questions for the validation of extensions are not trivial to answer, and the S2 bounding volume extension (fortunately?) shows some of them.

The initial approach sketched above was:

  • Register a new validator for the S2 extension
  • When a bounding volume with S2 extension was found:
    • Skip the default validation of the bounding volume
    • Validate the 3DTILES_bounding_volume_S2 object instead

This does not sensibly cover the case that there is a bounding volume that has the S2 bounding volume and a box, for example. In this case, the validation of the box should still be peformed. More specifically, in the case of S2, the pseudocode for the validation would be

if (!has(boundingVolume, "S2")) {
    validateAnyOf("box", "region", "sphere");
}
validateBox();
validateRegion();
validateSphere();

I.e. it should only skip the anyOf check, but still validate everything else.

More generically, it's hard or impossible to define exactly which aspect of the default validation should be omitted due to the presence of an extension.

One "obvious" way of tackling this is:

  • Register a validator for the S2 extension
  • When a bounding volume with S2 extension is found:
    • Use the registered validator for the validation of the bounding volume. This registered validator would do the same as the default validator, except for the anyOf check, and additionally validate the 3DTILES_bounding_volume_S2 object

This still raises questions for the more general case, or for future extensions. For example, consider a future 3DTILES_bounding_volume_KDOP extension (something that I'd like to see one day...). The process would then be

  • Register a validator for the S2 extension
  • Register a validator for the KDOP extension
  • When a bounding volume with S2 and the KDOP extension is found:
    • This may cause duplicate issues, because the validation of the box is performed twice as part of the default behavior

I'd like to avoid constraining what extensions are allowed to do - except for that extensions may not cause a valid object to become invalid. But considering all possible ways of how an extension may cause an (otherwise) invalid object to become valid (and how this affects the overall validation process) is not entirely trivial.

I'll update this PR soon, but wanted to sort my thoughts here.

@javagl
Copy link
Contributor Author

javagl commented Oct 13, 2022

The current state contains a basic implementation of a plugin concept for validating extensions, and various cleanups and generalizations for further working towards that goal.

There are two high-level points that are part of this PR:


It is possible to register a Validator for objects that contain certain extensions:

      const s2Validator = new BoundingVolumeS2Validator();
      const override = true;
      ExtendedObjectsValidators.register(
        "3DTILES_bounding_volume_S2",
        s2Validator,
        override
      );

(Note that this is a validator for the objects that contain the extensions. and not for the extension objects themself - although the validation of these objects should be part of the registered validator, of course).

Whenever the validator encounters an object that may contain extensions, it passes it to the ExtendedObjectsValidators class. This class will check whether the object contains an extension for which a new validator was registered, and applies this validator accordingly. When the override flag was true, calling this validator will cause the default validation to be skipped.

One example of such a validator is the BoundingVolumeS2Validator. This validator performs the basic validation of a single 3DTILES_bounding_volume_S2 object. The way how this validator is integrated is shown in the BoundingVolumeValidator

Note: This sort of validation has a scope that is still too narrow. There are some forms of extensions and associated validation that can not be covered with this approach:

  • The validation of a single S2 bounding volume can not validate any form of consistency of the S2 hierarchy. This part will require some form of generalization, which roughly means: Finding the right place to "hook in" something like a Validator<TwoBoundingVolumes> that will be called for parent+child and/or tile+content bounding volumes. Eventually, this could replace the legacy BoundingVolumeChecks.
  • There are certain extensions that do not require an explicit validation of one object. For example, an extension that allows additional content types, like 3DTILES_content_glTF. Such an extension can affect the validity of a tileset as a whole, referring to places that are deeply hidden and unrelated to the point where the extension is declared.

The last point (namely, extensions that refer to content types) was the reason for another generalization that is applied in this PR. Until now, the ContentDataValidator contained a few hard-wired, specific checks for certain content data types. The necessity for generalizing this was obvious, and is therefore addressed here. It is not yet fully fleshed out, but should already be a reasonable step forward.

There now is a ContentDataValidators class (note the s - it might be renamed to ContentDataValidatorRegistry or so...). This class allows registering Validator instances that are used for different content types, based on different criteria. These critera may be the magic header, or the file extension, or vague things like ~"something is probably a tileset"...

An example for how the current set of content data validators is registered is shown in the current state of the registerDefaults method.

The actual ContentDataValidator will then validate the content data: It will look up the Validator that is supposed to be used for the given content data, and apply it to the content data (as shown here).

Note: There are still a few questions that are not fully resolved. One question is about the connection between 1. an extension that allows a new content type and 2. the actual validator for that new content type.

Roughly:

  • Where and how should an extension for 3DTILES_content_XYZ be registered, alongside the XYZValidator?
  • Related to Clarify extension usage validation #231 : How to detect whether the extension is used?
  • (Important): How to encode the criterion for determining whether a certain content actually does have the type "XYZ"?

The latter is particularly difficult for JSON-based content types. Iff we already had 3D Tiles 2.0, then a valid 3D Tiles 2.0 file (with asset.version = "2.0") would also be a valid glTF asset 😬 . So this has to involve some guesses. (The file extension may be a hint, but the extension is not required...).

@javagl
Copy link
Contributor Author

javagl commented Oct 14, 2022

A basic infrastructure for the validation for extended objects (and extensions) is added with this PR. There are still some open questions about the interface of how certain types of extension validators will be added or registered.

Edit: These questions are now summarized in #227 (comment)

@javagl javagl marked this pull request as ready for review October 14, 2022 17:18
@lilleyse lilleyse merged commit dbfbcee into CesiumGS:main Oct 14, 2022
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.

None yet

2 participants