Custom output chunk size can now be specified in the map properties #2130
Conversation
|
This pull request would close #2121. |
There was a problem hiding this comment.
I've left a bunch of comments, mostly about some details.
Overall though, I think the main problem with the patch in its current form is that it always saves the chunk size. This way, the format ends up with duplicate information since each chunk also saves its size. In addition, it suggests that chunks of varying sizes will not occur, even though that's a mode that I considered supporting in the future.
But, maybe there's no real reason to support that. Maybe we should deprecate the size attributes on each chunk in favor of the new attributes at the tile layer level?
|
Whoopsie about the tabs. Something Visual Studio does by default. Thought I had converted them all to spaces, but seems like I missed some. Will add all of the suggested fixes later today. As for the problem of ambiguity: the simplest "solution" I can think of would be to simply rename the new attributes I added to "outputchunkwidth" and "outputchunkheight" respectively. That would make it clear that those attributes only refer to the output format specifically, whereas the sizes on each chunk refer to the input sizes. The alternative would be to drop the attributes entirely and just set this size based on whatever chunk sizes are found in the input file, but yeah, that would get problematic as soon as differently-sized chunks were present in any given input file. I think differentiating between "input" and "output" attributes, as described above, would be the more robust solution here. |
|
@RPGHacker I think that rename is not a bad solution. Ah, the other thing I don't really like is that we're introducing another property that in the UI is presented on the map, but in the saved format is put on each layer. I know this is the case for the compression as well. What do you think of either moving these settings up in the TMX hierarchy or having these values on each TileLayer instance? |
|
I'm personally thinking of having a configuration section right after the map element, like: <map ...>
<editorsettings>
<chunksize width="16" height="16"/>
<tilelayerformat encoding="base64" compression="zlib"/>
</editorsettings>
...
</map>This would make it clear that some things are set in the editor on a map-basis, and at the same time that these things can be ignored by readers in general. |
|
Haha, ironically, having the property in the layer properties was actually how I solved it originally, but then moved it to the map properties since you suggested taking a look at the encoding format for reference. Since that one is edited via the map properties, I also moved this property there. I can change that, though, no problem. Yeah, the editorsettings element sounds like a reasonable solution. Do you want me to try implementing it, or would it be okay to keep it as something to add down-the-line (and just have the property in the layer settings for now)? Adding this element seems like it would be a slightly bigger undertaking, since things like backwards-compatibility and supporting all the different formats would have to be considered. |
Well, it's a bigger undertaking, but it avoids any backwards-compatibility issues.
Well, at least it would need to happen before releasing Tiled 1.3. Doesn't need to be part of this pull request though. But there is no need to move those settings to the layer properties if we're going to have them on the map in the end anyway. |
|
Alright, I'll leave them in the map properties for now and implement all the other suggestions, then I can try implementing the new element in a another pull request.
But if the settings move from the <data> element of each layer to this new <editorsettings> element, won't that introduce an incompatibility? Or would the idea be to still check for the old attributes in the <data> element, "just in case"? |
The idea was to explicitly mark this section as "editor settings" and not to change the rest of the format in any way. So the editor settings do not influence the way a map is loaded, apart from that those settings are read and may affect other things, in this case how the map is saved. I could imagine grid rendering options to become part of this section as well. In addition, I think in Tiled 1.4, we'll see these editor settings show up in project files, and then in map files they would only serve to override the project settings. |
|
(Whoops, just realized that my previous comment was a bit messed up because GitHub interpreted some parts as HTML) Okay, just to make sure I'm understanding correctly. After the proposed changes, when opening, let's say, a .tmx file with the following contents: Tiled would set its compression to "zlib", and when opening a .tmx file with the following contents: Tiled would also set its compression to "zlib". So effectively, it would just keep whatever was read last, right? |
Right, for backwards-compatibility reasons, we need to keep setting that value also based on those attributes, but I'd say when an "editorsettings/tilelayerformat" element is present, only the value specified there applies to the map. |
|
Alright, thanks for the clarification. Will give it a try. :) |
…her changes suggested by bjorn
* Reduced code in TMX and JSON map readers * Increased minimum chunk size slightly * Removed tabs and other small code style changes * Renamed from "Tile Layer Chunk Width" to "Output Chunk Width" since I think it's important to communicate that this has nothing to do with the chunk size used internally.
* Only write out "outputchunkwidth" and "outputchunkheight" for infinite maps for now, since chunks are not used for non-infinite maps. * Reduced code indentation in sortedChunksToWrite a little.
|
Alright, after some tweaks I think this is fine to merge. I'll look into introducing the "editorsettings" section as a follow-up change. |
For both the TMX and the JSON map formats. For the Lua format, the this information was simply removed since each chunk already specifies its size and this additional information was only for the editor. Lua files are currently not readable by the editor. Finalizes issue #2130
|
Last week I introduced the |
No description provided.