Object alignment

[Phlosioneer]

Implement object alignment property as described by bjorn in #91

[bjorn]

Thanks for getting the ball rolling on this one!

This is not a finished review. While I've made some inline comments, please don't do further work on this patch for now because as I was reviewing I've also made some local changes that I'd like to push first, just to avoid conflicts. If I don't get to pushing those changes today it may take until tomorrow.

[bjorn]

That's indeed a necessary evil, but now that we've got a project file we should definitely use that as a place to store a different default value (not affecting the map format though, just affecting the initialization of new maps with that project active). No need to do that as part of this patch though.

[Phlosioneer]

I'll look into making that change when this PR is done, then.

Reposted this because github deleted my original comment.

[bjorn]

@Phlosioneer Alright, I've pushed my changes now so feel free to continue work on this change and please also verify that my changes make sense. I've mainly adjusted the code somewhat to apply to existing functionality like MapObject::alignment and Tiled::alignmentOffset and to fix repainting and interaction issues.

Note that I've also rebased the changes onto latest master, since they were based on a few months old code before.

What I'm still unsure about, is whether we really want this property to apply to non-tile objects. Issue #91 is primarily about tile objects and in my last comment there I had suggested we resolve this by introducing a "tile alignment" option on the tileset. Why did you decide to apply this at that map level and to include the shape objects in this change?

I can see the advantage of consistency by applying this property to all objects, but there's at least the following problems:

• It makes the tile objects inconsistent with tiles on tile layers, which would still align to bottom-left unless we'd introduce another option for those.

• With the map origin in the top-left it seems a bit strange to me to support setting a different origin on shape objects, and I'm really having difficulties thinking about a use-case for this. Even more so for isometric maps. Whereas for tiles, mainly due to them being images, it makes sense to have a configurable pivot point, I don't see this being useful for plain rectangles.
  
  I think most people use Tiled through some framework or existing library, and all of them would eventually be burdened to support the object alignment option. Hence it's important to avoid options that nobody needs.

Let me know what you think.

[Phlosioneer]

I pushed it up to the map level and incorporated shape objects because of how I saw the problem statement.

I saw the problem as "My rectangle objects are using different alignment rules than my tile objects, and it's weird." So the solution was to make rectangle and tile objects use the same origin. The various alignments then change which origin we use, while any alignment other than "unset" would ensure that a rect and a tile of equal sizes and positions will always occupy the same space.

The extension from rects to ellipses is fairly natural. I thought the polygons would also make sense, but you have pointed out that they have their own origin defined by the user already (i.e. the first point drawn).

[bjorn]

The various alignments then change which origin we use, while any alignment other than "unset" would ensure that a rect and a tile of equal sizes and positions will always occupy the same space.

Right, whereas, if we could only set an origin on tiles, then only setting it to "TopLeft" would make it match with the rectangle/ellipse objects. I think you're right that consistency between the objects is more important here than consistency between alignment of objects and tiles. Still, due to the nature of the isometric renderer I'm not sure if it makes sense to support the alignment options on shapes there.

And actually I think we may have a use-case for turning TopLeft into BottomLeft also for shape objects, which is when we want to finish the feature of allowing the Y axis to be inverted, see #2040 (comment).

[Phlosioneer]

The sticking point here seems to be the isometric renderer.

The only alignments we really want to be messing with in the isometric renderer are top corner vs bottom corner. I suggest we use different enum values to handle the isometric case, instead of the current one-size-fits-all approach.

So the docs would read:

  **objectalignment:** Controls the origins for tile and shape objects.
  Valid values in isometric mode are ``unset``, ``top``, and ``bottom``. In
  all other modes, valid values are ``unset``, ``top-left``, ``bottom-left``,
  and ``bottom-center``.
  

[bjorn]

I don't think it's worth adjusting the terminology to the map's orientation. I think it's easier if "top-left" alignment simply means the corner at the top in isometric projection. After all, that is still also the top-left corner of the object's rectangle as far as the code is concerned.

