Possible issue with Savable

I started using the Savable implementation about a week ago and came across something that might be an issue. I have a subclass of Node and in my subclass I have an ArrayList of Spatials called “lights”. When I went to load this object after having saved it, I kept getting errors to the effect that my array had a negative size, then I basically just saved an empty array and got an error that it couldn’t be cast to ArrayList. Well I took a wild guess that it was the name of the object causing the problem and sure enough, I was right. So there must be something saved in a superclass called “lights” that my code ended up loading instead of the object I had saved called “lights”. This seems like it could be dangerous because I don’t always know what objects are being saved further up the tree in the JME superclasses. Had this object been an int for example, named the same, I could’ve easily loaded erroneous data which could’ve caused a very hard to track down bug.

As a user, the only way I can think of to prevent this would be to prefix all of my objects name’s with something unique so there is no conflict. But it seems that perhaps the load/save code in jme should take this issue into consideration and differentiate objects saved in subclasses and superclasses as being different even if named the same. Just a suggestion to avoid some grief.

Did your IDE not tell you that you were overriding the existing lights collection and that your local var was hiding the super classes implementation?

Nope. It didn’t. I’m guessing that’s because in the superclass the actual object is probably called something like lightslist but in the read/write methods for savable it’s just ‘lights’.

Probably the only reasonable “solution” would be to catch the issue early and prevent you from doing such a thing. Otherwise, we have to bloat every object in a non-backwards compatible way just to let people do something we strongly discourage in the first place (and is apparently pretty rare). That seems unlikely.

At least if you got an error when you were saving you’d know you had a conflicting name.

It’s not about the field names in the class but about the field names that are saved. The IDE can’t know that the write() method used “lights” when storing the spatial’s light list.

True, but what if the objects are of the same time? You’d never know for sure if you had an issue and I’d think it’d be hard to track.

Yup I understand now after his edit. Carry on :slight_smile:

I don’t understand how time comes into it.

When writing out fields, if you’ve already written that field then we could throw an exception.

Heh, oops, typo, I meant same TYPE. My bad.

That’s one of the many example why we recommend to not override Node or Geometry or Spatial.
A spatial has a list of lights called localLights, and it’s serialized as “lights”.

What’s your use case? why do you store this list of spatials? Maybe we can guide you to a safer approach.

I understand nehon, and you are probably right about overriding Node. I’m manipulating certain spatials in the object, culling them at times and so forth. Anyway, I think actually the easiest solution to this issue would just to make sure that all jme objects are serialized with the same name as the actual object. I think that would help a lot.

No, that breaks every existing j3o just to save one user from a bad thing.

The fields that are saved may be different than the fields in the object for any number of reasons.

The best we can do is to throw an exception if you try to write the same field. That’s the answer. That would have let you know immediately that you were doing bad things the first time you tried to save it.

That would also break compatibility with older j3o’s unless we do many convoluted things… so let’s explore the other options.

Got it. Yeah that would be a pain.

I think your solution is good. If it threw an error on save when it found conflicting names, that would work well I think.

I only looked for five minutes… but it seems like we could modify this line:
cObj.nameFields.put(name, new BinaryClassField(name, alias, type));

…in BinaryOutputCapsule to throw an exception if put() returns a previous value.

Edit: except I’m wrong. a) there is an earlier check preventing that from working, and b) that cObj is shared across instances. Just need to add a Set<String> to the BinaryOutputCapsule to detect dupes.

Well that’s not that easy. OutputCapsule is an interface, and experters implements it. That would mean we have to change this to handle this for every exporter in a default capsule, or that every exporter has to handle it…

There are like, two, right?

yeah :stuck_out_tongue:
but anyone can make its own exporter. Feels weird conceptually to defer this N times to each implementation

But it’s an implementation specific limitation.

Maybe some exporter stores the class name with every written field and doesn’t have this issue.

OUR exporters do bad things if you use the same field name from different classes. So we should abort on write instead of reading corrupted data.

(Actually, to be honest, I’m not sure why it didn’t just work… perhaps the read/write order is not consistent from super to subclass.)

yeah true…

Well depends when OP called super.wrtie().