search

mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.31k stars 35.35k forks source link

GLTFLoader parser.associations refactor #25252

Open elalish opened 1 year ago

elalish commented 1 year ago

Description

<model-viewer> has two open issues that trace back to errors in parser.associations: https://github.com/google/model-viewer/pull/3287#issuecomment-1075505706, and https://github.com/google/model-viewer/issues/4029

The second turns out to be caused by ReduceAssociations() being called for multiple scenes, which was introduced here to handle a memory leak: https://github.com/mrdoob/three.js/pull/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Sounds good to me as well. 👍🏻