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

Spline's type can only be deserialized as CatmullRom #2366

Open
xiaojiao-zsw opened this issue Feb 10, 2025 · 7 comments · May be fixed by #2368
Open

Spline's type can only be deserialized as CatmullRom #2366

xiaojiao-zsw opened this issue Feb 10, 2025 · 7 comments · May be fixed by #2368
Assignees
Labels
defect Something that is supposed to work, but doesn't. Less severe than a "bug"

Comments

@xiaojiao-zsw
Copy link

When Spline is serialized, type will be serialized to SplineType.CatmullRom by default, regardless of the current type value.

public void write(JmeExporter ex) throws IOException {
OutputCapsule oc = ex.getCapsule(this);
oc.writeSavableArrayList((ArrayList) controlPoints, "controlPoints", null);
oc.write(type, "type", SplineType.CatmullRom);

Also, when deserializing, type reads the key name as “pathSplineType” instead of “type”.

public void read(JmeImporter im) throws IOException {
InputCapsule in = im.getCapsule(this);
controlPoints = in.readSavableArrayList("controlPoints", new ArrayList<>());
/* Empty List as default, prevents null pointers */
float list[] = in.readFloatArray("segmentsLength", null);
if (list != null) {
segmentsLength = new ArrayList<>(list.length);
for (int i = 0; i < list.length; i++) {
segmentsLength.add(list[i]);
}
}
type = in.readEnum("pathSplineType", SplineType.class, SplineType.CatmullRom);

@stephengold
Copy link
Member

Good catch!
Thank you, @xiaojiao-zsw, for documenting this issue.
I'll write some tests and try to get this corrected in the next release.

@stephengold stephengold self-assigned this Feb 10, 2025
@stephengold
Copy link
Member

On closer inspection, the Spline.write() code might be correct:
The value written by OutputCapsule.write() is the 1st argument. The 3rd argument to OutputCapsule.write() is a default value that is never written.

However, the mismatch of names ("pathSplineType" versus "type") is clearly a bug.

@stephengold
Copy link
Member

Testing uncovered another issue with spline serialization: in the case of an NURBS spline, the knot list is a non-null List<Float>. Line 483

oc.writeSavableArrayList((ArrayList<Float>) knots, "knots", null);

causes a ClassCastException to be thrown in BinaryOutputCapsule.writeSavableArrayList() because Float cannot be cast to Savable.

@stephengold
Copy link
Member

stephengold commented Feb 10, 2025

I submitted PR #2368 to solve the bugs, which apparently date back to at least 2014:

https://github.com/jMonkeyEngine/jmonkeyengine/blob/a16857c8f4e7aadf3ca5fcb79bff5c8f16eb5481/jme3-core/src/main/java/com/jme3/math/Spline.java

In other words, they aren't recent regressions. It appears they were missed due to inadequate test coverage.

@stephengold stephengold added the defect Something that is supposed to work, but doesn't. Less severe than a "bug" label Feb 10, 2025
@xiaojiao-zsw
Copy link
Author

On closer inspection, the Spline.write() code might be correct: The value written by OutputCapsule.write() is the 1st argument. The 3rd argument to OutputCapsule.write() is a default value that is never written.

However, the mismatch of names ("pathSplineType" versus "type") is clearly a bug.

Oh, that was my mistake, it's now clear that this is entirely due to the mismatch of names causing type to only be loaded as CatmullRom when deserializing, thanks for that!

@xiaojiao-zsw
Copy link
Author

On closer inspection, the Spline.write() code might be correct: The value written by OutputCapsule.write() is the 1st argument. The 3rd argument to OutputCapsule.write() is a default value that is never written.
However, the mismatch of names ("pathSplineType" versus "type") is clearly a bug.

Oh, that was my mistake, it's now clear that this is entirely due to the mismatch of names causing type to only be loaded as CatmullRom when deserializing, thanks for that!

Now I totally think “Spline's type can only be deserialized as CatmullRom” is the correct title for this issue.

@stephengold stephengold changed the title Spline's type can only be serialized as CatmullRom Spline's type can only be deserialized as CatmullRom Feb 11, 2025
@stephengold
Copy link
Member

I changed the title as @xiaojiao-zsw suggested.

Possibly related: I'm puzzled as to why Spline can't be deeply cloned. I've opened a new issue (#2370) for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Something that is supposed to work, but doesn't. Less severe than a "bug"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants