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

NURBSCurve.toJSON doesn't serialize points data #29478

Closed
canxerian opened this issue Sep 23, 2024 · 6 comments · Fixed by #29514
Closed

NURBSCurve.toJSON doesn't serialize points data #29478

canxerian opened this issue Sep 23, 2024 · 6 comments · Fixed by #29514
Labels
Milestone

Comments

@canxerian
Copy link
Contributor

canxerian commented Sep 23, 2024

Description

The JSON object returned from NURBSCurve.toJSON() doesn't contain data relating points, knots or control points, but instead contains:

{
    "metadata": {
        "version": 4.6,
        "type": "Curve",
        "generator": "Curve.toJSON"
    },
    "arcLengthDivisions": 200,
    "type": "Curve"
}

Reproduction steps

  1. Create an instance of NURBSCurve (sample code below),
  2. console.log nurbsCurveInstance.toJSON()
  3. Notice there is no data relating to the instance, e.g points, knots and control points)

Code

const nurbsControlPoints = [];
const nurbsKnots = [];
const nurbsDegree = 3;

for (let i = 0; i <= nurbsDegree; i++) {
  nurbsKnots.push(0);
}

for (let i = 0, j = 20; i < j; i++) {
  nurbsControlPoints.push(
    new THREE.Vector4(
      Math.random(),
      Math.random(),
      Math.random(),
      1 // weight of control point: higher means stronger attraction
    )
  );

  const knot = (i + 1) / (j - nurbsDegree);
  nurbsKnots.push(THREE.MathUtils.clamp(knot, 0, 1));
}

const nurbsCurve = new NURBSCurve(nurbsDegree, nurbsKnots, nurbsControlPoints);

console.log(nurbsCurve.toJSON());

Live example

https://codesandbox.io/p/sandbox/threejs-nurbscurve-tojson-bug-s298gc

Screenshots

No response

Version

168

Device

Desktop, Mobile

Browser

Chrome, Firefox, Safari, Edge

OS

MacOS

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 23, 2024

NURBSCurve is part of the addons and there is no JSON support for this class right now. Only curves classes in the core support that.

Even with a proper NURBSCurve.toJSON() implementation you also need the fromJSON() counterpart to make a deserialization work.

@Mugen87 Mugen87 added the Addons label Sep 23, 2024
@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 24, 2024

The next problem is that objects deserialized with ObjectLoader can't restore instances of NURBSCurve because the type is unknown for the core. So we can only add partial serialization/deserialization support right now.

@canxerian
Copy link
Contributor Author

canxerian commented Sep 25, 2024

Thanks for the quick reply, Mugen87.

I'm happy to add the functionality for NURBSCurve.toJSON and NURBSCurve.fromJSON, and can push a PR shortly.

With regard to ObjectLoader, if I understand correctly it appears that there's doesn't exist any support for any type of Curve yet.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 25, 2024

ObjectLoader can save and load e.g. instances of ShapeGeometry which define their outlines via curves. The deserialization is delegated to CurvePath. The problematic bit is this:

this.curves.push( new Curves[ curve.type ]().fromJSON( curve ) );

Curves represents what is defined in this file: https://github.com/mrdoob/three.js/blob/be667f12f9f114ab3a4e12bbd45b99f9822ef56f/src/extras/curves/Curves.js

@canxerian
Copy link
Contributor Author

Quick q @Mugen87, I came across the doc, https://github.com/mrdoob/three.js/wiki/Mr.doob%27s-Code-Style%E2%84%A2, but didn't see any mention of a VS Code formatter extension. Is there one available to auto format code for the preferred code style?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 27, 2024

If you configure VS code with an ESLint extension, it should automatically detect the lint configuration in https://github.com/mrdoob/three.js/blob/dev/.eslintrc.json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants