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

ObjectLoader: Moved json handling to geometries #22040

Merged
merged 4 commits into from
Jun 24, 2021
Merged

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jun 24, 2021

Description

This PR moves the primitives json handling out of ObjectLoader.

@mrdoob mrdoob added this to the r130 milestone Jun 24, 2021
console.warn( 'THREE.ObjectLoader: Unsupported geometry type "' + data.type + '"' );
if ( data.type in Geometries ) {

geometry = Geometries[ data.type ].fromJSON( data, shapes );
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is not the prettiest...
Only ExtrudeGeometry and ShapeGeometry uses the shapes parameter 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider to resolve the shape UUIDs in ObjectLoader and then store the resulting shapes in the data object. In this way, all fromJSON() method would have a common signature.

However, it would be necessary to check for data.shapes in ObjectLoader for each geometry.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure what's better...
I think I'll try the current approach for now.

@mrdoob mrdoob merged commit 9517d2e into dev Jun 24, 2021
@mrdoob mrdoob deleted the objectloader-cleanup branch June 24, 2021 16:04
@joshuaellis joshuaellis mentioned this pull request Jul 5, 2021
5 tasks
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

Successfully merging this pull request may close these issues.

2 participants