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

Add URI test assets #157

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hybridherbst
Copy link
Contributor

This PR adds various test assets for URIs after I noticed that the allowed formats in the glTF spec include percentage encoding, JSON-string escaping and regular UTF8 strings.

The test assets currently break when dropped in almost all implementations, including the Khronos Sample Viewer. The glTF Validator is able to validate most of them as correct, but not all of them (URIs traversing into parent directories can't be validated it seems).

I included ZIP files for convenient download for testing.

@lexaknyazev
Copy link
Member

Thanks for the tests. Although it's a very useful set of assets, it needs a bit more polish.

  • Zip files shouldn't be checked-in. For temporary testing, they could be attached to GitHub posts via drag-n-drop.
  • The assets produce a fair number of unrelated validation messages, e.g., unreferenced nodes and accessors.
  • Some variant names do not match the content. For example, glTF-UriTest-03-RelativeBin has both the texture and the bin files in subdirectories. Consider either further splitting these assets so that each asset would focus on one specific case or regrouping them to be more consistent.

The glTF Validator is able to validate most of them as correct, but not all of them (URIs traversing into parent directories can't be validated it seems).

There are several Validator builds (web drag-n-drop, npm, and native) and resource access may be platform-dependent.

Quick testing results:

Native

  • All assets pass validation with native executables on macOS and Windows.
  • glTF-UriTest-02-RelativeTexture-UTF8 and glTF-UriTest-03-RelativeBin fail validation with a native executable on Linux (as seen on CI bots), I'll look into that.

Web drag-n-drop

  • glTF-UriTest-03-RelativeBin fails with the drag-n-drop tool, I'll look into that too.
  • Parent directory access support isn't planned for the drag-n-drop tool, so glTF-UriTest-04-Nested-NoUTF8 and glTF-UriTest-05-Nested-UTF8 don't pass there by design. By the way, this is another example of suboptimal test case grouping - these assets are supposed to test resources in parent directories but also they include absolute URIs.

NPM (as in VS Code)

  • glTF-UriTest-04-Nested-NoUTF8 and glTF-UriTest-05-Nested-UTF8 produce bogus errors due to VS Code plugin mishandling absolute URIs. /cc @emackey

@javagl
Copy link
Contributor

javagl commented Nov 25, 2024

A clearer description of each asset would be nice, including the name and what exacty the asset tests or demonstrates, e.g.

  • glTF-UriTest-03-RelativeBin: Contains relative references to the .bin file and texture that are contained in subfolders
    ...

The naming of the files is somewhat inconsistent. We should consider using the same naming conventions as for the other models, even when the actual assets are contained in a subfolder. For example, the .gltf file in glTF-UriTest-03-RelativeBin is called RelativeResourcePaths.gltf - maybe it should be called glTF-UriTest-03-RelativeBin.gltf for consistency...?

A consistency nitpick: The other file names. There are Buffer.bin and buffer.bin, and once a RelativeTexturePaths.bin...

An aside: I think that this is the only model so far where the actual models are contained in subfolders. In some way this is similar to the "Compare..." assets that have been added via #136 , and where one could make a case for "grouping" them. But here, having them in one "top-level" directory probably makes sense. ....🤞 hoping that this for itself doesn't cause trouble for the CI Python scripts and generated pages.

While I generally see the point of using non-ASCII characters (to give some sense to the different forms of encoding them in the URIs), I wonder whether the names really have to be नमस्ते, दुनिया/texture/ChildFolder 你好,世界.jpg, or whether something like example/texture/childFolder ①.jpg wouldn't be sufficient for that.

EDIT:

The textures would be smaller if they were stored as PNG. (Not a big deal, though...)

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.

3 participants