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

Limit number of glTF validation issues #291

Merged
merged 3 commits into from
Nov 9, 2023
Merged

Limit number of glTF validation issues #291

merged 3 commits into from
Nov 9, 2023

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Nov 8, 2023

Addresses #290

glTF tile content is validated with the glTF-Validator. When validating a glTF asset that contains many errors, this can lead to an out-of-memory error. (For example, the GLB in the linked issue would generate roughly 6 million issues...).

The glTF validator offers an option to limit the number of issues that are reported. It's hard to pick a "universally sensible" value for that.

(One critical corner case could be that when the limit is set to x, then there may be an asset that generates x warnings or infos, and the x+1th issue would be an 'error', leading to the asset counting as 'valid' even though it isn't...)

This PR just adds a fixed maxIssues: 1000 for now, which prevents the issue. Questions beyond that are:

  • How many issues should be the default?
  • Should this be configurable from the 3d-tiles-validator side (via validation options in a config file)?
    • (If yes, some thought would have to go into that. It could be a maxContentIssues for all types of content, but only the glTF Validator currently supports such a 'limit'...)

@javagl
Copy link
Contributor Author

javagl commented Nov 9, 2023

There is now an upper limit for the number of issues that are reported by the glTF-Validator.

This limit is 1000 for now, but this not specified. The point is: When a glTF asset generates 1000 issues, then this is a single issue, namely, 'the glTF asset being 'bad''.

If necessary, that limit could be made configurable (via the validator config JSON file). But it's unlikely that an end-user would like to change this value, say, from 1000 to 1200. If necessary, it can always be made configurable later.

@javagl javagl marked this pull request as ready for review November 9, 2023 16:15
@javagl javagl linked an issue Nov 9, 2023 that may be closed by this pull request
@lilleyse lilleyse merged commit 5656f67 into main Nov 9, 2023
2 checks passed
@lilleyse lilleyse deleted the limit-gltf-issues branch November 9, 2023 16:19
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.

Out of memory crash
2 participants