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

Initial draft of 3D Tiles 1.1 validator #222

Merged
merged 99 commits into from
Oct 3, 2022
Merged

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Sep 21, 2022

This is an implementation of a validator for 3D Tiles 1.1. This new state has been developed in an internal repository, and is now moved here as a PR for first reviews. This state is supposed to replace the original validator. But it is not supposed to be merged before the tools and samples-generator projects from this repository have been moved into new repositories.

The README.md shows the basic usage, based on the current state. There are some discussion points in IMPLEMENTATION.md that will be addressed soon.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some really early feedback that's usage related. Haven't dived into the code yet.

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
IMPLEMENTATION.md Outdated Show resolved Hide resolved
src/main.ts Show resolved Hide resolved
src/structure/BoundingVolume.ts Show resolved Hide resolved
IMPLEMENTATION.md Show resolved Hide resolved
.gitignore Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
src/implicitTilingDemos.ts Show resolved Hide resolved
src/issues/JsonValidationIssues.ts Outdated Show resolved Hide resolved
src/tileFormats/legacy/defaultValue.js Outdated Show resolved Hide resolved
src/tileFormats/legacy/defined.js Outdated Show resolved Hide resolved
src/tileFormats/legacy/utility.js Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

Ok I dived into the code... but just barely. It looks good from what I've seen.

Some other validation checks that I didn't see in the code right away (correct me if wrong) include:

  • Check that geometry is within tile.boundingVolume, content.boundingVolume, and parent.boundingVolume
  • Check that metadata statistics are correct. Or at the very least, check that metadata values fall within min/max
  • Check that metadata values fall within the class's min/max
  • Declarative styling validation

These are somewhat more involved and don't necessarily need to hold up this PR. I'm sure others will come up over time.

@javagl
Copy link
Contributor Author

javagl commented Sep 22, 2022

The points from the previous comment have been added as desired features in the implementation notes.

A few more general notes, related to the inlined comments:


Some of the comments referred to areas that have not yet completely be sorted out. For example, I consider linting+prettier to be related to (or even part of) the broader topic of building/deployment/CI, and this will also include discussions about configuration details, build scripts, unit tests, and even things like coverage analysis.

For linting, the presence of legacy (.js) fragments and the bazillion configuration options for ESLint alone, and particularly in combination with Typescipt, lead me to the point where I'd rather ask: "How should this be done?". I have seen that there is https://github.com/CesiumGS/eslint-config-cesium , and tried to "use" that, pragmatically, but it does not seem to work perfectly with TypeScript.
(It's always annoying to see the linter and the build process disagree, and dive into the question of whether both can be satisfied by some magic configuration setting that cannot sensibly and deterministically (!) be found by reading the documentation, but only by searching for error messages...)

For formatting, I used the formatting function of VSCode. If there is a particular prettier version and configuration that should be used, then I'd add this. Otherwise, I'll have a closer look at existing configurations at Cesium, and how well they work for TypeScript...


Duplicate with other defaultValue.js?
Duplicate with other defined.js?

The other ones are not .js, but .ts. Getting the mix of import/export/module/require right across .js and .ts is apparently not as trivial as it should be (particularly, when some linter complains about one or the other...). These files are in the legacy directory, and only used there. On the one hand, it's not pretty. On the other hand, I think that it's not worth spending too much time there (similarly, with trying to lint some of the legacy code - I just disabled eslint for these files now). But if this is considered to be important, I can have another look. (Or I could just "port" the code to TypeScript. That might, in fact, be easier than working around the inconsistencies and glitches of such a mixed environment...)


What are implicitTilingDemos.ts and metadataDemos.ts used for? Is it for internal development?

These are very basic tests of the functionalities of the classes in the respective packages. They are intended to answer questions like "Imagine I had a metadataEntity - how would I like to use it?", or "What code do I want to write when I want to traverse a tileset?".

The functions there could eventually become @example fragments of the documentation, and be the "templates" based on which proper unit tests for the final API are built.

@lilleyse
Copy link
Contributor

lilleyse commented Sep 22, 2022

You should be able to use the latest prettier version. CesiumJS uses 2.1.2 only because that was the latest version at the time prettier was added. A different, internal repo uses 2.6.2. The important thing is that it's pinned to a specific version so that people don't get different formatting depending on which version of prettier is installed.

Also CesiumJS uses the default configuration I believe.

@lilleyse
Copy link
Contributor

(Or I could just "port" the code to TypeScript. That might, in fact, be easier than working around the inconsistencies and glitches of such a mixed environment...)

Yeah, might be worth doing. It's just a few small files.

@lilleyse
Copy link
Contributor

Once the repo situation is sorted out it would be good to open separate issues for the discussion points in IMPLEMENTATION.md

A VERY pragmatic error handling for now. The handling
of non-resolvable resources has to be sorted out.
The error handling is VERY pragmatic. The general
handling of missing resources has to be sorted out.
@javagl javagl marked this pull request as ready for review October 3, 2022 18:02
@lilleyse
Copy link
Contributor

lilleyse commented Oct 3, 2022

@javagl could you add ThirdParty.json to this branch?

It may require adding a gulp script. See #219

@lilleyse
Copy link
Contributor

lilleyse commented Oct 3, 2022

In LICENSE.md this sentence can be removed:

All the tools are licensed with the Apache 2.0 license. Check the LICENSE.md in each directory for more information on third-party libraries.

Also can you update the copyright date?

Copyright 2016-2020 CesiumGS, Inc. and Contributors

@javagl
Copy link
Contributor Author

javagl commented Oct 3, 2022

Merging this PR will fix some issues, or make them obsolete:

@javagl
Copy link
Contributor Author

javagl commented Oct 3, 2022

The license file has been updated.

The ThirdParty.json has been added. The functionality for creating this does not seem to require gulp. Maybe gulp will be necessary, at some point, for coverage checks or whatnot. But until then, having that functionality as a plain .js file may be OK.

@lilleyse
Copy link
Contributor

lilleyse commented Oct 3, 2022

I did a few final validation checks. Looks good!

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