Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Optimized 2CylinderEngine and added all variants #4

Closed
wants to merge 1 commit into from
Closed

Optimized 2CylinderEngine and added all variants #4

wants to merge 1 commit into from

Conversation

lasalvavida
Copy link
Contributor

No description provided.

@pjcozzi
Copy link
Member

pjcozzi commented Nov 23, 2016

  • Can we move glTF-KHR_binary_glTF/embedded to just a single file in glTF-KHR_binary_glTF? Otherwise, it is confusing making it seem like the binary data is base64-encoded like the other embedded sub-directories.
  • Are we sure that we need both embedded and separate directories for glTF-KHR_materials_common and glTF-WEB3D_quantized_attributes directories? Is there diminishing returns here? I recall the request for the separate version of glTF-KHR_binary_glTF, but can you point me to the discussion on providing separate/embedded for all variants?
  • In the main README.md, can we include instructions on using gltf-pipeline for generating all the model variants?

@lasalvavida
Copy link
Contributor Author

Can we move glTF-KHR_binary_glTF/embedded to just a single file in glTF-KHR_binary_glTF? Otherwise, it is confusing making it seem like the binary data is base64-encoded like the other embedded sub-directories.

I agree, will make this change.

Are we sure that we need both embedded and separate directories for glTF-KHR_materials_common and glTF-WEB3D_quantized_attributes directories? Is there diminishing returns here? I recall the request for the separate version of glTF-KHR_binary_glTF, but can you point me to the discussion on providing separate/embedded for all variants?

This was an offline discussion that we had in September I think. The justification was readability and usability. The separate sample models are much more readable for exploring and understanding the structure of glTF, while the embedded ones are more usable since they are contained in a single file and have no external dependencies.

In the main README.md, can we include instructions on using gltf-pipeline for generating all the model variants?

Absolutely

@pjcozzi
Copy link
Member

pjcozzi commented Nov 30, 2016

Are we sure that we need both embedded and separate directories for glTF-KHR_materials_common and glTF-WEB3D_quantized_attributes directories? Is there diminishing returns here? I recall the request for the separate version of glTF-KHR_binary_glTF, but can you point me to the discussion on providing separate/embedded for all variants?

This was an offline discussion that we had in September I think. The justification was readability and usability. The separate sample models are much more readable for exploring and understanding the structure of glTF, while the embedded ones are more usable since they are contained in a single file and have no external dependencies.

Given that the base glTF format has the readable, isn't there diminishing having separate/embedded for each variation? I think it is fine for the base format and binary format because it helps engines cover all their cases, e.g., external references in binary glTF (which there was an issue with in three.js before we had sample models). I'm just trying to understand if having two versions of glTF-KHR_materials_common and glTF-WEB3D_quantized_attributes is worth the burden. It feels like no.

@javagl
Copy link
Contributor

javagl commented Nov 30, 2016

Although I might overlook or underestimate some justification for this, I agree that creating "all variants" can be taken arbitrarily far. A devil's advocate could consider this a first step on the road towards a combinatorial explosion: One can embed Buffers, Shaders, and/or Images, in 8 combinations, each of them combined with binary/commonMaterials/quantization, leading to 64 variants...

But seriously: There certainly should be test cases for "mixed type assets", like

  • binary glTF with external references
  • binary glTF with embedded parts
  • ...

But I think that these are rather conceptual tests that could all be covered with a single model (e.g. the textured box).

@pjcozzi
Copy link
Member

pjcozzi commented Dec 6, 2016

@lasalvavida what did you think of #4 (comment)? @javagl seems to agree, #4 (comment)

@lasalvavida
Copy link
Contributor Author

I understand the concerns of a slippery slope of having too many variants of models, so let me completely explain the justification for what I have here and we'll work from there. I know it's a bit long; thank you for reading through it.

I think at the bare minimum, the sample models should have a single variant for each supported extension, which enables this repo to be used easily for testing extension support. I hope we can agree on that.

The separate/embedded question is a bit more complex. The embedded models are more convenient (drag-and-drop a single file). However most text editors struggle to read them because of the enormous single line base64 encoded uris. For example, the embedded BrainStem model is almost unreadable in atom. For someone wanting to learn about glTF and explore how the models are implemented, a separate variant seems like it should be a must.

