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

Clone objects with userData that isn't JSON encodable? #23956

Closed
willstott101 opened this issue Apr 27, 2022 · 7 comments
Closed

Clone objects with userData that isn't JSON encodable? #23956

willstott101 opened this issue Apr 27, 2022 · 7 comments

Comments

@willstott101
Copy link
Contributor

Is your feature request related to a problem? Please describe.

I cannot clone my scene (which I wanted to do for export purposes, see #23951) because my userData has got cyclic references in it. Now on the one hand I'd concede perhaps it shouldn't, but it does seem like an unnecessary limitation to me.

Describe the solution you'd like

A) An extra option to the clone/copy functions to toggle the copying of userData.
B) Use of structuredClone in environments that support it over JSON.parse(JSON.stringify(x))

I think I should have the time to implement one of these if they're likely to get merged.

Describe alternatives you've considered

Traversing and removing my userData before cloning, and then re-adding it.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 28, 2022

I don't vote to support this use case. It's over-engineering. I would rather suggest to clarify the documentation.

https://threejs.org/docs/index.html#api/en/core/Object3D.userData

The docs already mentioned a limitation about functions. It should be no deal to enhance this section about cyclic references.

@willstott101
Copy link
Contributor Author

It's not so much about cyclic references, though cyclic references are fairly unique in the fact that the thrown error just completely prevents cloning... it's more about the constraint on JSON serialization.

(Admittedly modern) standard types like Set and Map just get serialized to {} with JSON.stringify.

I agree that the extra option is over-engineering, but structuredClone is ECMAScript's native answer to this problem, can using it really be considered so?

I guess it would be a breaking change since functions would throw when cloning, rather than simply being ignored. But if three wants to try and deep-clone userData why would we not want to use the better, simpler and less hacky function for it? Is it just cause it's new and we don't want to have code that needs to fallback to JSON-cloning?

@LeviPesin
Copy link
Contributor

I agree that the extra option is over-engineering, but structuredClone is ECMAScript's native answer to this problem, can using it really be considered so?

How would you stringify an object using it?

@willstott101
Copy link
Contributor Author

You can't. That does seem like a separate bit of functionality to me though.

Cloning and serialization are separate concerns, evidenced by the fact that Three.js internally has separate hand-written toJSON and copy functions for pretty much all of it's classes. In fact afaict in the core nothing uses this json hack at all other than cloning userData and BufferGeometry.groups.

I'm not certain that structuredClone is the perfect solution, but if we we want to continue automatically cloning the userData fields without any customizability then I think structuredClone is the better solution.

JSON round-trip:

  • doesn't support modern container types (Map & Set)
  • loses cloned object's prototype reference
  • ignores functions completely
  • throws for reference loops

structuredClone:

  • structuredClone would appear to be a little slower in v8 currently (which surprised me)
  • still doesn't keep the prototype reference
  • throws when encountering a function

if (typeof this.userData.clone == "function") other.userData = this.userData.clone() 🤷 :

  • puts the power (of making mistakes) in the hands of users
  • kinda confusing and weirdly "magical" - specially named functions seem odd somehow
  • could be replicated for toJSON - to have a neat way of putting other THREE.js objects as userData

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 30, 2022

Sorry, but this feature request is out of scope. userData fields should only be used for what you can encode to JSON and not with functions or (cyclic) object references. With that policy, userData is supported similar to glTF's extras field.

@wlinna
Copy link
Contributor

wlinna commented Jun 8, 2023

@willstott101 Instead of traversing and removing the cyclical references, why not use Symbols as keys instead? Symbol keys are ignored by JSON.stringify

@willstott101
Copy link
Contributor Author

If it was data I wanted excluded from clone I could just add my own "realUserData" field or something. This ticket is about extended what kind of data is supported in user data by three.js itself. I can store my own data elsewhere if I didn't want three.js' existing clone to work

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

4 participants