KhronosGroup / glTF-Sample-Assets

To store all models and other assets related to glTF
349 stars 36 forks source link

The MetalRoughSpheres are HUGE #45

Open javagl opened 7 years ago

javagl commented 7 years ago

From just looking at the glTF file and textures, it's hard to understand the actual structure of the MetalRoughSpheres model. The asset reports its generator as "COLLADA2GLTF with hand-edits", so I'm not sure whether the culprit here is the original COLLADA file, COLLADA2GLTF, or (unlikely: ) the hand-editing. There seem to be five meshes, but it's not clear which part of the model belongs to which mesh (and it may be fiddly to figure it out).

But please correct me if I'm wrong at any point here:

So

When this is considered or supposed to be a "very basic test model for PBR in glTF", then this may not shine the best light on glTF ("Compact"? "No, not at all...")


At the risk of sounding self-righteous: In the (now somewhat out-dated) https://github.com/javagl/gltfTestModels/tree/master/SimpleSpheres test model, I used one sphere, and showed it 25 times...

emackey commented 7 years ago

I'm the author. This model isn't intended to be either basic or compact. The spheres are high-poly and there are a lot of them, the idea being to carefully test the effects of the metalness and roughness channels of the map. I had intended to make another one that tested the factors, as opposed to the texture, but never got around to that.

The model has more than 64k vertices, and I had problems exporting 32-bit indicies with the then-current version of COLLADA2GLTF. So, I split the model into 5 separate meshes, and exported with 16-bit indicies. Each sphere has 2562 vertices and 7680 triangles.

Is this really considered a bug? It was certainly intentional on my part. It doesn't need to be shown in a high-performance rendering situation, it's for examining the materials in fine detail.

donmccurdy commented 7 years ago

Is this really considered a bug?

I tagged it with the intention of categorizing it alongside other (possible) issues in the sample models. I'm fine with removing that tag.

I don't have a strong opinion on the tradeoff between samples that showcase glTF best-practice, vs including extra hand optimizations that tooling doesn't automatically provide (e.g. reusing a single sphere mesh).

javagl commented 7 years ago

@emackey Yes, I noticed that the textures are carefully adjusted to the model (and that it's not just 100 spheres with different materials).

(I also considered that this might be a side-effect of COLLADA2GLTF, which, IIRC, may "bake" translations into meshes and create files that are structurally simpler, but based on what you said, this is probably not the reason).

I just was surprised to see that an asset that could be very small and simple actually was huge and complex. The point is: One could easily have not 100 spheres with 8000 triangles each, but even 1000 spheres with 20000 triangles each, and the size of the resulting asset could still be only a fraction of the current one...

However, OK to close this one, if it's not considered to be worth changing.

scurest commented 7 years ago

FWIW, MetalRoughSpheres also takes a noticeably longer time to import into Blender than any other sample (about 9s on my PC), more than twice the time taken by the next slowest (GearboxAssy, about 4s). I import all the sample files to test the importer and the two files for MetalRoughSpheres account for about 17% of the total test time. Not that that means anything, but it really is huge :)

I put together this script to generate the glTF @emackey mentioned that's like MetalRoughSpheres, but with factors instead of textures. There's one mesh/material pair for every sphere and they all reuse the same accessors. All together the glTF and bin come to about 141KB. If you want to try it, just run it and it should drop MetalRoughFactorSpheres.{gltf,bin} in the current directory. You'll need to put Spheres_BaseColor.png there too (for the labels).

For MetalRoughSpheres, you could do the same thing, reusing the position/normal accessors between spheres, but you'd still need texture data for every one. That would still cut a significant chunk of the filesize out though. I could modify my script to do that if there's interest in making this smaller (the annoying bit is just writing down the net of an icosahedral sphere).

emackey commented 7 years ago

I can certainly see advantages to having a much smaller sample model, so long as most of the quality is preserved (meaning, don't substantially drop the triangles-per-sphere or number of spheres). If @scurest or anyone wants to take a shot at compacting the model as described above, that would be great!

The original model came from a .blend file that is included in the sourceModels folder. This was before we had a stable Blender exporter and I was using the work-in-progress COLLADA2GLTF at the time. The hand-edits were mostly to apply the PBR material manually.

scurest commented 7 years ago

Here's a script to generate MetalRoughSpheres (again, it will just drop a gltf and bin in the current directory and you'll need to copy the two textures over). The new sizes are

