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

Validation of EXT_instance_features #284

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Oct 11, 2023

This builds upon #280

It adds functionality for validating EXT_instance_features. There is a considerable overlap on a low technical level: For EXT_mesh_features and EXT_instance_features, the validator has to validate "feature IDs and their values". The main difference is...

  • for EXT_mesh_features, the feature ID attribute refers to the mesh primitive attributes
  • for EXT_instance_features, the feature ID attribute refers to the EXT_mesh_gpu_instancing attributes
  • for EXT_instance_features, the feature ID can not refer to feature ID textures

The current state here is to "carve out" the validation of feature IDs so that most of the code can be shared for the validation of both extensions.

It does raise some questions for the unit tests. Most of the feature ID validation should already be covered with tests for the EXT_mesh_features part. There's probably no need to add dozens of files for the EXT_instance_features validation that are invalid in the same way. At some point, this will touch #226 and raise the question of whether all unit tests should be done based on complete files, or whether some of them should be broken down to the lower ("classical") level of specs that check the behavior of individual functions. So instead of having a bunch of

// These are essentially checking the same error...
runSpec(    'meshFeaturesAttributeInvalidType.gltf);
runSpec('instanceFeaturesAttributeInvalidType.gltf);

one could have a single

const featureId = { 
  attribute: "NOT_A_NUMBER"
}
runSpec(FeatureIdValidation.validate(featureId));

to check the same thing on a different level. But (for now) I still like the "holistic" approach of using complete files, and deciding on the level and granularity for the other approach could be tricky.

Base automatically changed from gltf-extension-validation to main October 16, 2023 13:47
@javagl javagl marked this pull request as ready for review October 16, 2023 14:23
@lilleyse lilleyse merged commit 11709b4 into main Oct 16, 2023
2 checks passed
@lilleyse lilleyse deleted the gltf-extension-validation-instance-features branch October 16, 2023 14:45
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