In addition, only shape objects are isometrically projected. For tile objects the object alignment can apply exactly the same way on orthogonal maps and isometric maps, since isometric projection is only applied to the position.

Btw, just noting here that this PR probably also closes #330, a near duplicate of #91 about isometric maps.

[Phlosioneer]

I am unclear what changes I need to make.

[bjorn]

I am unclear what changes I need to make.

I just pushed some more fixes. There was quite a bit left to do, but it was hard to describe or to explain how to fix it. Mostly been testing and considering different approaches to resolve the remaining issues.

I think it should be ready for merging now, but could you give it a final round of review and testing?

Regarding the discussion above, I decided we shall stick to having this as a map property and to apply it consistently for tile and non-tile objects, also in the isometric renderer. It's just that say BottomLeft is to be interpreted in unprojected coordinate space (in the code this is called "pixel coordinates", as opposed to "screen coordinates"). Whether it will be useful to somebody I don't know, but I think anyway most people will just want to use this property to set the alignment to TopLeft for all objects, heh.

[Phlosioneer]

Seems good to me!

[bjorn]

One remaining problem that occurred to me, is that while this change allows setting a consistent alignment for objects on maps, it does not affect tile collision objects. Of course, tile objects are not allowed as tile collision shapes so there actually isn't an inconsistency there, since the alignment is already TopLeft for shape objects, but it seems weird to support alternative alignments for all objects on maps but not for tile collision shapes.

That kind of brings me back to the idea of only supporting this alignment option for tile objects, and putting the property on the tileset instead of the map. What's your opinion on this? We can leave most of this change intact and just move the property and adjust the MapObject::alignment function.

[Phlosioneer]

I don't think it's as important to worry about the alignment of collision objects. As you said, there's no actual inconsistency that this would fix.

I don't understand your second paragraph, though. Are you saying that tilesets should dictate the orientation of tile objects on maps? So if I had two tilesets, one with Upper Left and one with Bottom Right, tile objects from the two tilesets would behave completely opposite?

In any case, I think this is a fine change as-is, and if someone makes an issue asking for object alignment on tilesets, we address it then. Tiled itself has no defined mapping from tile-collision-coordinates to map-coordinates, so any problems users would encounter, would be through code they wrote or a library they use.

[bjorn]

I don't think it's as important to worry about the alignment of collision objects. As you said, there's no actual inconsistency that this would fix.

Right, I don't think it's important either, but I do think it's a bit strange to introduce an alignment option that applies to all objects and then to not support it in the tile collision editor. If there was a use-case for this property, then it would be expected to work there as well. If the only use-case of the property is fixing the inconsistency, then it could be on the map or the tileset.

I don't understand your second paragraph, though. Are you saying that tilesets should dictate the orientation of tile objects on maps? So if I had two tilesets, one with Upper Left and one with Bottom Right, tile objects from the two tilesets would behave completely opposite?

That's what I was saying, but I'll consider the implications further tomorrow...

In any case, I think this is a fine change as-is, and if someone makes an issue asking for object alignment on tilesets, we address it then.

We need to decide now where this property is best added, because there is no way to remove it again. Due to compatibility issues it's hard to simplify things, we can generally only make things more complicated.

[Phlosioneer]

That's what I was saying, but I'll consider the implications further tomorrow...

I strongly disagree with that idea, then. It would cause even more confusion than there currently is. Just swapping tilesets could mean you need to reposition all the tile objects on your document. Tile objects could behave differently if they come from different tilesets, even completely opposite as in the example I provided above. And what happens when different maps want different alignments? Enforcing this on the tileset level means that a normal map and an isometric map couldn't share a tileset for tile objects without weird things happening, or the user would be forced to manually correct the alignment for each object. Which is how things are right now.

We need to decide now where this property is best added, because there is no way to remove it again. Due to compatibility issues it's hard to simplify things, we can generally only make things more complicated.

What I was talking about maybe adding in the future is an alignment option for a tileset that only affects collision objects, not related to your actual idea. This was a miscommunication.