Allow Savables to be loaded by static builder methods

[codex128]

Currently Savables are required to have a parameterless constructor in order to be deserialized by a JmeImporter. This puts some fairly major restrictions on how classes are implemented, even ones that don't implement Savable. I think this can be fixed by allowing Savable classes to specify a static builder method, which is responsible only for instantiating the Savable from the InputCapsule. Further deserialization is performed using the normal read method.

public class MySavable implements Savable {
    @SavedObjectBuilder
    public static Savable loadSave(InputCapsule in) throws IOException {
        return new MySavable(in.readInt("value", 0));
    }
}

I've already managed to implemented this on my vulkan fork without any outright breaking changes, so I am curious if this is something we want in jme3 (or whether we want this at all).

[riccardobl]

This would make things much harder.

For example, some classes might want to deserialize internal states, and while you could do that from the loadSave method, now every class extending this class would have to know about this and reproduce the same logic, you would also need to make everything protected instead of private.
On the contrary, with the current system, you just call super.read with peace of mind.

[riccardobl]

What restrictions did you find in the current serialization system?

[codex128]

What restrictions did you find in the current serialization system?

First, a Savable cannot easily extend a non-Savable class that does not have a parameterless constructor itself. The Savable subclass forces the parent to behave like a Savable. Second, fields that are should be final often can't due to the deserialization process. Not horrible but still annoying imo.

you would also need to make everything protected instead of private.

I think this best illustrates how it works:

public class A implements Savable {
    
    private final int value;
    private int internal = 0;
    
    @SavedObjectBuilder
    public static A loadSave(InputCapsule in) throws IOException {
        return new A(in.readInt("value", 0));
    }
    
    public A(int value) {
        this.value = value;
    }
    
    @Override
    public void write(JmeExporter ex) throws IOException {
        OutputCapsule out = ex.getCapsule(this);
        out.write(value, "value", 0);
        out.write(internal, "internal", 0);
    }
    
    @Override
    public void read(JmeImporter im) throws IOException {
        InputCapsule in = im.getCapsule(this);
        internal = in.readInt("internal", 0);
    }
}

public class B extends A {
    
    private int otherInternal = 0;

    @SavedObjectBuilder
    public static B loadSave(InputCapsule in) throws IOException {
        return new B(in.readInt("value", 0));
    }
    
    public B(int value) {
        super(value);
    }

    @Override
    public void write(JmeExporter ex) throws IOException {
        super.write(ex);
        OutputCapsule out = ex.getCapsule(this);
        out.write(otherInternal, "otherInternal", 0);
    }
    
    @Override
    public void read(JmeImporter im) throws IOException {
        super.read(im);
        InputCapsule in = im.getCapsule(this);
        otherInternal = in.readInt("otherInternal", 0);
    }
    
}

value can be private and final and still be deserialized, and A and B don't need parameterless constructors. Also, B is still able to properly deserialize value even if A is not a Savable.

[riccardobl]

oh so you mean to keep also the current system and use this only as a replacement for the default constructor.
I see, but this makes things even more complex, as you have now the serialization logic split in multiple methods.

First, a Savable cannot easily extend a non-Savable class

I think this is a bad idea, there are many things that can make a class non savable, imo the developer should not try to convert a non-savable class to a savable class like this. They should rely instead on classes that we define as savable from the ground up or new classes they make.

I think all these issues can be resolved by building a savable wrapper for non savable classes, in this way you can control the logic entirely.

[codex128]

What about using a constructor accepting an InputCapsule instead of using a static builder method? That would solve the problem of having duplicate logic between builders.

public class A implements Savable {
    
    public A(InputCapsule in) throws IOException {
        // deserialize
    }
    
}

public class B extends A {
    
    public B(InputCapsule in) throws IOException {
        super(in);
        // deserialize
    }
    
}

the developer should not try to convert a non-savable class to a savable class like this.

Ok, that is fair point. There is still the issue of forcing fields to be non-final though. I can design around it, but it makes things messier and less clear than it otherwise could be.

[riccardobl]

What about using a constructor accepting an InputCapsule instead of using a static builder method? That would solve the problem of having duplicate logic between builders.

I think this is still asking for troubles, since it split the serialization logic between the constructor and read method.
Unless this is really needed, i think we'd better accept that savable classes will be a little bit uglier and keep the current method.

[codex128]

The serialization logic might be split, and that could be a bit confusing. I'd argue that the current system is more confusing and harder to work with because it forces these weird design decisions. For example, Material has to make its MaterialDef field non-final. What does that mean? Can it be changed? Can it be null? At what point must it be initialized? If it were final, these questions would be succinctly answered by it's own field declaration, but it can't even though it should. The current system also duplicates initialization logic between the constructor and read method, this new system would remove that duplication.

[riccardobl]

I think the idea is that initialization is one thing, deserialization is another.
Afaik what we do in jme is a common pattern, that is also similar to java's own serialization logic, except you almost never have to override the read method there.

[codex128]

initialization is one thing, deserialization is another.

If deserialization brings the object from an unusable state to a usable state, it is initializing the object. Material is a good example of this: during the deserialization process, the material object is instantiated in a broken, unusable state since its material definition is null. read populates the definition and causes the material object to be usable. So I don't think initialization and deserialization are strictly separate ideas.

what we do in jme is a common pattern, that is also similar to java's own serialization logic

Yes, but java serialization is able to serialize final fields, which we are unable to do.

[riccardobl]

So I don't think initialization and deserialization are strictly separate ideas.

You might be deserializing cached values that are otherwise computed during normal initialization, so a constructor like public A(InputCapsule in) would probably need to be treated as deserialization-only (and likely protected), similar to how we sometimes use a default constructor only for deserialization, because it might do only partial initialization and would still be part of the deserialization system.

So, this boils down to the main benefit of this approach being only that it allows final fields, but then there is another problem: super(...); must be the first call in a constructor. That limits flexibility a lot.

If it is just a matter of making the code safer to work with, eg. for things like the MatDef example you mentioned, i think an alternative would be to use error-prone with a custom checker that works on two custom annotations: eg. @EffectivelyFinal to tag fields and @EffectivelyFinalWriter to tag methods that can write these fields.

[codex128]

i think an alternative would be to use error-prone with a custom checker that works on two custom annotations

Sure, that sounds like a good idea. 👍
I'd obviously prefer to use final, but this seems like an acceptable balance.