go-tiled
go-tiled copied to clipboard
lafriks
Review all default values
I've noticed that in some cases you define default values for the struct interface. For example in the text unmarshal function
type Alias Text
item := Alias{
FontFamily: "sans-serif",
Size: 16,
Kerning: true,
HAlign: "left",
VAlign: "top",
}
What is the purpose of those defaults? I would like to use go-tiled as a tool to read/write Tiled files and these defaults may cause weird situations if they differ from what Tiled defines.
My preference is to display what is defined in the .tmx/.tsx files without modifications or assumptions. providing an easy interface for special values like layer tiles.
Thank you
Default values are defined in TMX file specification and are not dependant on editor. See https://doc.mapeditor.org/en/stable/reference/tmx-map-format/#text
I see, The point is that looks like the code is not up to date.
Looking only at map defaults, it is defined right-down for RenderOrder, but other defaults are missing
To list a few...
- BackgroundColor -> Default transparent
- nextLayerId -> defaults to the highest layer id in the file + 1
- nextObjectId -> defaults to the highest object id in the file + 1
- infinite -> default 0
item := Alias{
loader: m.loader,
baseDir: m.baseDir,
RenderOrder: "right-down",
}
Should we update all the default values?
Yes, if any default is missing we should update it as per TMX specification
Maybe we can turn this issue into a task "Review all default values"
I noticed this is what is affecting objects not having some values set, due to a lack of Unmarshal method.
Anyways I can't offer up anything concrete yet, only been working with Go for the last....month I guess? Still learning a lot of the inner workings and core library stuff. Going to dig into the xml parsing stuff to get my head wrapped around that.
Fundamentally it seems to make sense where the defaults are set, what I would think would help is having all of those defaults preset into a single file that the respective Unmarshal methods draw from. In this way at least updating the defaults to match the current Tiled spec will be a one stop update. Also ensuring that each type of Tiled item that is represented has an appropriate Unmarshal method defined.
Upon reading through the TMX map format spec, I do notice that some values it indicates have a default value, but these values might not need to have defaults set, as they are generated by the editor and from what I can tell, always present. The nextlayerid and nextobjectid would be examples of that. The editor calculates that on file load if it's missing, and always sets it in the saved file. Additionally there are some values which are effectively useless outside the editor.
The biggest targets are the values which aren't written into the XML if they are the default value. The visible property is a big one on this front, as if it's visible, it doesn't specify that field in the xml, and if it's set to false, then it is specified in the xml. Since right now there is no Object Unmarshal, the visible property is reading as always false. Granted by my own logic, it should be not present at all...not simply reading false.
While writing this I gave myself a crash course in the XML parser items....I may just be able to find a way to contribute....at any rate this issue is a roadblock on my own project I don't want to create a workaround for, so I know I'll look into figuring something out here.
Hi @Qwarkster ! Welcome to the amazing Go community
The visibility field is a clear good example. I also found that the editor doesn't write it when the value is set to the default one. We are currently having a discussion about how to load/save files here.
in a few words, In the current implementation, visibility is defined as a bool. This only yields 2 possible states (true/false). If we want the state undefined, we can define visibility as *bool. This way you have nil for undefined. The same logic applies for any field if we need to tell the difference between defined and undefined.
I would be wary of allowing fields to be undefined, as that feels like an unnecessary complication, in particular for bool fields. It's good practice to accommodate a vast majority of use cases, but using reasonable common sense in some cases makes sense too. Using visible as an example, it represents whether something is shown or not shown. There's no conceptual "in between" in how it functions.
So initially what needs to be identified is the fields where if they are not present, will break the loading or saving process. Fields which have a default of 0 or empty strings, or false values we may not need to concern ourselves with, if the absence of these fields doesn't break anything.
I've have some other ideas I'll share on the relevent thread about the saving process....but what simply should be done so as to not complicate anything is ensure that for each type of element (objects, layers, etc.) all the functional defaults are prepared so nothing unexpected happens when loading. The idea being that if you didn't change the setting when you made the map, you are probably not too concerned as to what it's set to, as long as it loads and functions properly.
I'll probably tinker sometime very soon with saving a very simple map with all the defaults left....see what my output is, compare to the xml spec, and see what should be considered and set in loading. Just blanket covering all default values may not be necessary or even wise.
I'll reiterate again that the other aspect that should be figured out, is since these default values are set in the various Unmarshal functions, having those functions draw from a common file would be helpful, so updating the defaults to match a tiled spec update is easily done in one place. While the overall concept might seem a little error prone, there's a little "if it ain't broke don't fix it" going on. Centralizing the defaults is the balancing act against the possibility of errors, such as missing one value in one file.
We could create interface with method func WithDefaults(interface{} obj) to get object with defaults that could be implemented in separate file for all types and thus called in unmarshal and wherever needed.