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

Add "Repeat ..." properties to Image Layers #3205

Merged
merged 7 commits into from
Dec 13, 2021
Merged

Conversation

krukai
Copy link
Contributor

@krukai krukai commented Dec 8, 2021

Introduce the boolean properties "Repeat Horizontally" and "Repeat Vertically" to Image Layers. This mirrors GamerMaker's "Horizontal Tile" and "Vertical Tile" options.
The YY plugin will export these properties accordingly. For backwards compatibility however, the respective custom properties "htiled" and "vtiled" take precedence over the built-in ones.

I am currently still tinkering with the drawing to actually behave as described in the commit message. I may also have to touch the changes for the bounds again. There is also some room for more consistency I still want to tackle.
Finally, I am planning to move these two new properties above "Offset X", if that feels right to you as well.

Until then, feel free to review the rest. I hope I did not overlook anything serious; I only started working with the source code yesterday.

Cheers!

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this feature! I did no testing yet, but left some comments on the changes. :-)

src/libtiled/imagelayer.cpp Outdated Show resolved Hide resolved
src/libtiled/imagelayer.h Outdated Show resolved Hide resolved
src/libtiled/mapreader.cpp Outdated Show resolved Hide resolved
src/libtiled/maprenderer.cpp Outdated Show resolved Hide resolved
src/libtiled/mapwriter.cpp Outdated Show resolved Hide resolved
src/tiled/changeimagelayerproperties.cpp Outdated Show resolved Hide resolved
src/tiled/editableimagelayer.cpp Outdated Show resolved Hide resolved
@krukai
Copy link
Contributor Author

krukai commented Dec 8, 2021

I resolved most of the review comments.
Originally, I wanted to move the properties above Offset X and Y because that would feel natural for Image Layers, IMHO. However, that would require to insert properties into the overall layer group of properties. I am not sure if we want to do that.

I also still have to tinker with the actual drawing. It works fine until offsets are set, but then the drawing rectangle would have to be adjusted to fill the space created by the offset within the overall bounds. I am not entirely sure yet where to make the adjustments, but I will look into it further.

@bjorn
Copy link
Member

bjorn commented Dec 9, 2021

Originally, I wanted to move the properties above Offset X and Y because that would feel natural for Image Layers, IMHO. However, that would require to insert properties into the overall layer group of properties. I am not sure if we want to do that.

In the end the attribute order does not matter in XML, and I think it's fine to have the repeat attributes follow after the parallax ones.

I also still have to tinker with the actual drawing. It works fine until offsets are set, but then the drawing rectangle would have to be adjusted to fill the space created by the offset within the overall bounds. I am not entirely sure yet where to make the adjustments, but I will look into it further.

Yes, to handle the offset you'd need to call QBrush::setTransform with a transform created with QTransform::fromTranslate.

@bjorn
Copy link
Member

bjorn commented Dec 9, 2021

Ah, handling the offset will be a little more complicated, because currently that's done by simply moving the ImageLayerItem around.

Hmm, is it really important that when repetition is used, the layer is clipped to the map boundaries? It's rather inconsistent with the way image layers have worked so far, and actually nothing else is clipped to the map boundaries. If it would be alright if the layer expands beyond the map boundaries then we'd just need to increase the bounding rect of the ImageLayerItem to be practically infinite.

@krukai
Copy link
Contributor Author

krukai commented Dec 9, 2021

In the end the attribute order does not matter in XML, and I think it's fine to have the repeat attributes follow after the parallax ones

I was referring to the properties panel. My bad.

Hmm, is it really important that when repetition is used, the layer is clipped to the map boundaries? It's rather inconsistent with the way image layers have worked so far, and actually nothing else is clipped to the map boundaries. If it would be alright if the layer expands beyond the map boundaries then we'd just need to increase the bounding rect of the ImageLayerItem to be practically infinite.

Would infinity actually be acceptable here or would that screw with other features relying on the bounds?

While I see your point, I would prefer if it was cut off at the overall bounds at least optionally, assuming we would not have to jump through too many hoops to achieve that. Otherwise, the overall view would get extremely noisy and it might be difficult to see at a glance where the actual level bounds begin.
But I realize while typing this that arguing for allowing an inconsistency here is pretty close to arguing for a "show me what the player would see" mode, which would include cutting off images with an offset but no repetition as well. This would also help when fiddling with parallax parameters.
So perhaps this would be a feature on its own and not within scope here.

@bjorn
Copy link
Member

bjorn commented Dec 9, 2021

Otherwise, the overall view would get extremely noisy and it might be difficult to see at a glance where the actual level bounds begin.

Right, I think visualizing the map boundaries more effectively is a different problem. Maybe darkening the area outside of the map would help, for example.

But I realize while typing this that arguing for allowing an inconsistency here is pretty close to arguing for a "show me what the player would see" mode, which would include cutting off images with an offset but no repetition as well. This would also help when fiddling with parallax parameters.
So perhaps this would be a feature on its own and not within scope here.

Yeah, this really sounds like issue #2962.

Btw, another option to consider, is to add a repetition mode to tile objects. They already have a size that would naturally be used to clip the repetition. Generally image layers are mostly an inferior version of the tile objects, with their only advantage being (at least until this PR) that you did not have to first add the image to a tileset.

@krukai krukai force-pushed the master branch 2 times, most recently from 1d53803 to bd583f3 Compare December 9, 2021 17:24
@krukai
Copy link
Contributor Author

krukai commented Dec 9, 2021

Added scripting support (I hope). Everything should work as expected with "infinite" repeating now, I hope.

I also refactored the Use the same pattern as ChangeImageLayerProperties as requested, albeit in a separate commit. I am also leaving renaming the class to singular to another commit as otherwise the diff would not be very useful. If you want to squash any of that, please let me know.

That being said, is there anything else to take care of? Do my changes warrant modifying some header comment or anything?

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some minor issues and a bug that need addressing, but I think this will be the last review! :-)

src/tiled/changeimagelayerproperties.cpp Outdated Show resolved Hide resolved
src/tiled/changeimagelayerproperties.cpp Outdated Show resolved Hide resolved
src/libtiled/maprenderer.cpp Outdated Show resolved Hide resolved
src/libtiled/mapwriter.cpp Outdated Show resolved Hide resolved
src/tiled/changeimagelayerproperties.cpp Outdated Show resolved Hide resolved
Introduce the boolean properties "Repeat X" and "Repeat Y" to Image
Layers. The YY plugin will export these properties accordingly as
"Horizontal Tile" and "Vertical Tile".
For backwards compatibility however, the respective custom properties
"htiled" and "vtiled" take precedence over the built-in ones.
Use the same pattern as ChangeMapProperty. Renaming the class
accordingly will happen in a future commit.
The respective values are only added if they are true.
@krukai
Copy link
Contributor Author

krukai commented Dec 11, 2021

I implemented all review comments, updated the documentation, and added support for JSON and Lua export.
Please just squash everything into one commit; I just kept the latter commits separate for easier reviewing and at this point, I do not think it makes much sense to separate anything into individual commits.

If there is anything else, let me know!

* No braces for one-line bodies
* Added "since Tiled 1.8" and mentioned repeat attributes in TMX and
  JSON changelogs
@bjorn bjorn merged commit 0e7f813 into mapeditor:master Dec 13, 2021
@bjorn
Copy link
Member

bjorn commented Dec 13, 2021

@krukai Thanks a lot for implementing this very welcome feature!

@krukai
Copy link
Contributor Author

krukai commented Dec 13, 2021

Thank you! My bad for missing the changed filename in the comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants