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

generate-gltf-classes: Request glTF schema files from github instead of embedding as a submodule #108

Closed
kring opened this issue Jan 25, 2021 · 4 comments

Comments

@kring
Copy link
Member

kring commented Jan 25, 2021

The glTF spec repo isn't tiny, and we only need it when generating glTF classes. And even then we only need the JSON schema files., which are very small compared to the whole repo. So a nice improvement (originally suggested by @javagl in #106) would be to request the schema files from github as needed, i.e. in SchemaCache.js instead of forcing users to download the whole glTF repo as a submodule.

@javagl's suggested code in SchemaCache.js:

var request = require('sync-request');
...
var res = request('GET', new URL(name, this.schemaPath).href);
const result = JSON.parse(res.getBody(), "utf-8");

And in package.json:

"generate": "node index.js --schema https://raw.githubusercontent.com/KhronosGroup/glTF/master/specification/2.0/schema/ --output ../../CesiumGltf --readerOutput ../../CesiumGltfReader --extensions https://raw.githubusercontent.com/KhronosGroup/glTF/master/extensions/2.0/ --config glTF.json"
@javagl
Copy link
Contributor

javagl commented Jan 25, 2021

Although sync-request loudly says "You should not be using this in a production application", I think it's OK for a command-line tool. (Usually, I'd even take it one step further, and end up with some jsonSource.get(this.schemaPath, name) function that can either be a filesystem based one, or a sync-request one, but that's not important).

I already did that/tried out that download approach locally, it's only a small change. I can create a PR for that, but would probably do that after the main PR is merged. (Or should I just plug it on top of the gltf branch?)

@javagl
Copy link
Contributor

javagl commented Jan 26, 2021

I have added this for now in https://github.com/CesiumGS/cesium-native/tree/gltf-from-online-schema

Depending on whether the "base paths" that are given in the package.json start with "http", it either fetches the schema files from a URL or from a file. Will create a PR when the gltf branch is done.

@kring
Copy link
Member Author

kring commented Jan 27, 2021

I think it's OK for a command-line tool.

yep agreed 👍

I already did that/tried out that download approach locally, it's only a small change. I can create a PR for that, but would probably do that after the main PR is merged. (Or should I just plug it on top of the gltf branch?)

Great! I'm happy with either approach - PR into gltf or wait until it's merged into master.

@javagl
Copy link
Contributor

javagl commented Feb 23, 2021

Fixed via #121

The schema is now downloaded from the Khronos repo by default, but it's also possible to change the input to be local files (this can be done in the package.json, but without any actual code changes).

@javagl javagl closed this as completed Feb 23, 2021
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

No branches or pull requests

2 participants