Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom output chunk size can now be specified in the map properties #2130

Merged
merged 4 commits into from Jul 10, 2019

Conversation

@RPGHacker
Copy link
Contributor

commented May 17, 2019

No description provided.

@RPGHacker

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

This pull request would close #2121.

@bjorn
Copy link
Owner

left a 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?

src/libtiled/map.h Outdated Show resolved Hide resolved
src/libtiled/mapreader.cpp Outdated Show resolved Hide resolved
src/libtiled/maptovariantconverter.cpp Outdated Show resolved Hide resolved
src/libtiled/maptovariantconverter.cpp Outdated Show resolved Hide resolved
src/libtiled/mapwriter.cpp Outdated Show resolved Hide resolved
src/libtiled/varianttomapconverter.cpp Outdated Show resolved Hide resolved
src/libtiled/varianttomapconverter.cpp Outdated Show resolved Hide resolved
src/plugins/lua/luaplugin.cpp Outdated Show resolved Hide resolved
src/tiled/changemapproperty.cpp Outdated Show resolved Hide resolved
src/tiled/changemapproperty.h Outdated Show resolved Hide resolved
@RPGHacker

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

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.

@bjorn

This comment has been minimized.

Copy link
Owner

commented May 28, 2019

@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?

@bjorn

This comment has been minimized.

Copy link
Owner

commented May 28, 2019

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.

@RPGHacker

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

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.

@bjorn

This comment has been minimized.

Copy link
Owner

commented May 28, 2019

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.

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)?

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.

@RPGHacker

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

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.

Well, it's a bigger undertaking, but it avoids any backwards-compatibility issues.

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"?

@bjorn

This comment has been minimized.

Copy link
Owner

commented May 28, 2019

But if the settings move from the element of each layer to this new element, won't that introduce an incompatibility? Or would the idea be to still check for the old attributes in the 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.

@RPGHacker

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

(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:

...
  <editorsettings>
    <chunksize width="16" height="16"/>
    <tilelayerformat encoding="base64" compression="zlib"/>
  </editorsettings>
...

Tiled would set its compression to "zlib", and when opening a .tmx file with the following contents:

...
      <data compression="zlib">
         ...
      </data>
...

Tiled would also set its compression to "zlib". So effectively, it would just keep whatever was read last, right?

@bjorn

This comment has been minimized.

Copy link
Owner

commented May 28, 2019

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.

@RPGHacker

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Alright, thanks for the clarification. Will give it a try. :)

RPGHacker and others added 3 commits May 29, 2019
Some tweaks
* 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.
Further tweaks
* 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.
@bjorn

This comment has been minimized.

Copy link
Owner

commented Jul 10, 2019

Alright, after some tweaks I think this is fine to merge. I'll look into introducing the "editorsettings" section as a follow-up change.

@bjorn
bjorn approved these changes Jul 10, 2019

@bjorn bjorn merged commit e297ae7 into bjorn:master Jul 10, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
WIP Ready for review
Details
bjorn added a commit that referenced this pull request Sep 8, 2019
Moved output chunk size into "editorsettings" section
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
@bjorn

This comment has been minimized.

Copy link
Owner

commented Sep 8, 2019

Last week I introduced the <editorsettings> section and now I've finally moved this information there rather than repeating it on the <data> elements. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.