GLTFLoader parser.associations refactor

[elalish]

Description

<model-viewer> has two open issues that trace back to errors in parser.associations: google/model-viewer#3287 (comment), and google/model-viewer#4029

The second turns out to be caused by ReduceAssociations() being called for multiple scenes, which was introduced here to handle a memory leak: #22583. I believe these kinds of errors will keep cropping up unless we can simplify the associations.

Solution

I think a better approach for avoiding memory leaks would be to make the associations into a WeakMap instead of a Map. Then we could remove ReduceAssociations() entirely and not worry about having entries for extra clones. This is a breaking change and I'll have to refactor MV a bit for it, but since associations isn't documented I think @takahirox is the main other user of it, so I'm mostly looking for his opinion. @donmccurdy, hopefully you'll appreciate the simplification here.

Alternatives

Just fix these bugs in place, but I fear the code will get even more convoluted and will continue to be prone to memory leaks.

Additional context

No response

[donmccurdy]

This is a breaking change and I'll have to refactor MV a bit for it...

I don't think I see what makes this a breaking change, wouldn't the API be the same, and the contents of the association would be a superset of what it is now?

/cc @drcmda in case this affects R3F or gltfjsx

[elalish]

Well, it's a breaking change for me because I was looping through the keys of the Map, which you can't do with a WeakMap. But yeah, it's not a huge change.

[takahirox]

I'm a bit too busy now and haven't deeply thought yet but changing Map to WeakMap sounds ok to me. And we may add the note to the migration guide.

[donmccurdy]

Sounds good to me as well. 👍🏻

[donmccurdy]

PRs would still be welcome, but no current plans to implement this.