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

Consistency on layer type and object type detection #1922

Closed
bitinn opened this Issue Mar 29, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@bitinn
Copy link

bitinn commented Mar 29, 2018

I was writing a parser and serializer for JSON map in C# for Unity, and found some inconsistency on how layer type and object type are handled.

  • For example, if a layer contains both chunks and data then Tiled Editor will say that layer is corrupted.

  • But in another case, if an object contains both gid and text, then Tiled Editor will take gid with precedence, basically display it as a tile object. Furthermore if gid is 0 then it would be ignored.

The data for both cases are semi-invalid, so I am not expecting miracle. But maybe Tiled can establish an order of precedence for attributes, so that invalid data can fallback consistently? Like ignoring data when chunks and infinite: true is present?


By the way, I am ran into this issue largely because I am using the less-than-great Unity built-in serializer, it not only doesn't support null on deserialize, but also doesn't ignore null value on serialize. Which means if I have both data: null and chunks: [{}, ...] field, it will always serialize to both data: [] and chunks: [{}, ...] in JSON, which leads me to corrupted layer.

At this point I am preparing to dump Unity serializer so my issue could very well be a non-issue. Just something people should watch out for...

@bitinn

This comment has been minimized.

Copy link

bitinn commented Mar 30, 2018

A even more difficult problem is object type for point and ellipse, they are both boolean flags, which is not nullable in C#.

And point: false will still convert the object into a point object, for example this object:

                {
                    "x": 512,
                    "y": -1280,
                    "height": 0,
                    "width": 0,
                    "rotation": 0,
                    "id": 7,
                    "name": "",
                    "type": "",
                    "gid": 0,
                    "ellipse": false,
                    "point": false,
                    "polyline": [{
                            "x": 0, "y": 0
                        }, {
                            "x": 0, "y": 224
                        }, {
                            "x": 320, "y": 224
                        }, {
                            "x": 320, "y": 448
                        }],
                    "visible": true
                },

will be a displayed as a point object instead of polyline.

I haven't looked at the code directly, but I think the precedence is something like:

gid > point > ellipse > other object.


Basically if I want to serialize current object format, I will need:

  1. a more mature and full feature serializer like Json.NET

  2. write a custom converter that allow me to have true -> true or false -> null/skip field, instead of the default true or false serialization.


An alternative: just use object instead of boolean for ellipse and point attributes, this will cause boxing of values, but at least we can now serialize it correctly (if point/ellipse is null then we don't serialize it).

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Mar 30, 2018

And point: false will still convert the object into a point object

This is a bug. It is caused by the assumption that when an object is not a point, then there will not be a point property. The Tiled loading code doesn't even bother to check whether the value is true, but it really should since the behavior in your scenario makes no sense.

I'll be sure to look at improving the data / chunk code as well to improve the situation, though due to Easter holiday and a business trip next week I may not have time for this until Friday 6th.

@bitinn

This comment has been minimized.

Copy link

bitinn commented Mar 31, 2018

It might be a bit late to suggest this: Is object.type going to be used to identify object type?

Feels like it's there for the same reason as layer.type but somehow isn't used.

(I am basically checking gid, point, ellipse, polyline, polygon to identify what object type it is in my game's code, would be great to just check type.)

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Apr 12, 2018

A bit later than I intended, but this issue is now addressed in the commit above.

It might be a bit late to suggest this: Is object.type going to be used to identify object type?

Feels like it's there for the same reason as layer.type but somehow isn't used.

Well, in fact there is already object.type, and it is the custom string value that you can type into the "Type" property of the object. This naming conflict is indeed rather unfortunate. In the Lua export I decided to add a "shape" property, but this is also not ideal.

I'm not sure what would be a better solution here. It's definitely something to keep in mind for future format changes.

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Apr 12, 2018

Ah, I forgot to look in the first issue (having both data and chunks). Will try to get to that soon.

@bjorn bjorn closed this in 31fe0ef Apr 18, 2018

bjorn added a commit that referenced this issue Apr 24, 2018

JSON plugin: Ignore a "data" member of a tile layer when it is "null"
This may help some people who are having problems avoiding the presence
of members due to the JSON library they are using.

See issue #1922
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Apr 24, 2018

I pushed another small change, to not raise data corrupted error when there is a data: null entry. I hope this helps you avoid problems there. Though data: [] still raises an error (since I was not sure how to ignore this... it looks wrong to me).

@bitinn

This comment has been minimized.

Copy link

bitinn commented Apr 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment