Paper.js: Item exportJSON with visible=true does not export "visible" property causing side effects on importJSON

[simonemazzoni]

Hello,
I just discovered a little inconsistency in the exportJSON/importJSON methods on items.

I tried to export a visible in a JSON object and I noticed that the visible property isn't exported in the JSON since, I guess, paperjs logic implies that by default the item is visible and that's ok.

'["Path",{"name":"ByzSHWmgW","applyMatrix":true,"data":{"pathId":"ByzSHWmgW","userAlias":"usr_47c72bf42d92"},"segments":[[[227,352],[0,0],[3.49673,3.49673]],[[255,354],[-3.82246,-0.16619],[22.91894,0.99648]],[[324,355],[-22.93356,-0.54604],[52.93142,1.26027]],[[486,333],[-49.66118,20.37382],[54.45013,-22.33852]],[[624,209],[-26.55556,53.11113],[0,0]]],"strokeColor":[0,0,0],"strokeWidth":10,"strokeCap":"round","strokeJoin":"round"}]'

If I set the same item to visible=false, to make it invisible, and export it again to JSON, the result JSON is similar to the first with the difference that this time, it contains also the visible property set to false.

'["Path",{"name":"ByzSHWmgW","applyMatrix":true,"visible":false,"data":{"pathId":"ByzSHWmgW","userAlias":"usr_47c72bf42d92"},"segments":[[[227,352],[0,0],[3.49673,3.49673]],[[255,354],[-3.82246,-0.16619],[22.91894,0.99648]],[[324,355],[-22.93356,-0.54604],[52.93142,1.26027]],[[486,333],[-49.66118,20.37382],[54.45013,-22.33852]],[[624,209],[-26.55556,53.11113],[0,0]]],"strokeColor":[0,0,0],"strokeWidth":10,"strokeCap":"round","strokeJoin":"round"}]'

Now, If I try to import the first JSON in the path created with the second JSON to modify it (from invisible to visible), the resulting path is invisible.

My guess is that this behaviour is caused by the fact that the fist JSON doesn't have the visible property explicitly set to true, so the importJSON method doesn't change it as I hoped.

Is this an intended behaviour? I use the importJSON method in my project to update existent paths and it work good except for this case.

I found a workaround that consist in create a new_path, and then invoke the method replaceWith(new_path) on the old one. But I think is a less handy and efficient approach. Don't you agree @lehni ?
Here a paper sketch example of the behaviour.

Simone

[sapics]

Hi, @simonemazzoni
I guess that we would see similar behavior in the other properties, like opacity, blendMode, etc.
If you have problem only visible in your project, path.visible=true;path.importJSON(json); would be an easy solution.

[simonemazzoni]

@sapics I don't think you get my point. Let me better explain.

I slightly edited the paper sketch to better show you what is the behaviour. paper sketch

As you can see, the whole point is given by the fact that the exportJSON() method contain the visible property only if the path on which is called has that property set to false. This, cause the side effect that if I import that json representation of the path on a path that has the visible property set to false, it doesn't change the property to true, because isn't written in the json.

I use this method to serialise and store path states since they can be changed during the time and reverted back to a previous state. So I use exportJSON and importJSON to export and import the state of a path and change it according to the situation.

The thing is that in my opinion, the exportJSON should export all the relevant properties of a . path including the visible property with both values true and false, and not only false.

I tried to have a look at the code of exportJSON function, but since it uses recursive functions, is not that easy to read, and I still couldn't find a way to 'force' the export of the visible property even if it is set to true.

[sapics]

Of course, your way replaceWith(new_path) is better in general.
If you have problem only visible property in your project, sketch would be faster solution.

[simonemazzoni]

Yes, I got it. Both the solutions works. The fact is that they look a bit hacky, and I think that many people never realises this behaviour with path import/export. So I guess that having a look at those functions to prevent this case in the first place can be convenient. Don't you think?

As I said before, I tried myself to have a look at the code, but I didn't find the actual part of code that created the export json. Could you give me some hint for this? @sapics

[sapics]

a bit hacky

Yes :)

And I don't know that this behavior is intended or not.

In my two cents, I use importJSON only for loading from initial project, so, I have no problem.
And I don't want to save all properties of item, because it makes much larger JSON file.
It might be better to save by option.

As I said before, I tried myself to have a look at the code, but I didn't find the actual part of code that created the export json. Could you give me some hint for this?

I guess that it would be done when we remove the lines
https://github.com/paperjs/paper.js/blob/develop/src/item/Item.js#L185,L186,L190
Sorry, I have not tested.

[simonemazzoni]

And I don't want to save all properties of item, because it makes much larger JSON file

You are totally right about this, but I guess that since visible is a key property of the path, it is mandatory to export this property to maintain consistency.

That property is in fact exported when set to false. The thing I don't get is why it isn't exported if set to true? This is the thing that messed all up :)

[sasensi]

I think the main reason behind this issue is that some properties are only exported if they are different from default value, visible is one of those but we can build up a similar example with almost any other one. See for example this sketch using fillColor.
I think this make sense because exportJSON()/importJSON() were originally made to be used at project level where importing means instantiating objects from scratch. In that case, not exporting default values cause no problem and saves space, as @sapics pointed out.
But the case of exporting from/importing into an item is problematic. I see 2 possible solutions:

• keep current behaviour and clarify its limitations in item documentation
• do a backward incompatible change that would internally use a replaceWith like trick to rebuild the item from scratch when importing

@lehni, what do you think ?

[simonemazzoni]

I think the main reason behind this issue is that some properties are only exported if they are different from default value.

That explains the behaviour, which makes sense in general.

• keep current behaviour and clarify its limitations in item documentation

This would help in any case. Since it isn't that easy to understand just trying.

• do a backward incompatible change that would internally use a replaceWith like trick to rebuild the item from scratch when importing

That will possibly break older logics causing errors so I would propose a slightly different approach. The approach consists in adding another param in the importJSON function to use a replaceWith under the hood.
So let's say existingPath.importJSON(json, cleanImport) where cleanImport is a boolean param that force replaceWith to be called inside.

What do you think @lehni @sasensi

[sasensi]

@simonemazzoni, your proposal seems good to me 👍

If @lehni is ok, I'll implement that.

[sapics]

@simonemazzoni Sorry, my previous answer was not good. existingPath.importJSON(json, cleanImport) looks good for me, too.

[simonemazzoni]

@sapics no problem, I'm happy it helped improve Paper.js!