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

Curves: Added serialization/ deserialization #12797

Merged
merged 7 commits into from
Dec 18, 2017
Merged

Curves: Added serialization/ deserialization #12797

merged 7 commits into from
Dec 18, 2017

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Dec 4, 2017

This PR adds serialization/ deserialization for all curve entities. It's now possible to store and load ShapeGeometry and ShapeBufferGeometry.

The implementation allows to perform serialization/ deserialization on all hierarchy levels (Curve, CurvePath, Path, Shape) with .toJSON() and .fromJSON() methods.

I'll consider ExtrudeGeometry in a separate PR.

Demo: https://jsfiddle.net/ht3zuzkr/2/

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 4, 2017

This PR directly addresses #8007 since ObjectLoader can now load ShapeGeometry and ShapeBufferGeometry.

@mrdoob
Copy link
Owner

mrdoob commented Dec 5, 2017

Hmm... It's kind of tempting to move shapes to its own group (along geometries and materials in the JSON format. In order to support this we'll probably need to add a UUID to Shape too. Would that make things too complicated?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 5, 2017

Okay, I've changed the code so shapes are stored in their own group. ShapeGeometry and ShapeBufferGeometry now refer with uuids to their shapes.

Updated Demo: https://jsfiddle.net/ht3zuzkr/2/

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 12, 2017

@mrdoob Is the PR now okay like this? 😊

@mrdoob
Copy link
Owner

mrdoob commented Dec 13, 2017

Checking!

@@ -923,6 +923,32 @@ Object.assign( BufferGeometry.prototype, EventDispatcher.prototype, {

}

// ShapeBufferGeometry

if ( data.type === 'ShapeBufferGeometry' ) {
Copy link
Owner

Choose a reason for hiding this comment

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

How about adding this in ShapeBufferGeometry's own toJSON()?

ShapeBufferGeometry.prototype.toJSON = function () {

	var data = BufferGeometry.prototype.toJSON.call( this );

	data.shapes = [];

	if ( Array.isArray( shapes ) ) {

		for ( var i = 0, l = shapes.length; i < l; i ++ ) {

			var shape = shapes[ i ];

			data.shapes.push( shape.uuid );

		}

	} else {

		data.shapes.push( shapes.uuid );

	}

	return data;

}

I think that should work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Indeed, it's better to place this code not in Geometry or BufferGeometry but in ShapeGeometry.

@@ -1009,6 +1009,32 @@ Object.assign( Geometry.prototype, EventDispatcher.prototype, {

}

// ShapeGeometry

if ( data.type === 'ShapeGeometry' ) {
Copy link
Owner

Choose a reason for hiding this comment

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

Same here...

@mrdoob
Copy link
Owner

mrdoob commented Dec 13, 2017

Apart from these two. It's looking good to me! 👌

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 13, 2017

Done! Updated Demo https://jsfiddle.net/ht3zuzkr/2/

The PR performs no changes to Geometry or BufferGeometry anymore 😊

if ( parameters !== undefined && parameters.shapes !== undefined ) {

var shapes = parameters.shapes;
var uuids = [];
Copy link
Owner

Choose a reason for hiding this comment

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

What's this array for? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, this is a remnant of a previous solution. I'll remove it...

@mrdoob
Copy link
Owner

mrdoob commented Dec 18, 2017

Beautiful! 😊

@mrdoob mrdoob merged commit 130a02e into mrdoob:dev Dec 18, 2017
@mrdoob
Copy link
Owner

mrdoob commented Dec 18, 2017

Thanks!

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