mapeditor / rs-tiled

Reads files from the Tiled editor into Rust
https://crates.io/crates/tiled
MIT License
268 stars 102 forks source link

"Error parsing optional attribute 'visible'" while parsing an older Tiled map (version 1.4) #317

Closed gzhynko closed 2 weeks ago

gzhynko commented 2 weeks ago

Trying to build a map parser that deals with Stardew Valley's maps, and it appears that some maps have a "visible" attribute within <objectgroup> like in the following example:

<objectgroup id="6" name="Buildings" visible="false" locked="false" />.

This does not seem to work well with rs-tiled, as it gives the following error:

MalformedAttributes("Error parsing optional attribute 'visible'").

The issue is not there when I remove this attribute. I understand these maps are built with an older version of Tiled, but maybe there's a way to skip errors like these during parsing to just get things like layers and tileset? Thanks in advance for any help!

bjorn commented 2 weeks ago

Hmm, these maps are likely written by an entirely different tool rather than an older version of Tiled. Not even Tiled itself supports visible="false" locked="false" (the layer would just show up as visible, since it can't parse false as an integer). So that's an issue in the relevant tool that would be better to fix there instead.

I guess you're using StardewXnbHack to export the maps. It already makes sure to write out 0/1 instead of true/false for tile layers and image layers:

https://github.com/Platonymous/TMXTile/blob/aecccf41ee868b5e5c8b64a60366a6e5c0c819cd/src/TMXTile/Map/TMXLayer.cs#L27-L34

But the code for object groups does not:

https://github.com/Platonymous/TMXTile/blob/aecccf41ee868b5e5c8b64a60366a6e5c0c819cd/src/TMXTile/Map/TMXObjectGroup.cs#L26-L29

I'll open a PR to propose fixing TMXTile.

gzhynko commented 2 weeks ago

Thank you for the quick response! I didn't realize this might be an issue with the exporter, sorry for the issue! I tried running the export with the fix you proposed, and it seems to be working properly now.

One other thing I noticed is that TMXTile exports capitalized boolean values ("True" instead of "true", default behavior in C#) for properties (e.g. <property name="ViewportFollowPlayer" type="bool" value="True" />), which results in InvalidPropertyValue { description: "provided string was not 'true' or 'false'" } from rs-tiled. Should this be something that also gets fixed on the TMXTile side?

bjorn commented 2 weeks ago

Should this be something that also gets fixed on the TMXTile side?

Ideally yes, because "true" and "false" are the only documented valid values for boolean properties and I expect many frameworks to only support those.

I wouldn't really mind making these checks case-insensitive in rs-tiled, though. It seems that while Tiled would only ever write "true" or "false", it is actually more flexible in which values it accepts here, though this wasn't a conscious choice, it is just because it relies on QVariant to convert the string to a boolean value.

But if it can be easily fixed in StardewXnbHack then this should be preferred.

gzhynko commented 2 weeks ago

Got it, thanks for clarifying. I opened a pull request on TMXTile to convert the boolean values to lowercase, hopefully the project is still open to contributions so these PRs can get merged. Thanks again for the help!