2.2M MetalRoughSpheres.bin
 72K MetalRoughSpheres.gltf

Here's what the UVs look like excolor

There's actually more verts in this version because of how I unwrapped the icosahedron I guess D: The number of tris is the same.

For some reason, the viewer draws this model with flat shading despite it having normals. Anyone know why?

edit: One more thing; although it's interesting to get the size down by reusing accessors, it really tanks the performance when viewing, so I don't necessarily think this is a good idea.

scurest commented 7 years ago

Btw, running the textures through zopflipng will shave about half the filesize off. It's insignificant compared to the size of the .bin but I figure we might as well.

cx20 commented 7 years ago

@scurest The three.js's glTF Loader treats it as flat shading if geometry.attributes.normal is undefined. https://github.com/mrdoob/three.js/blob/dev/examples/js/loaders/GLTFLoader.js#L1841 Because the model in glTF-Sample-Models is displayed correctly, it seems that there is a problem with the generated model.

cx20 commented 7 years ago

@scurest The above problem seems to be flat shading because Labels has no NORMAL. https://gist.github.com/scurest/3c67b60e900f4142436730eef8b68f92#file-mrspheres-py-L372 However, In this case, I can not judge how to handle it. Is this a library problem? /cc @donmccurdy

donmccurdy commented 7 years ago

three.js is following this implementation note from the specification:

Implementation note: When normals are not specified, client implementations should calculate flat normals.

So if smooth normals are intended, then attributes.NORMAL must be included.

scurest commented 7 years ago

@cx20 👍 Specifically, it happens when the spheres (which have normals) and the labels (which don't) share a material. I've revised the gist to include normals for the labels so the spheres are smooth now.

donmccurdy commented 7 years ago

Specifically, it happens when the spheres (which have normals) and the labels (which don't) share a material.

Oh, yeah that's a bug in THREE.GLTFLoader then. Shared material isn't supposed to matter. 😁 Will fix.

cx20 commented 7 years ago

@scurest Although it is not a big problem, When I tried to display with the same test code, it seems that the diameter of the sphere is about 2.4 to 2.5 times larger. image If possible, I think that it is desirable to have the same diameter as the original sphere.

scurest commented 7 years ago

Is it? As far as I can tell, they're all unit spheres; the ones in the original .blend, the ones in the current MetalRoughSpheres, and the ones generated by my script. How do you see a difference?

cx20 commented 7 years ago

@scurest I tried testing the two models with the same code. Three.js + current MetalRoughSpheres.gltf result: image model: https://cdn.rawgit.com/KhronosGroup/glTF-Sample-Models/c89c1709fbfd67a11aa7e540ab4ecb795763b627/2.0/MetalRoughSpheres/glTF/MetalRoughSpheres.gltf

Three.js + new MetalRoughSpheres.gltf(generated by mrspheres.py) result: image model: https://cdn.rawgit.com/cx20/jsdo-static-contents/5dd7b91beb205ca3c5138709ad83d8e3f12f3221/models/gltf/2.0/MetalRoughSpheres_scurest/MetalRoughSpheres.gltf

Unfortunately, I am not familiar with Blender, so I did not know where to check.

scurest commented 7 years ago

Ah, you're right, it's because of this matrix. 1/0.4 = 2.5. I missed it in the glTF because I was looking for a scale property ><

I revised a "scale": [0.4, 0.4, 0.4] in.

cx20 commented 7 years ago

@scurest I confirmed that it was improved by the latest version of mrspheres.py. Three.js + new MetalRoughSpheres.gltf(generated by modified mrspheres.py) result: image

javagl commented 1 year ago

The same problem can be observed in IridescenceDielectricSpheres and IridescenceMetallicSpheres: They have >10MB bin files (and this alone may cause hesitation to offer them as glTF-Binary as well).

In contrast to the MetalRoughSpheres, these models do indeed use the same data for many spheres. The following snippet, based on the dedup function from glTF-Transform, can be used to optimize them:

import path from "path";
import fs from "fs";
import { NodeIO } from "@gltf-transform/core";
import { dedup } from "@gltf-transform/functions";
import { KHRONOS_EXTENSIONS } from "@gltf-transform/extensions";

const baseDir = "C:/glTF-Sample-Models/";

function ensureDirectoryExists(fileName: string) {
  const directory = path.dirname(fileName);
  if (!fs.existsSync(directory)) {
    fs.mkdirSync(directory, { recursive: true });
  }
}

async function runOptimize(modelName: string) {
  const inputDir = baseDir + modelName + "/glTF/";
  const outputDir = baseDir + modelName + "/glTF-Optimized/";

  const inputFileName = inputDir + modelName + ".gltf";
  const outputFileName = outputDir + modelName + ".gltf";

  const io = new NodeIO().registerExtensions(KHRONOS_EXTENSIONS);
  const document = await io.read(inputFileName);
  await document.transform(dedup());
  const jsonDocument = await io.writeJSON(document);

  ensureDirectoryExists(outputFileName);
  fs.writeFileSync(outputFileName, JSON.stringify(jsonDocument.json, null, 2));
  for (const uri of Object.keys(jsonDocument.resources)) {
    const resource = jsonDocument.resources[uri];
    const resourceFileName = path.join(outputDir, uri);
    ensureDirectoryExists(resourceFileName);
    fs.writeFileSync(resourceFileName, resource);
  }
}

async function run() {
  await runOptimize("IridescenceDielectricSpheres");
  await runOptimize("IridescenceMetallicSpheres");
}

run();

Now... whatever we're doing with that: We'll still have these 10MB files in the Git history. But I'll probably still open a PR for that, because this reduces the size of the .bin files from 10.3 megabytes to 41 kilobytes, with no visual difference in the results ... and that ain't bad, I guess...

DRx3D commented 1 year ago

@javagl Perhaps both can be left in the directory with a separate explanation of optimization.

javagl commented 1 year ago

@DrX3D I'm not sure what you mean. I think that we should generally try to make sure that the sample models (or sample assets) are good. This term has many dimensions and trade-offs (and maybe some of them are somewhat subjective). But I can not imagine any reason to not reduce the size of a model by 97%, while also reducing the amunt of RAM that is required for rendering the model. There are no disadvantages or drawbacks, from what I can tell.

Unfortunately, this optimization is not applicable to the MetalRoughSpheres. Creating a new, "clean" MetalRoughSpheres would be a matter of minutes, except for the 'labels'. But I'll probably extend my local "sample model generator code" to cover things like this as well, or try to take the original labels and somehow merge them with new, redundancy-free spheres geometry in blender or so...

emackey commented 10 months ago

It looks like this issue has already been resolved for the Iridescence sphere tests. They are substantially smaller in this repo than in the previous one.

For MetallicRoughnessSpheres, it looks like we now have both a large textured version and the small non-textured version side by side. Is this intentional that we keep both? Is only one needed?

javagl commented 10 months ago

The iridescence models had been updated in https://github.com/KhronosGroup/glTF-Sample-Assets/pull/6 . The original version still increases the repo size, but I still think that it's worthwhile to have a small version of the model - e.g. when it is supposed to be a basic demo model that may be included in some viewer package, and to have faster downloads when selecting it in the glTF-Sample-Viewer.

For the MetalRoughSpheres:

I'm not sure about the roles or intentions of the original model and the ...NoTextures version. Specifically: I don't fully understand what the 'textures' are in the first one. AFAICT, the 'textures' are basically used to assign a single material to each sphere. This means that it accomplishes the same as a small material { ... roughness: 0.123 } JSON snippet, but using a texture makes it necessary to have texture coordinates in the geometry (and the PNG file for the texture itself)

I don't have a strong opinion (beyond "The model should not be so large"), but

But ... replacing the textures would make it so similar to the ...NoTextures version that it could be hard to justify keeping both in the repo...