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

The textureInfo is not linked from pbrMetallicRoughness #1013

Closed
javagl opened this issue Jun 12, 2017 · 16 comments
Closed

The textureInfo is not linked from pbrMetallicRoughness #1013

javagl opened this issue Jun 12, 2017 · 16 comments
Assignees

Comments

@javagl
Copy link
Contributor

javagl commented Jun 12, 2017

The pbrMetallicRoughness currently refers to baseColorTexture and metallicRoughnessTexture as being of type object. In fact, they are of type textureInfo.

Is this just a minor glitch in the generator? It would be nice if the type could be defined to be textureInfo, including the appropriate link. (There is a section for the textureInfo, but it is not really used (i.e. linked to) right now...)

@pjcozzi
Copy link
Member

pjcozzi commented Oct 19, 2017

Discussed offline. @javagl will see if he can fix wetzel to make the link work.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 24, 2017

@javagl bump for if/when you have bandwidth.

@emackey
Copy link
Member

emackey commented Jan 1, 2018

Yesterday I was looking at the diffs between what wetzel puts out and what's embedded in the current 2.0 README. The biggest differences appeared to be:

  • A large number of lowercase titles became capitalized (for example, bufferView became Buffer View)
  • The order has changed, perhaps unintentionally, making some diffs impossible to do in-place
  • There do appear to be a number of hand-edits that need reconciliation
  • The "child of glTF root" thing appeared, but I think I'm just missing the command-line switch to turn that one off.

This is the commandline I was using:

$ bin/wetzel.js -l2 --schemaPath=schema -a=cqo ../glTF/specification/2.0/schema/glTF.schema.json > ../wetzel_output/gltf.md

@javagl
Copy link
Contributor Author

javagl commented May 11, 2018

@emackey The capitalization of the titles was done in the schema. Unfortunately, this caused some ambiguities and quirks in wetzel: Before this change, the title of a schema was analogous to a "type name", and was used as such and referred to as such. This included several formatting routines, where the title was passed in "as-if" it was a type name, causing inconsistent links etc.

However, I tried to handle this in this wetzel commit: javagl/wetzel@0f7eb03

The result of running this over the current spec is shown here:

https://github.com/javagl/glTF/tree/master/specification/2.0#objects

The links seem to work, and the formatting seems to be OK.

I'm a bit hesitant to create the PRs:

  • for the wetzel fixes, someone should review them. I'm pretty sure that one could make a somewhat larger-scale cleanup regarding the comments (what types and structures the objects have that are passed around there - I added some comments, but...)
  • for the README fixes: This might cause some headaches downstream. There probably already are many links out there that lead to the "anchors", which might have to be reviewed and updated (most prominently, the glTF-Tutorial: It contains many links directly to the respective types)

@lexaknyazev
Copy link
Member

@javagl
Can we merge the updated properties reference to the main repo?

@javagl
Copy link
Contributor Author

javagl commented Nov 24, 2018

Someone should probably still review the wetzel commit in javagl/wetzel@0f7eb03 , and iff it is merged, I'd re-run the generation based on the current state. It has been a while since the output from the link above has been generated, and I think there is no point in merging a "volatile" or "preliminary" state into the main repo. This should only be done with a state that can reliably and easily be reproduced.

@emackey
Copy link
Member

emackey commented Nov 25, 2018

@javagl To get that reviewed, please sign the CLA (can be done via email) and submit a Pull Request for it. Thanks!

@sbtron
Copy link
Contributor

sbtron commented Jan 23, 2019

@javagl could you open a PR to wetzel for your changes.

Thanks

@javagl
Copy link
Contributor Author

javagl commented Jan 24, 2019

@sbtron This CLA thing is/was an impediment here. There are waaay to many lawyers out there. But I just noticed that it says "Sign by typing your full name." - does that really mean that I do not have to print, sign and scan the whole thing, but just write an email with a TEXT file attached? (If so, I'd do it. There is no way that has any legal meaning anyhow...)

@emackey
Copy link
Member

emackey commented Jan 25, 2019

@javagl Yes, for AGI's CLA you can just send an email with a text file attached, no scanning required.

There is no way that has any legal meaning anyhow

I'm not a lawyer, but I certainly hope your assessment of that is incorrect. The CLA isn't there just to protect AGI's interests, it protects individual contributors by allowing them to keep their own copyrights while contributing code, and protecting contributors from legal action from users. It also protects end users, by ensuring that all contributions were explicitly made with the intention of being provided to users under the project's licence terms. The CLA we use comes from the Apache Foundation and is fairly standard practice in the open source industry. Signing it once allows contributions to all AGI projects, including Wetzel, Cesium, the glTF-VSCode extension, and many others.

@javagl
Copy link
Contributor Author

javagl commented Feb 1, 2019

The PR for wetzel is at CesiumGS/wetzel#33

The output with the latest schema is shown at https://github.com/javagl/glTF/tree/reference_links_update/specification/2.0#objects

@javagl
Copy link
Contributor Author

javagl commented Feb 7, 2019

The PR for wetzel has been merged, but for the reasons mentioned in CesiumGS/wetzel#33 and the comment in CesiumGS/wetzel#33 (review) I hesitate to create a PR for the actual update of the README.

The fact that the order of some properties has changed in the output is technically not an issue, but makes a 1:1 comparison difficult. Beyond that, some of the #anchor links changed, and this is critical.

Therefore, a link to the the comparison here: master...javagl:reference_links_update for others to have another look at it.

Beyond that:

One could probably automate some processes here, but I'm not a big fan of establishing certain infrastructures that are hard to maintain - particularly when they'd be triggered only rarely (in the case of changes to the JSON spec). But maybe we could establish some "guideline"/"rule of thumb": Whenever there is a change to the JSON schema, the corresponding output of wetzel should be part of the PR. (Possibly even with a tagged version/state of wetzel?). Otherwise, there's always the danger that the JSON schema, the wetzel output and the final README diverge.

@lexaknyazev
Copy link
Member

I'd suggest moving forward with a more automated approach:

  • the markdown file contains neither "Property Reference" section nor last update timestamp;
  • on each commit to the master branch, CI produces a new build with the last git timestamp and updated "Property Reference";
  • built spec is published to gh-pages branch.

This workflow would increase documentation quality without adding extra maintenance burden on contributors.

@lexaknyazev
Copy link
Member

@javagl what's the latest here? Should we close this issue and open a new one about automating spec updates?

@javagl
Copy link
Contributor Author

javagl commented May 8, 2019

I have opened the related issue about possibly using jsonschema2md at #1607

I'm not sure in how far the outcome there will affect the setup of an automatic spec generation. (In both cases, it's just a node application that is run).

One of the latest outputs of wetzel is currently shown at https://github.com/javagl/glTF/tree/update-properties-from-schema/specification/2.0#properties-reference (note that the links there still have the wrong anchors, but that should already be fixed - I can check this again if the discussion in 1607 converges towards wetzel)

I'd still leave this one open until a decision is made and the textureInfo links are working properly.

@emackey
Copy link
Member

emackey commented Sep 23, 2021

The new AsciiDoc spec is using wetzel and GitHub CI for auto-generation. The texture objects mentioned in the original issue here are correctly linked now.

@emackey emackey closed this as completed Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants