Add option to invert Y-axis #3679
Conversation
Co-Authored-By: AdrianFL <adrianfranceslillo@gmail.com>
Co-Authored-By: AdrianFL <adrianfranceslillo@gmail.com>
Mostly just personal preference.
Conflicts: src/tiled/preferences.h
No longer using this namespace (71c1de5).
* Refactored preferences getter/setter and removed all references to `mInvertYAxis` as per a9ad617 * Used `ChangeMapObject()` instead of `MoveMapObject()` * Used `QStringLiteral` for coordinate status info * Trivial adjustments to includes and UI XML
|
Thinking more on this, this should probably be configured on the map level? The anchoring of objects on the lower-left would mean engines need to know how the user has it configured. |
Since inverting the Y-axis changes the coordinates of objects, engines will need to know how the user has this set.
|
Still WIP- I've gotten it superficially working, but it doesn't output the correct Y-value to the TMX. I could go and height-adjust that value as well, but TBH this approach makes my skin crawl; when I get home I'm going to redo this my making it loop over every object on the map and adjust their canonical Y properties every time the "Invert Y-Axis" option is checked/unchecked, and then adjust the positioning as needed in the renderer. I think that approach will be much more maintainable than this hack fest. |
Oh, boy, thar be dragons. This proved to be far uglier than I expected; the rendering logic for objects is all over the place, I had to make a way bigger mess getting this to work than I did just adjusting the Y value by the object's height in a couple of places, so I scrapped it and went back to the initial approach. I have my findings saved in a branch, but I think this is the lesser of the 2 evils. I've also adjusted the snap to grid so that it snaps to the lower-left corner while the Y-axis is inverted. I think this is ready for review. :) |
|
@vinnydiehl First of all, thanks for picking up this change again!
This is something I had really wanted to avoid, because introducing this as a map option means we're putting additional effort on generic Tiled map supporting libraries and renderers, which would now need to either be able to render in either mode, or support a conversion from one mode to another in their loader. However, since the invert Y axis option affects the object alignment, I think it is indeed inevitable. Also, this problem is mostly theoretical, because we could recommend renderers or frameworks to just support the mode that makes the most sense for them, and output an error / warning if it detects a map saved in another mode. Regarding the implementation, instead of changing the return value for |
|
@bjorn No problem! I'm in the early stages of developing some Tiled libraries for DragonRuby, you can expect to see me around here and in the Discord. :)
This is sensible for libraries that are trying to be as lightweight as possible, yes. In my development use case this option reduces complexity, but I can see how libraries that have been using top-left and now need to support inversion would be affected. Heck, even libraries that have been adjusting for bottom-left would need to update to support this. But, I think that this is a minor growing pain for a feature that future developers will be glad is there; I think the alternative of outputting different coordinates to the TMX than are displayed in the property browser is unexpected behavior, backwards-compatible as it may be. We may need to note to application users somehow that this option may not be supported by engines early on in the update. How do you usually handle this? Would you like a docs update as part of this PR?
Oh, wow, I don't know how I missed this; this is way cleaner. Handles grid snapping, so I was able to revert those hacks. A consequence of this is that the height of every non-tile object needs to be adjusted whenever the option is toggled. I needed to move |
The Y position would need to be adjusted, though I wonder a little whether that is worthwhile. I guess it would be for people who have already made their map and then realize they need to invert the Y axis (possibly once their framework asks them to do this). I'm doing no such adjustment for existing tile objects when the user changes the "Object Alignment" option of a tileset, but there it is also more difficult because that is a change to a tileset and the objects would need to be moved on the map, which might not even be loaded. Adjusting the position is a little more involved in the general case, unfortunately, since it should also take into account object rotation, and even worse, likely the map orientation as well due to the somewhat strange way rotation is applied on isometric maps. I wonder a little whether it should really be part of the |
There was a problem hiding this comment.
Reviewed the code and left mostly small feedback.
Overall I'm still reluctant about actually inverting the saved object positions. As mentioned, the logic for this is more involved than what is implemented since rotated objects aren't handled yet. If we want to keep it, the InvertYAxisHelper should probably be moved to src/libtiled (and take a Map* instead of a MapDocument*) so that it can also be used by map readers and writers.
In summary, we still need to address at least the following cases:
- Infinite maps
- Object rotation
- Isometric orientation
Regarding inverting the Y axis in general, I wonder about a few other things that it currently does not affect:
- Layer Horizontal Offset and Vertical Offset
- Map Parallax Origin
- Tile Collision Objects (impossible to invert Y axis for those currently, since the option is on the map... it would have to be available for tilesets as well)
- Tileset Drawing Offfset
Considering everything, I'm still not convinced it's meaningful to go through with this feature and think it might just be easier to deal with the different Y axis in the framework or game.
| // Obtain the map document | ||
| if (mMapDocument->map()->invertYAxis()) { | ||
| const MapRenderer *renderer = mMapDocument->renderer(); | ||
| QRect boundingRect = renderer->boundingRect(QRect(QPoint(), mMapDocument->map()->size())); |
There was a problem hiding this comment.
I think this should just call renderer->mapBoundingRect(), which is the same thing for fixed-size maps.
For infinite maps, it seems to me like in that case the origin should just stay where it is, and the only thing we want to do is to negate the Y value, so it should probably be handled explicitly (also in InvertYAxisHelper::tileY).
Another issue with this is actually that it is completely meaningless for isometric maps, since it will take the map screen pixel height and subtract the non-projected object position. I'm not sure how we can handle isometric maps properly in the case of "invert Y" actually, we may need to add a mapPixelBoundingRect (or just mapPixelHeight) function to the MapRenderer, since the mapBoundingRect is essentially a mapScreenBoundingRect function.
Removed vestigal whitespace modifications from development, addressed include order, removed unnecessary map property references.
|
Formatting has been addressed. You bring up some very good concerns. My initial thoughts-
Yeah, it definitely feels a little janky seeing everything jump around the map when you toggle the option, I feel as though not correcting would be inviting bug reports.
Are you suggesting creating a generalized "transform all objects on the map by X and Y" function, and then simply call that from
Yep, bugged. I'll look into this.
Bugged anchoring besides, the current behavior of this option on isometric maps doesn't make sense to me. In order to meaningfully invert the Y-axis on an isometric map, you would need to invert both axes, such that (0, 0) is at the bottom corner. The other option would be to not support Y-inversion for isometric maps, as I'm not sure sure they're a prominent use case for this feature anyway. The property could be excluded from the property browser, and it could just be always set 0 in the TMX. This would mean that suggesting engines force one orientation is out of the question, and there would have to be an adjustment period where applications users might not be able to use the option until their framework picks up support for it.
This is a definite oversight that will need to be fixed.
Wow, this is a tough one. I think including a property on tilesets is an inevitability in order to make this feature feel cohesive.
Not doing this in combination with not correcting position upon inversion would solve some of our other problems and allow this to be implemented as a preferences option- and, a global preference eliminates the need to add a new tileset property. However, I'm not sure that this would be preferable, since a user might work in different orientations across different games/engines and there would be the sub-optimal experience of opening a map that was designed in a different orientation, being confused why everything is rendering in the wrong spot, and needing to switch the orientation in order to correct it (possibly after having to research the problem). That idea aside, don't you think that not correcting the saved coordinates would be confusing from the point of view of a TMX consumer? That is, that they would need to adjust for the positional correction in the case of the Y-axis being inverted, but still need to manually correct the coordinates? By writing the changes to the object positions, the (much more confusing, especially once I fix rotation) positional correction would be baked in and the TMX data would be more "honest". This feature is definitely not as trivial as meets the eye. While it is not a big deal for frameworks to make this correction in order to render correctly, IMO it would be really nice for game developers to be able to easily corroborate the pixels that they see in Tiled with the map coordinates within their game. If their framework is making this conversion for them, great, but now the coordinates that they're working with in their engine differ from the coordinates they see working in Tiled. I'll think on this some more, hopefully we can land on an elegant solution. |
I don't think that'd work in this case since each object's needed adjustment will depend on its size and rotation, but I think a generalized function to "change objects alignment to X while keeping them visually in the same place" could be helpful and might be nice also outside the context of changing an "invert Y axis" function. In general I wonder a little whether, if this option turns out to be too gnarly, just the option to change the default object alignment from TopLeft to BottomLeft might suffice (or whether it would be useful as separate option regardless), since the rest of the "invert Y axis" option is just about converting numbers but this alignment option affects the visual behavior.
Hmm, yes, good observation. There have been questions about the option to choose a different origin for isometric maps, though I think it's been more about having it on the left instead of the top, and not about putting it at the bottom. So I think it might be fine if for isometric maps, the "invert Y axis" will put the origin on the left, actually.
Unfortunately, this will create situations where you have a map with inverted Y axis using a tileset without inverted Y axis, or the other way around. Of course since in memory the values aren't actually inverted it should work fine, but it does get somewhat confusing. Rather than having the option in the preferences as with the old change, we could also consider putting it in the Project. That makes it easier to switch between projects that use a different Y axis, but of course it does still leave the behavior broken if you happen to open some map that wasn't made in the same project. It also means we can't immediately correct object positions, since we don't know if the user is changing the project alignment to fix the way the map is displayed, besides not all maps in the project being open. (again, I'm thinking it might be easier to have the object alignment as completely separate option)
I agree you'll generally want the in-game coordinate system to match the one in Tiled. That said, I expect that generally frameworks will be able to adjust much more easily to Tiled than the other way around. Most frameworks would not need to actually convert the object coordinates, but could instead set up a view matrix projection that matches the one Tiled uses, or if other stuff needs to stay in the original transform, they could at least apply a transformation wrapping the entire map structure and the objects within. Would such options exist in DragonRuby as well? |
Gotcha, and this goes hand-in-hand with:
This idea is growing on me. Seeing how integrated all of these mechanics are, I think it'd be a good feature to implement "while we're in there". I'd propose that we take Object Alignment off of the tileset properties entirely and make it a map property, making the property universal to all objects. It would make the most sense, but would introduce a backwards incompatibility; do you have a mechanism to convert TMX files from old formats to newer formats, and is it acceptable to produce TMX files that would not render properly on old versions of Tiled? This would also have an impact on framework developers who would need to change their Object Alignment implementation, so maybe this is out of the question, but having 2 different Object Alignment properties for different types of objects seems awfully confusing for... everyone.
I planned to mitigate this with an alert or warning if mismatched Y-inversions were loaded. Doing it on the project would help to unify the setting across maps and tilesets within the same project, but frameworks loading only the TMX files wouldn't know about the setting. Is passing the properties from the project through to the map/tileset's TMX a possibility?
Yes, that's essentially what I'm doing currently. I agree that it's not a very big deal for frameworks to make this transformation, my concern is more for users of the frameworks- in an API that accepts map coordinates (expecting a lower-left origin, for reasons I'll explain), if a developer wanted to e.g. have a scripted event pan the camera to a certain spot, they could enter the map coordinate to pan to. This is trivial for the framework to transform, but the thing is, when the game developer goes into Tiled to find the coordinate to tell the API to pan the camera to, they're all flipped. (other ways to do this, camera could pan to point objects, but this was just an example.) Of course, I could have the API expect Tiled coordinates (with an upper-left origin) and that solves that, but the origin in DragonRuby is in the lower-left and unconfigurable, and having map coordinates flipped from screen coordinates is... unergonomic, to say the least, considering they will be working with both. And, when choosing a tech stack to build a game with, some might pass up a library whose documentation starts out by noting a fundamental difference with the runtime in how they handle something as basic as coordinate orientation. At any rate, this isn't about my framework. :) I'd like to consider all use cases and introduce a well-rounded patch. There are many cases where it makes sense to choose bottom left origin even if it is not the default, and certainly having map coordinates match screen coordinates would be QoL for dozens of us. Interesting that people would want the origin on the left with isometric maps, but I guess it kinda makes sense- you can tilt your head to the right and read it like a graph. |
Well, no, we can't break backwards compatibility on this one, so the behavior would likely be that a map-based object alignment option would need to get overridden by the tileset one, for any tile objects where their tileset hasn't left this property on "Unspecified". I think this is fine, because actually removing the option from the tileset would take away important functionality, for example the value "Center" is often desired for graphics, whereas it is uncommon for people to want their rectangular areas aligned to their center (likely with the exception of defining Box2D fixtures).
We could mirror the option in the Project as a default value, such that it helps to set up new maps / tilesets the correct way for that project, but the option would still be stored in the map / tileset. Storing defaults in the project is a more general issue though, so this kind of solution would ask for a separate and more generic implementation.
Thanks for going into a bit more detail on that one. Indeed I would personally suggest that providing some conversion functions would be enough here, which are often needed anyway, like getting the tile coordinate under the mouse cursor, or for converting an object's position to its screen position on isometric maps. Such that there would generally be a sensible place to invert the Y axis when needed. I had to deal with the same thing while integrating Box2D (upwards Y axis) with Qt Quick (downward Y axis). |
|
Yeah, I've thought of a few neat ways to abstract this problem on my end- none transparent, but in the end I'd like users to have to use raw coordinates as little as possible anyway. Nonetheless, I'll keep hacking on this in my spare time. I understand this isn't a priority feature for you, and I appreciate how helpful you've been anyway. :)
Good point- separate properties it is. Regarding putting this on the project as a default to be forwarded to the map and tileset properties upon their creation, this is a good idea. And, as mentioned, this could warn when a map/tileset with a conflicting property is loaded, maybe a modal with an option to set the properties to match the project's setting. Agreed it would be worthwhile to make project-level defaults a generic implementation. Could be useful for map orientation, among other things. |
Merged @AdrianFL's changes in preparation for picking #2040 back up. I'd really like to see this QoL feature pushed through.
My plan is to get this usable by correcting the Y-anchoring behavior of rectangles, ellipses, and labels based on the
InvertYAxissetting. I've read the other discussions about allowing configurable anchor points for all objects (except I suppose polygons), but IMO that's a separate feature and warrants a separate PR if we ever decided to support it.