baylej / tmx

C tmx map loader
http://libtmx.rtfd.io/
BSD 2-Clause "Simplified" License
242 stars 54 forks source link

tmx_object doesn't distinguish which attributes were or were not present in the XML, which is information we would use to determine whether a template property has or has not been overridden by the instance #72

Open dennisferron opened 1 year ago

dennisferron commented 1 year ago

Based on my reading of libTiled in the Tiled codebase, the following properties may be defined in a base template and optionally overridden (or not) by a map object instance: name, gid, width, height, rotation, visible. If that attribute is not present in the XML, we have to look at the template (template_ref in libTMX) to get the value.

In libTMX the member of tmx_object retains its default value (usually 0 or NULL, but 1 in the case of visible) if the attribute wasn't parsed from the XML. The problem comes in when this default is within the range of values that could have been explicitly set by an attribute that is present. Code using libTMX cannot tell if the intention of an object property matching the default was to inherit the base value of the template property or to override it back to a value that happens to match the default.

(There's potential for confusion here because the technical term for these in XML is attribute, but the Tiled code seems to call them properties - not to be confused with custom properties. I will use the word "attribute" when talking about the XML input, and "property" when talking about the tmx_object.)

The simplest example of the ambiguity is the rotation property. If an inherited template sets a nonzero rotation, an instance with a rotation="0" attribute cannot be differentiated by a user of libTMX from an object that has not provided that attribute at all. So we will not know whether to use the zero or use the base template's value.

Visibility has the opposite problem. If the template is set to not visible (0), but the instance overrides that to 1, that (post XML load) is indistinguishable (in the tmx_object) from if the instance in the XML had not provided the attribute at all. Both cases will result in the tmx_object visibility being "1", but we don't know if that "1" means change it to visible or default to template value.

There are some cases which are distinguishable. Obviously the name property being a string can be null thus we would look at the template. The code in libTiled will not write the width or height property if they are zero, so we can safely assume if libTMX has zero here it probably did not read that from the XML and the inherited template value should be consulted. X or Y can be explicitly zero, but those properties aren't written by libTiled on template objects. I take that to mean X and Y are not inheritable properties.

A gid of zero we can probably assume means that attribute wasn't present (instance didn't override the property), but I don't fully understand the business with firstgid and local ids and whether that can result in an intentional zero value?

In summary, the problem is we need to be able to distinguish default property values from explicitly set ones in order to properly handle templating from user code when using libTMX. There are work-arounds for name, gid, width, and height, but there's not a work-around for visibility or rotation.

One possible way to fix this would be for libTMX to resolve the values for properties from the template itself (when the attribute isn't present on the instance during XML parsing), and fill in those inherited values in the tmx_object. That would certainly make the library easier to use and satisfy the Law of Least Astonishment. (What sent me down this rabbit hole was my not expecting a tile object gid to zero; I had to spelunk with the debugger to see the problem was the gid was only set in a template and not the instance.)

Another possible way to fix it would be to maintain a separate collection of flags saying whether the property has been changed or not. This is the approach libTiled uses. (Actually libTiled also copies values into instances but uses changed flags to not write the attribute when not changed.) In libTMX there would be a changed_flags member of tmx_object, and it would be set by the XML loading code.

Which should be preferred depends on whether the intention of libTMX is to produce objects directly usable by rendering code (which would favor the pre-lookup approach), or if it should represent what's in the TMX file with minimal (re)interpretation. It makes no difference to me which way that goes, since for my game I translate the libTMX objects into my own custom classes anyway.

P.S. I hope none of the above comes across as complaining or demanding of a feature. This is the best TMX loading library I've found, and I'm glad there's something tracking current Tiled file versions. If it didn't exist I'd have to either use something old and lacking more features, or I would have to fork libTiled and do the substantial work of stripping all the Qt dependencies from it to get something I could drop in to my project.

ForeverZer0 commented 1 year ago

I just ran into this issue myself in my current C project. As was correctly pointed out, simply loading the template and referencing it within the object is inadequate in nearly all cases. This unfortunately makes templates far less useful than they actually are. With the exception of perhaps the user who created the map, who knows explicitly which attributes need to be overridden and which not on a per-map and/or per-object basis to account for it, they are actually rendered somewhat useless.

A while back I was using the TMX format in a C# project where I had implemented my own loader, and solved this by not even bothering to expose templates to the user in any meaningful way. You could check if an object had one or not and inspect it, but the final deconstructed object was a complete object with its values reflecting what it inherited from the template with its own changes applied, no need to even check or worry about where those values came from to the person using the loader.

When loading an object, the first property to check was the presence of a template. Afterwards, when there was a missing attribute for the object, it assigned the value from the template instead. This was a very easy-to-implement and robust solution for a "read-only" implementation. though would have obvious caveats if writing was implemented, and some more complexity if using a forward-only parsing method, neither of which apply to this project.

While I do have my hands a in full right now, so it might be a few days, but I would be willing to implement such a solution if this project is still taking PRs, as I rather need to have this working correctly in my current project.