Save/Load Tileset

[MetalBlueberry]

I have been working on a tool to automatically turn a folder of frames into a Tileset. For this purpose, I've modified the structures in this package to have the "omitempty" flag so I can write files similar to those generated by Tiled. Although this is working fine for most of the fields right the way, I had to make a few important changes

• avoid nested structures like Properties Properties `xml:"properties>property"` replacing it by an intermediate structure.
• if the field is an structure > use a pointer to allow zero values
• if zero value == default value > just add omitempty
• if zero value != default value > use pointer to differentiate between not defined and other value. (only applies to visibility and kerning)

I've tested TilesetTile and Tileset in different blocks because TilesetTile can be really complex so though that testing it in two steps could be easier to understand.

• The Load* tests compare the structure with the one stored in the test
• The Save* tests compare that after loading/saving, the file xml content is the same.

[lafriks]

Other option could be implement custom MarshalXML functions for types that contain Visible or Kerning as currently not all Visible properties are changed.

[MetalBlueberry]

I see your point, but is it worth to add custom Marshal/Unmarshal functions? It will require to manually assemble all the other fields and can be error-prone.

Using pointers when Unmarshaling only requires to set the default value as we are already doing. When marshaling it requires you to specify the value that you want to print, and if you do not define it, it simply ignores the field.

I know that not all the Visibility values have been modified to use pointers, this is because I wanted to keep the scope of the merge request as small as possible. Basically, I would like to perform only the necessary modifications to Marshal/Unmarshal Tilesets. But thinking it again, maybe it makes sense to modify all the booleans with default true now to keep consistency.

PD: I'm not on holidays anymore, so I will slow down my contributions ;)

Thank you for your time.

[MetalBlueberry]

weekend again! what can I do to keep this moving?

[lafriks]

I don't quite like changing bool to pointer so imho custom xml encoder would be better

[MetalBlueberry]

What do you think about following the pattern of other packages to handle nullable fields?

For example, in aws cli you can find the String function or the Bool function that basically converts a value to a pointer. Then, they use pointer in the structures that will be serialized.

To follow this approach, we just have to replace the values with pointers and create the helper functions. Although the functions are not really needed.

[lafriks]

Yeah I have seen them but they are useful in cases where bool can acutally be nil, true, false. In TMX bool can not be xml:nil, it will be either true/false or if absent (default value). So we should build API on same principle imho with custom encoder

[MetalBlueberry]

I understand that absent means the default value. I think that the key difference is that with the current implementation you can not tell the difference between the default value set on purpose or the default value because it was missing.

Maybe we can do this in two steps. We could define the structures with pointer references so we can track if the values was defined in the original file or not. Then we provide a method fillTiledDefaults() that will replace the missing values with the defaults. This way, an user interested in working with Tiled defaults could work with this library without preventing other use cases such as generating valid Tiles files without the default values forced by the current implementation.

[lafriks]

Other point for not using *bool is that we will need to check if value is true anyway to not write value equal to default value to output. I don't see much point telling difference between default value and set value

[Qwarkster]

A simple thought on this, there's no reason to omit values from the save file if it's the same as the default value. Writing out all the field values when saving, even if they are the default values, should not create an unworkable file. Just because visible=true is default, it's not going to hurt if every relevant element has that attr set. It might appear a little more verbose in the output file, but it's likely a lot less headache than going through and deciding on a case by case basis what fields might be able to be omitted.

[MetalBlueberry]

Hey, I'm probably not going to follow up with this MR, I'm currently focused on other projects. Maybe we should just close it.

[lafriks]

Just leave it open, I will work on this later on to what I have more free time