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

GltfImporter: Only copy actually used vertex data ranges #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hugoam
Copy link

@hugoam hugoam commented Jun 14, 2022

Hello !
We are using GltfImporter plugin on a model such as this one: https://sketchfab.com/3d-models/issum-the-town-on-capital-isle-e433923a64d549fabb2d30635d643ab6 and we discovered the behavior of the importer on this specific file is unfortunately quite unoptimal, so this PR attempts to fix it

One characteristic of this model is that it has a single huge 12MB buffer, and all the ~600 meshes in the scene are just disjoint (very far apart) views on parts of this huge array. So for example the vertex positions of one mesh will be a small range in the beginning of this 12MB buffer and the UVs will be a small range at the end.

With the current GtfImporter logic, when calling the importer->mesh(id) for all those 600 meshes, this results in 600 MeshData objects where each has a copied vertexData array of about 12MB, for a total of a couple of GB, which is quite far from optimal memory usage. Looking at the code, the reason seem to be that we use a single range that encompasses all the attributes accessor ranges in the same buffer (so when those are disjoint and very far apart, we end up with huge unused buffer parts in the middle)

So this PR tries to address this by creating a number of ranges when the attribute accessors are disjoint (but we just keep one if they are interleaved/overlapping or contiguous)

Let me know your thoughts and if you think this approach for a solution is suitable 😅

@hugoam hugoam force-pushed the gltf-importer-subranges branch 2 times, most recently from 59396c9 to e048c62 Compare June 14, 2022 15:59
@mosra mosra added this to the 2022.0a milestone Jun 15, 2022
@mosra
Copy link
Owner

mosra commented Jun 15, 2022

Thanks for tackling the serious issues first 👍 -- I was well aware of this problem but tried to pretend it doesn't exist, procrastinated away the fix and where it got serious I band-aided over it by passing the mesh through MeshTools::interleave() to get rid of the extra weight before it snowballs and takes down the machine 😅

We were discussing this problem and various ways to fix this with @pezcode some months ago, and I suppose you had that as a starting point. So just for completeness, the reason I did it like this was to prepare for zero-copy asset import from glTF (and to some extent PLY and STL). There the idealistic case would be memory-mapping the file and feeding the GPU vertex/index buffers directly from the SSD, which means the to-be-uploaded data range needs to be taken directly from the file, without any data shuffling happening along the way. But that is an idealistic case, and unfortunately the case you're describing here is far more common. Another use case for the zero-copy behavior is for various asset pipeline tools, for example opening a file and calculating a bounding volume of a mesh. There it doesn't really matter if the pieces are far apart, as the unused data will be simply skipped.

Besides that, to complicate the situation further, I'm about to add a "mesh views" workflow for multi-draw scenarios, in particular having multi-draw-ready assets without any subsequent runtime processing. This would mean that, if the importer discovers that accessors for the same attributes in different meshes are compatible (same type/offset/stride), it'll expose them as a single MeshData. The scene then, instead of refering to "render mesh 15", would contain information like "reder Y indices from mesh 0 starting at offset X". In practice that could very well mean the 12 MB buffer with very far apart views you mention actually is already a multi-draw-optimized mesh, and it just doesn't have the attributes interleaved -- first positions for all meshes, then normals for all meshes, etc. With the mesh views workflow you'd get those 12 MB just once, and then 600 offsets/counts.

So, I think the original behavior still makes sense for the planned zero-copy and mesh view workflows, so I'd like to not completely discard it. On the other hand, eating gigabytes of RAM for a 12 MB file by default is stupid, so the compacting behavior you're proposing has to be there as well.

I don't know yet how to combine the two, so if its okay (and you don't critically need this merged yesterday), I'll take a few days to think about this, and then I come back to you. I'm leaning towards having a standalone tool in MeshTools for this, that the importer (or any other code) would then use -- the compacting behavior isn't exactly trivial, especially when alignment and packed attributes come into play, and having it standalone would make testing much easier.

@hugoam
Copy link
Author

hugoam commented Jun 15, 2022

Hey, no problem, this is not urgent on our side, I was just investigating a performance topic that unveiled this issue and then I tackled it, a bit out of curiosity, while I was at it, but I don't think we are in a big hurry.

The idea indeed floated through my mind that the exporter might give us sometimes fully owned mesh data but also sometimes views or references to an already imported chunk of mesh data, but I thought this complicates the importer API quite a bit, but I'm sure you already have ideas about how to do this.

I gave a try at a quick practical solution as I assumed something more well designed would probably take many months to design. It might be nice to expose something like the code path I wrote through an optional compactVertexData importer setting or something of that nature if that's something you're willing to consider. And yeah, it would be nice to separate the logic to something that is easily testable as standalone, indeed.

We'll wait for your input then 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants