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

MergeBuffers causing "Destination array was not long enough." in _StaticBufferBuilder #101

Closed
xPaw opened this issue Apr 28, 2021 · 9 comments

Comments

@xPaw
Copy link

xPaw commented Apr 28, 2021

Original issue: ValveResourceFormat/ValveResourceFormat#346

We call the code here: https:/SteamDatabase/ValveResourceFormat/blob/b277571cd47b5f41f94982170cb07e45c6c5e04a/ValveResourceFormat/IO/GltfModelExporter.cs#L438-L446

It's throwing here:

I tested with WriteSettings.MergeBuffers = false and it exports, it also seems to not happen with other models.

I don't know if this is a mistake on our end or what, but we don't call that Save multiple times, so this shouldn't be a threading issue. That particular model has 732 buffers, so it could be something to do with that.

Stacktrace:

System.ArgumentException: Destination array was not long enough. Check the destination index, length, and the array's lower bounds. (Parameter 'destinationArray')
   at System.Array.Copy(Array sourceArray, Int32 sourceIndex, Array destinationArray, Int32 destinationIndex, Int32 length, Boolean reliable)
   at System.Array.Copy(Array sourceArray, Int32 sourceIndex, Array destinationArray, Int32 destinationIndex, Int32 length)
   at System.SZArrayHelper.CopyTo[T](T[] array, Int32 index)
   at System.Collections.Generic.List`1.InsertRange(Int32 index, IEnumerable`1 collection)
   at SharpGLTF.Schema2._StaticBufferBuilder.Append(Byte[] data)
   at SharpGLTF.Schema2.BufferView._IsolateBufferMemory(_StaticBufferBuilder targetBuffer)
   at SharpGLTF.Schema2.ModelRoot.MergeBuffers()
   at SharpGLTF.Schema2.WriteContext._PreprocessSchema2(ModelRoot model, Boolean imagesAsBufferViews, Boolean mergeBuffers)
   at SharpGLTF.Schema2.WriteContext.WriteTextSchema2(String baseName, ModelRoot model)
   at SharpGLTF.Schema2.ModelRoot.SaveGLTF(String filePath, WriteSettings settings)
   at SharpGLTF.Schema2.ModelRoot.Save(String filePath, WriteSettings settings)
   at ValveResourceFormat.IO.GltfModelExporter.WriteModelFile(ModelRoot exportedModel, String filePath) in ValveResourceFormat\IO\GltfModelExporter.cs:line 445
   at ValveResourceFormat.IO.GltfModelExporter.ExportToFile(String resourceName, String fileName, Model model) in ValveResourceFormat\IO\GltfModelExporter.cs:line 293

Version: SharpGLTF.Toolkit 1.0.0-alpha0022

@vpenades
Copy link
Owner

vpenades commented Apr 28, 2021

Ok, I'll give it a look as soon as I can. Right now I'm working on another issue, so it will take a few days.

Additionally, it's going to be very hard to find unless a test case, or an offending model is provided.

@xPaw
Copy link
Author

xPaw commented Apr 28, 2021

so it will take a few days.

No worries, there is no rush of course.

What would be the easiest way to share the model with you?

That particular model is from https://steamcommunity.com/sharedfiles/filedetails/?id=928179372 (it's over 300 megs), and I can see how it's not easy to get hold of that if you don't use steam.

@vpenades
Copy link
Owner

hmmm... I have a steam account, but I barely use it; I tried but I've been unable to download that file

@vpenades
Copy link
Owner

vpenades commented May 3, 2021

@xPaw looking again at the exception call stack, I've realized the exception is being thrown within the List .AddRange method... and I would be surprised there's a bug in there.

So, the only thing that makes sense to me, given the size of the model, is that at some point, the List needed to resize its internal buffer, failed to allocate the required memory and threw the exception. (but not an outofmemory exception which is what you would expect in this case)

Notice that in order to merge the buffers it needs to reallocate all the buffers into a single, very large memory chunk.

Questions:

