Subclassing for sake of serialization/parsing and adding behavior, is broken

Bugs
1

I am trying to tap into serialization to add my own behavior to GPUParticle system

https://www.babylonjs-playground.com/#PJDB1J#1

Open up console, call data = particleSystem.serialize(); – It works. Now try to create a new GPUParticleSystem (the subclassed system):

data = particleSystem.serialize();
newSystem = GPUParticleSystem.parse(data, particleSystem._scene)

Blows up, because BABYLON.GPUParticleSystem.Parse, is doing

        var name = parsedParticleSystem.name;
        var particleSystem = new GPUParticleSystem(name, { capacity: parsedParticleSystem.capacity, randomTextureSize: parsedParticleSystem.randomTextureSize }, scene);

The easy fix is:

        var name = parsedParticleSystem.name;
        var particleSystem = new this(name, { capacity: parsedParticleSystem.capacity, randomTextureSize: parsedParticleSystem.randomTextureSize }, scene);

Which should be done for really all the serializable objects, in order to allow people to subclass things.

Beyond that however: It would be nice to have an easy/simple api to hook into to extend serialization, i.e.

BabylonObject {
  static Parse() { 
    //do parsing
    if(this.additional_serializable_fields) { //parse these fields also }
  }
}

– The other fix would be to separate “Parsing” from initializing the object altogether. I.e. parse just returns the javascript object, and instead of

Class.Parse()
to initialize,

Class.FromJSON = function(data) {
  let result = this.Parse(data);
  let instance = new this(result);
}

(or Class.Load, or whatever)

Which IMO would be the better way to handle it, however, would be a breaking API change – Reason I think it’s better way to handle is Parse implies you are parsing something, while in reality it is parsing and instantiating. Separating the behavior and adding a Load method would be more intuitive but more importantly, allow for subclassing. Additionally if every class defined the array of fields it needs to serialize in a static method, then that could be used in place of suggestion #2, as I could just call

FieldsToSerialize() {
  [...super.FieldsToSerialize(), 'additional_field1', 'additional_field_2']
}

I only looked in a few other places but seems like it’s broken (or rather, isn’t currently possible without overwriting the serialization entirely) for basically all classes.

2

While I am not currently being affected by it, It also looks like some instance clone methods (maybe all, on all objects) are doing similar thing, and should instead be doing

new this.constructor()

To be friendly to subclassing.

3

Also, now realizing that Vectors don’t seem to be able to serialize/deserialize themselves. Is there a reason for storing them as an array?

It seems to me that if EVERYTHING that can be serialized is expected to have a serialize/parse (and rely on that method for when serializing/unserializing), it would be quite a bit cleaner, and doing something like the FieldsToSerialize suggestion I mentioned above would then be possible (which I didn’t realize at the time, every class is implementing its own completely unique serialization logic) – it would remove walls of code from the repo, allow for more extensibility, etc. – Obviously one offs would still need to be handled separately, but having some base serialization logic to rely on, serialize/parse for everything, and things being broken into smaller methods would clean things up immensely IMO.

4

I’m ok move to new this instead of new Class (at least for particles for now)

Serialization is now done either using SerializationHelper which uses @serializeXXX decorator, either using custom Parse/Serialize functions when we want finer / more precise control

The serializationHelper or even the Parse functions rely on DeepCopier class that will check if a clone function exists on all members.

For the specific case of the particles, we could definitely add either an observable or a user defined functions for Parse/clone/serialize

TLDR: Please do a PR with your good ideas :slight_smile:

1 Like
5

Coolio I will try when I can find time, I didn’t see the serialization helper I’ll have to take a look at that

1 Like