Add OnBeforeDisposeVolatileData method

[brnkhy]

I was working on adding support to read custom vertex attributes for vertices as an add-on and if I understand the structure correct, I need the buffer information before it got disposed, to be able to read data. This method fires after data is loaded but before it's disposed.
About the usage; I have a custom GltfImport class, which overrides this new OnBeforeDisposeVolatileData method, then uses GetImportAddonInstance method to find my custom add-on and class a OnLoadCompleted method in it. Add-on uses accessor data, decodes it and caches the colors. later when OnMeshAdded is called, it applies the extracted data to the mesh.
Can't say I extensively tested my add-on and system built on top of this but it seems to work fine so far. I can also share those if you want to examine the use-case but they are not included into the PR of course.

[cla-assistant-unity]

All committers have signed the CLA.

[atteneder]

Thanks for handing this in and the patience.

While the solution seems sound and functional, I'd like to explore alternative and provide a bit more context.

I see three solutions:

1. Virtual method (your suggestion
2. Public event (public event Action OnBeforeDisposeVolatileData;)
3. Interface that add-ons need to implement.

One has the disadvantage that one needs to derive from GltfImport and if the goal is to invoke certain add-on methods still need to query them. As soon as multiple add-ons require this step it becomes hard to govern all of that.

Two removes the need to query targeted add-ons, so arguably better.

Three is aligned with recent work we did on the image loading API or animation API work. It's very explicit, but might be a bit convoluted/verbose.

GltfImport already seriously violates the single responsibility pattern and DisposeVolatileData is a symptom of that. In the future I'd like to see GltfImport get split into multiple modules (potentially add-ons), each self-governing short-lived allocations on their own. That being said, the only implication to this PR is that whatever we end up building, we should be careful when naming it. Something like PostAssetConversion, PostLoad might be more expressive to someone not familiar with the code base (though the latter is too generic imho).

I tend towards solution 2, but happy to hear your thoughts.

Thanks.

[brnkhy]

Hey @atteneder, thanks for your response!
I worked on this in short periods with long time in between so my memory and understanding is very fuzzy. Last year, I started this whole custom vertex attribute work for an internal project. I knew nothing about gltf and I just recklessly hacked library code to make it happen. It worked fine for that. But now we are considering to release that work publicly and I don't want it all to be based on a hacked version of gltfast we will never be able to update.

So after months, this year I went back to this; I forgot everything about gltf which I never really knew much anyway. I was also learning AI stuff those days so I took a shot with AI support to refactor it to use add-on system. I'm saying this because admittedly I can't claim to fully understand the full cycle of loading process and I might have missed some already existing possible solutions there.

Short what happens at the moment is; I have an add-on with an add-on instance. I also have custom gltfImport, which overrides the new OnBeforeDisposeVolatileData method above. then it finds the FeatureIdImportAddonInstance it has and calls the OnLoadCompleted method in it. In this method I extract the custom vertex data. I wasn't able to do it in OnMeshAdded because... I think vertex data was already disposed there? I can't remember exactly.

My first implementation was a public event as well, again I can't remember clearly but I think I switched to this because another visibility problem...
OK I just had a look, I think initially, I was using m_ImportInstances to find my own add-on instance in the custom gltfimport, and that field being private didn't allow me to do the event solution. Later I found about GetImportAddonInstance method and now... I haven't tested it yet but I think a public event would work just as good. I need to test though.

I have no idea about option 3, I don't think I understand add-on system well. But if it's an interface that'll receive a call before raw data is disposed, so I can extract custom vertex attribute; I guess it might work.

Whole idea is to have this as a clean add-on so it'll be easier to maintain and update in the future. I updated to latest gltfast during this refactor a few weeks ago and there was an easily noticeable performance improvement.

If you want to check, I can send you my add-on work stuff as well, it's just too much to drop here but I can email.

[brnkhy]

        /// <summary>
        /// Raised at the beginning of <see cref="DisposeVolatileData"/> before buffers are freed.
        /// Subscribe to process data while it is still accessible.
        /// </summary>
        public event Action<IGltfReadable> BeforeDisposeVolatileData;

        /// <summary>
        /// Free up volatile loading resources
        /// </summary>
        async Task DisposeVolatileData()
        {
            BeforeDisposeVolatileData?.Invoke(this);

did a short test and this seems to work just as good and I definitely agree it's better; I can change the PR tomorrow for another review.

[atteneder]

I've tried to get more context about your use-case in the original GH issue, and here's why:

Instead of positioning the callback you want right before the data you need (the buffers) are disposed, wouldn't it make much more sense to have a callback that fires as soon as the buffers are available? That's why I first asked what kind of temporary data you need to dispose on your end (which seems to be none, if I'm not mistaken).

The main advantage I see is that various conversion processes can happen in parallel (like loading images).

Since you're loading custom mesh attributes, you likely need to sync up with (or inject into) where glTFast generates the Unity meshes, so it's likely not so straightforward. Let's elaborate that further.

[atteneder]

btw: A generic API for custom mesh attribute import would be my preferred solution, but I understand that this exceeds the scope of your work. Still I want to learn, so that I know your requirements once we get into it.

[brnkhy]

@atteneder

Instead of positioning the callback you want right before the data you need (the buffers) are disposed, wouldn't it make much more sense to have a callback that fires as soon as the buffers are available?

that makes a lot of sense, I didn't really pick BeforeDispose because I need it, I guess it was the first one I stumbled upon and felt OK thinking I extract my custom data last. Shortly, I think I need an interrupt/callback for addons where I can reach the buffers; is that correct?

Whole point of my work is; our api endpoint servers gltf files with this custom vertex attribute we use for styling. Like doors, windows etc. has a custom style id. It's a combination of a few things but I just need to read it and pass it into the shader, then I apply a vertex color depending on that value in shader.

I just pushed my work on a public branch;
You can find most of it here; https://github.com/mapbox/mapbox-unity-sdk/tree/landmarksMerge/Runtime/Mapbox/LandmarksModule/Gltf
and the meat of it is probably here; https://github.com/mapbox/mapbox-unity-sdk/blob/landmarksMerge/Runtime/Mapbox/LandmarksModule/Gltf/FeatureId/FeatureIdImportAddonInstance.cs

I'm very open to all suggestions, as I mentioned this wasn't my initial implementation but a ai-supported refactor to turn it into add-ons.

[atteneder]

Perfect, that gave me quick insight.

Seems like you convert from buffers to IDs array and inject those into the resulting meshes' vertex colors after they've been created.

Short-term I think it's best to provide a suitable event for you to hook into.

Long-term it's better to inject your customization/extractino of additional attributes directly into the low-level mesh API magic for best performance.

Spending more thinking about it I realize that a dedicated interface has the benefit of a potential return value. Add-ons that hook into it could signal that the loading failed and should not proceed further (or alike). We did that with the latest addition, an interface for a post JSON parsing hook.

I plan to implement it like so, but feel free to beat me to it if you agree.

[brnkhy]

@atteneder
I think I'll leave it to you, as I might need to learn and understand more for the (add-on?) interface solution.
Also is there any chance you can give me a rough ETA for such a change? So I can decide how to progress, either wait for that version and have gltfast as dependency or do the changes myself and have the modified gltfast inside the project.