Are you compiling the code for 32bits?

which was the size of the _Data list when the exception happened? and the size of the data being added?

Additionally, Looking for that exception, I've found several threads reporting similar issues when calling List methods from multiple threads, but I don't think this is the case, but if it gives you a lead...

@xPaw
Copy link
Author

xPaw commented May 4, 2021

I am compiling for 64bit.

Unfortunately my VS seems somewhat broken, and I can't properly debug library code (it just refuses to load code from symbols).

image

I did manage to grab that, and it does look like it actually runs over int32.MaxValue?

So sounds like a better structure for this big buffer is needed (or perhaps use IEnumerable so that it's not copied into one big blob?)

@vpenades
Copy link
Owner

vpenades commented May 4, 2021

Well... it looks like it's a lot of data you're willing to put into a single buffer.

As far as I know, glTF schema doesn't support buffers larger than what can be indexed by a 32 bit value. I guess it's designed that way to simplify client development.

I think a long time ago it was discussed, and rejected (by the khronos board) to support such large buffers.

So even if I modify the library to allow 64 bit indexing (which woul be a huge change library wide), the written models would be unloadable by many clients.

My suggestion would be to use this code snippet:

long totalSize = ModelRoot.LogicalBuffers.Sum(buffer => (long)buffer.Content.Length);

before attempting to merge the buffers. If the size is larger than a safe value, then disable buffer merging.

Now, don't use int.MaxValue as the threshold value, because List increases its internal size by growing steps, in fact I have no clue which would be the safe value, you'll have to do some trial & error.... but I would bet a safe value is just 1Gb (which is already a VERY LARGE buffer)

This could have been addressed if GLB would allow for multiple buffers, again, this is something that has been discussed and requested in the khronos board.... the fact that GLB only supports a single buffer is a huge problem because it doesn't allow seamless conversion between glb and gltf

@xPaw
Copy link
Author

xPaw commented May 4, 2021

Ahh yeah, that would be one reason for failure.

From the spec: https:/KhronosGroup/glTF/blob/master/specification/2.0/README.md#buffers-and-buffer-views

It sounds like the 32bit limit is only for GLB, so that's worth keeping in mind.

I did disable merging buffer using your suggestion, so that issue is solved in this regard. Is this anything SharpGLTF may improve (in terms of throwing a validation error)?

EDIT: With the 32bit limit, shouldn't it at least be uint32?

@vpenades
Copy link
Owner

vpenades commented May 4, 2021

The difference between glTF and GLB is that glTF can have multiple buffers, and GLB only one. But each individual buffer is limited to what can be addressed by a 32bit value.

Furthermore, .NET uses a SIGNED Int32 value for most addressable collections. I'ts widely used in Arrays, Lists, ArraySegments, Spans, etc.

This means that in practice, the number of bits that can be used for addressing is just 31 bits, which means only 2Gb can be addressed.

And again, I suspect many implementations might have the same issue, so it could be said the largest a buffer can grow without risking having trouble is just 2Gb.

Allowing SharpGLTF to grow up to 4GB might require changing lots of Ints by UInts, and writing custom collections that supports large buffers. Just replacing List by a LargeList looks like a lot of work.

My rule of thumb would be: whenever a glTF is bigger than 1gb, store it as a zipped glTF, instead of a GLB

@vpenades
Copy link
Owner

vpenades commented May 5, 2021

@xPaw In 4788e47 I've just added a few improvements to handle scenarios with very large models.

When merging to a single buffer, it checks whether the total sum of buffers is less than 2Gb, otherwise it throws a NotSupportedException.

Also I've added new write settings to support merging the buffers up to a given size, splitting the resources into multiple bigger but valid sizes. This allows writing very large models, while keeping a short number of binary files.

An example of that configuration can be found here.

So, right now the approach to write a GLB would be:

  • If the resources sum less than 2Gb, write GLB
  • Else, write glTF, using the new buffer merging/splitting settings.

Let me know if this allows you to handle these large models.

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

No branches or pull requests

2 participants