AnalyticalGraphicsInc / gltf-vscode

This is an extension for Visual Studio Code to add support for editing glTF files.
Apache License 2.0
460 stars 63 forks source link

BufferView indices jumbled after .glb import #80

Closed zellski closed 6 years ago

zellski commented 6 years ago

Thanks for an excellent tool! I've come across a small snafu with this file:

blackvan_with_windows.glb.gz

When imported as a .glb, the created .gltf fails validation, and it's not hard to see why: the accessors' bufferVIew references have all been renumbered, but some of the images still retain their old bufferView references.

I have not dug deep into why this might be happening, but a pretty big clue is:

  "images": [
    {
      "name": "rough_met_/Users/zell/models/blackvan_take_2/blackvan_specular.png_/Users/zell/models/blackvan_take_2/blackvan_gloss.png_/Users/zell/models/blackvan_take_2/blackvan_windows.png",
      "uri": "blackvan_with_windows_img0.jpg"
    },
    {
      "name": "base_col_/Users/zell/models/blackvan_take_2/blackvan_windows.png_/Users/zell/models/blackvan_take_2/blackvan_specular.png",
      "uri": "blackvan_with_windows_img1.png"
    },
    {
      "name": "blackvan_normals.png",
      "uri": "blackvan_with_windows_img2.png"
    },
    {
      "name": "blackvan_emissive.png",
      "uri": "blackvan_with_windows_img3.png"
    },
    {
      "name": "rough_met_/Users/zell/models/blackvan_take_2/blackvan_specular.png_/Users/zell/models/blackvan_take_2/blackvan_gloss.png_/Users/zell/models/blackvan_take_2/blackvan_diffuse.png",
      "uri": "blackvan_with_windows_img4.jpg"
    },
    {
      "name": "base_col_/Users/zell/models/blackvan_take_2/blackvan_diffuse.png_/Users/zell/models/blackvan_take_2/blackvan_specular.png",
      "uri": "blackvan_with_windows_img5.jpg"
    },
    {
      "name": "blackvan_normals.png",
      "bufferView": 2,
      "mimeType": "image/png"
    },
    {
      "name": "blackvan_emissive.png",
      "bufferView": 3,
      "mimeType": "image/png"
    }
  ],
emackey commented 6 years ago

Since this results in a broken model, I submitted a fix for this in najadojo/gltf-import-export#3.

But, in terms of best practice, I don't think duplicating the image block in the original GLB is the right way to go. It would be better to re-use the image index number, such that only a single image block referenced a single bufferView. This way the model can't have different settings (for example different MIME types or different names) pointing to the same block of binary data.

emackey commented 6 years ago

Also note I didn't fix the export, as it's not broken, but not optimal. After importing this model with the "fixed" importer, you get two image blocks that both reference the same external file. The exporter (and I believe most engines) will not realize the duplication, and will load the file twice, and store separate copies of it in the newly re-exported GLB file. Engines will also likely load the same file twice, unless they are looking for matching filenames.

Ideally, there should be one image block per file (or per bufferView image), and the image block can be referenced from multiple places (or the texture block can be referenced multiple times, etc). Some engines are smart about this and will not reload the asset per reference.

zellski commented 6 years ago

Great work! Ironing out these dubious corner cases is not as exciting as obvious new functionality, but it's satisfying in its own way, yeah?

And, I agree -- in this case, it's at the Texture -> Image indirection that shared references make sense. At some point I added a createRawBufferViewFromFilesystem() type function in FBX2glTF, and while I was doing it I threw in a Map<FilePath, BufferView> cache without really thinking things through. I'll fix that.

Still, as you say, this is legal glTF. And there are other circumstances where multiple references to a shared BufferView does make more sense, though, right? The first example that strikes me is the way sparse accessors are designed, where -- if I understand it correctly (I have not done the deep dive to actually implement them yet) -- such an accessor can refer to one bufferView as the "backdrop" data (as opposed to all zeroes, which happens in the absence of a bufferView reference) and then supply that dictionary of individual element overrides?

One would imagine a loader/renderer that wants the memory saving nature of sparse accessors in this scenario would need to implement shared bufferViews through reference, probably at a low enough level that even the Texture/Image case would benefit some...

emackey commented 6 years ago

it's satisfying in its own way, yeah?

Certainly. If no one reports any bugs, it's safer to assume no one is using it 😄 so getting some fixable bugs squashed makes everything feel more robust.

emackey commented 6 years ago

Version 2.1.4 was just published, and includes the fix for this.