So then, for the examples of implemented extensions, what do we do? We could keep doing what we have been {glTF(separate), glTF-embedded, glTF-extension(separate)...}, which does allow readability for exploring how extensions are implemented. However, then if I want to quickly grab for example a WEB3D_quantized_attributes sample model since it is the smallest version of the asset, I have to run it through some other tool in order to get it as a single embedded glTF.

As a result, I arrive at the conclusion that the best arrangement of the sample models is to include a variant of each extension, and for each of those, a separate and embedded variant. The embedded variant can be easily dragged-and-dropped and is more convenient for use. The separate variant is more human readable for exploring the spec.

Does that make sense?

@javagl
Copy link
Contributor

javagl commented Dec 6, 2016

It sounds reasonable, and limiting the dimensions of the variants to "Extension X Embedding" will limit the slipperiness of the slope.

I think at the bare minimum, the sample models should have a single variant for each supported extension

Of course, having many valid sample models may help to increase the test coverage, broaden the loader support, help implementors to quickly test their tools with all these variants and quickly check whether a particular model works well with a specific extension.

In any case: For those who contribute models, it will be crucial to have an easy way to create all variants of one model (as Patrick already pointed out).

So I do not want to disagree with you, and do not want to propose anything particular here, but am once more playing devil's advocate:

If such an easy way to generate all variants exists, then the importance of really having all variants of all models in the repo may be lower. Everybody could generate all desired variants quickly, easily and locally.


The following may sound like arguments for or against something, but are really just points that I thought about in this context:

  • One could argue that the whole point of KHR_binary_glTF is basically to take the idea of embedded data one step further and just embed everything in one file.
  • I think that nobody would try to learn about the KHR_materials_common extension based on the BrainStem model. He would likely use the "Box" model, any maybe even prefer a simple scene, containing 12 spheres, rendered with the 4 materials x 3 light types, tailored to show exactly the functionality of the extension.

@lasalvavida
Copy link
Contributor Author

In any case: For those who contribute models, it will be crucial to have an easy way to create all variants of one model (as Patrick already pointed out).

We do. I've been using the gltf-pipeline project for generating all of these variants.

If such an easy way to generate all variants exists, then the importance of really having all variants of all models in the repo may be lower. Everybody could generate all desired variants quickly, easily and locally.

I'm actually not opposed to this as a solution, but it probably needs to be discussed more. We need to decide what this repository is supposed to be. It would certainly make maintenance/git compatibility better to only include the separated glTF asset and include instructions for generating the other variants with gltf-pipeline.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 8, 2016

We need to decide what this repository is supposed to be.

This repo is sample models for the glTF community. It is not sample models meant to be directly referenced, e.g., git submoduled, into an engine for tests. An engine may use all or a selection of these models, plus some of their own, for testing.

However, we want enough variants here that engines that can load them all have a very good chance of being fully conformant.

@lasalvavida perhaps the right approach is to start with the script/instructions for using gltf-pipeline to generate all the variants, and then decide what models to include here?

Your readable vs. convenience points, #4 (comment), are good. If models only had one variation, we would have to decide what that would be; if binary glTF is widely supported, which we would like, then that would likely have the best trade offs.

@lexaknyazev
Copy link
Member

However most text editors struggle to read them because of the enormous single line base64 encoded uris. For example, the embedded BrainStem model is almost unreadable in atom.

Just found that VS Code handles such files w/o any issues (with coloring, folding, etc).

@javagl
Copy link
Contributor

javagl commented Dec 20, 2016

There are several editors that can work with such long lines (e.g. https://www.textpad.com/ opens it smoothly as well), but the general statement is true: Many editors will bail out.

I'm not sure how important this is, because the files are not "supposed to be edited". It should only be a way to transfer the whole data at once, to be directly loaded by a renderer. Apart from the fact that, for such large files, the overhead of the base64 encoding becomes an issue: For BrainStem, it's 15.4 MB vs. 11.4 for the bin file. One could think about a concept that allows a single buffer to be assembled from multiple parts. Maybe by just adding a uri to a bufferView... but I doubt that this is worth the changes...

@lasalvavida
Copy link
Contributor Author

I'm going to close this set of pull requests for now in favor of the current organization.

@lasalvavida lasalvavida deleted the regenerate-2CylinderEngine branch May 19, 2017 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants