vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
454 stars 72 forks source link

[BUG] WriteSettings.MergeBuffers is ignored for both GLTF and GLB export formats. #165

Closed MeltyPlayer closed 1 year ago

MeltyPlayer commented 1 year ago

Describe the bug When trying to export a model root via modelRoot.Save(filePath, writeSettings) as a GLTF or GLB file, WriteSettings.MergeBuffers is ignored in recent releases of SharpGLTF.

This is a blocker for me to update to newer versions since some of the models I'm trying to export are enormous, and SharpGLTF will consistently throw OutOfMemoryExceptions in 32-bit applications unless MergeBuffers is set to false.

It appears that MergeBuffers is always set to true within WithTextSettings() and WithBinarySettings()--is this intended? In previous versions, this was not the case for GLTF files, allowing me to export large files as GLTF. I was experiencing this issue back then for GLB files, though.

To Reproduce This happens consistently when I try to call modelRoot.Save(filePath, writeSettings) for a filePath ending in .gltf or .glb. It appears that this is because Save() internally calls SaveGLB() or SaveGLTF(), which both override the WriteSettings that were passed in by calling WithBinarySettings()/WithTextSettings().

please complete the following information:

MeltyPlayer commented 1 year ago

Ah whoops, forgot to include the code locations:

SaveGLB()/SaveGLTF(): https://github.com/vpenades/SharpGLTF/blob/1bfe321a34347ba95f2cc1f1e5df993db60e7856/src/SharpGLTF.Core/Schema2/Serialization.WriteSettings.cs#L173-L211

WithTextSettings()/WithBinarySettings(): https://github.com/vpenades/SharpGLTF/blob/1bfe321a34347ba95f2cc1f1e5df993db60e7856/src/SharpGLTF.Core/Schema2/Serialization.WriteContext.cs#L105-L127

vpenades commented 1 year ago

First of all, yes, the merge buffers was intended, specially for the binary GLB format, which only supports a single blob file (which IMHO is a glTF design flaw)

For your particular use case, where you need to write more than one binary file, there's no point in using GLB because the first file would be inside the GLB, and the subsequent binaries would be in external files, which defeats the purpose of a GLB which is to be packed in a single file.

That leaves glTF as your only option, and yes, MergeBuffers needs to be allowed to be false, for text files. I'll look into it.

The solution is to remove these two lines from WithTextSettings

 MergeBuffers = true; 
 JsonIndented = false; 

For the binary version, they need to remain because it's the only way to ensure that the GLB is written to a single file.

MeltyPlayer commented 1 year ago

Ah, that sounds good. Thanks for the quick response!

vpenades commented 1 year ago

I've uploaded the changes a while ago.

Let me know if it fixes you problem

MeltyPlayer commented 1 year ago

Looks good, thanks for fixing this!