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

Added Y flip option to the coordinates #2040

Closed
wants to merge 12 commits into from

Conversation

AdrianFL
Copy link

@AdrianFL AdrianFL commented Dec 2, 2018

Added the possibility of flipping the Y coordinate, to set the origin at the bottom and not at the top, as discussed in the issue #249.

This doesn't change any internal behavior or logic, and only the visual representation of the numbers.

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.

Thank you for picking this up again! I have provided some feedback with inline comments, but it seems to me that largely the same things still apply that I had commented on #1897, so maybe read my comments there as well. I hope we can get the feature merged this time. :-)

src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/propertybrowser.cpp Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
@AdrianFL
Copy link
Author

AdrianFL commented Dec 2, 2018

Thanks for the detailed feedback! I'll work through all of it and commit again with the changes made as soon as possible. I hope the feature gets merged this time too :)

Sorry for having so many coding style mistakes, I'll pay more attention to it next time.

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.

More detailed feedback. :-)

src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/propertybrowser.cpp Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
@AdrianFL
Copy link
Author

AdrianFL commented Dec 3, 2018

Thanks for all the feedback provided, you are awesome :) I think I've finally understood how MapDocument and the MapRenderer work internally in the project thanks to it (I hope I can use that knowledge to be able to help in other issues in the future).

Regarding my last commit, I think I've implemented all the feedback, and after testing it I think the feature should be done, but, I have one question: You suggested to change the tileY function to use int instead of qreal. Should I change pixelY to use int too?

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 all the changes! However, you have misunderstood what I meant with using the MapRenderer. Explained further in an inline comment. :-)

You suggested to change the tileY function to use int instead of qreal. Should I change pixelY to use int too?

No. The reason to use int for the tile coordinates is because there don't exist sub-tile coordinates. But objects, which use pixel coordinates, do support sub-pixel positions as well.

src/tiled/abstractobjecttool.cpp Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/invertyaxishelper.h Outdated Show resolved Hide resolved
src/tiled/preferences.cpp Outdated Show resolved Hide resolved
@AdrianFL
Copy link
Author

AdrianFL commented Dec 3, 2018

No. The reason to use int for the tile coordinates is because there don't exist sub-tile coordinates. But objects, which use pixel coordinates, do support sub-pixel positions as well.

Okey, didn't knew about sub-pixel positions, thanks for clarifying.

Added all the suggestion to the most recent commit :) I had to modify a bit the code suggested for the boundingRect to make it work, I hope using the Map of the MapDocument to obtain the size it's fine, I think that's what you intended.

@bjorn
Copy link
Member

bjorn commented Dec 4, 2018

I had to modify a bit the code suggested for the boundingRect to make it work, I hope using the Map of the MapDocument to obtain the size it's fine, I think that's what you intended.

I don't understand, because the code in the commit looks exactly the same as the code I suggested, or am I missing something?

Thanks for addressing all the comments! :-)

@AdrianFL
Copy link
Author

AdrianFL commented Dec 4, 2018

Well, when I copied that code fragment to my editor, for some reason instead of mTarget->map()->size() it appeared mTarget->size(), and I needed to add that map() call in order to compile. But looking at your code here, it seems it was my mistake copying, so sorry for that.

Well, I hope everything is fine now :)

@AdrianFL
Copy link
Author

Sorry to bother you, but I would like to ask if additional change is needed to this pull request, or if it is ready to be merged?

Mostly just personal preference.
Conflicts:
	src/tiled/preferences.h
No longer using this namespace (71c1de5).
@bjorn
Copy link
Member

bjorn commented Dec 27, 2018

@AdrianFL I'm happy with the code formatting now, but I wondered about something while trying it out.

selection_278

In the above image, I've placed an object at the origin with the inverted Y axis option enabled. But it's going down instead of up. I don't think this is what you would expect when inverting the Y axis.

What's your opinion about this? Of course, changing objects to be bottom-left rather than top-left aligned in this mode is going to affect a lot more code. It would also mean that the object height is going to be relevant when calculating the display value of its position.

@AdrianFL
Copy link
Author

@bjorn Just to be sure I am understanding this correctly. What is happening is that the object's position is placed using the top-left corner as a reference, but besides that the inversion of the Y axis is working properly, right?

Well, in my opinion, that seems a problem that needs to be solved before merging the whole functionality of this PR, because it may lead to some confusion to the users if not. But depending on the approach, I think it could even be a completelly different functionality.

Do you think users may be interested in choosing the corner which the object is aligned? Because in that case, it could even be a different customizable option, implemented separately from this one. I'm not sure about if this has any utility for users, but if we are going to do the work, we could assess if it's worth doing it.

Anyway, in my opinion, I think we should work on changing the objects corner-alignment before merging it. I can work on this, but I'll need some time to familiarize myself with the code better before doing any changes. Is it possible to see a class diagram or something similar to understand better of the code is structured?

@bjorn bjorn mentioned this pull request Dec 19, 2019
@elleyer
Copy link

elleyer commented Dec 7, 2021

Was this one added in some another PR?

@AdrianFL
Copy link
Author

AdrianFL commented Dec 8, 2021

Was this one added in some another PR?

Hi @elleyer , unfortunately not, life got in the way and I never had the chance to properly work on it. I also don't think it has been merged in another PR, but I would be glad to help making the necessary core changes to get it done!

@jonatino
Copy link

jonatino commented Feb 2, 2022

What's left for this to be merged? This would be a huge QoL for me personally.

@bjorn
Copy link
Member

bjorn commented Feb 2, 2022

@jonatino This PR got stuck on the issue I commented on at #2040 (comment). What's your take on this?

@jonatino
Copy link

jonatino commented Feb 2, 2022

@bjorn I would say that's definitely unexpected behaviour. I can imagine that changing the anchoring logic for all objects will be quite a bit of work if it was never designed to do so.

Albeit, personally I wouldn't say this is enough of a blocker to stop this PR from being merged. It's unexpected behaviour that humans would notice and be able to fix within seconds. Could always track it in another issue.

Having the option will be a nice quality of life improvement for people who use bottom-left aligned maps to be able to quickly see the x/y of a tile without needing to subtract the height manually. Very minor change which saves a lot of manual work.

@vinnydiehl
Copy link

Could someone point me towards the object anchoring logic? I'd like to take a look at this soon.

@bjorn
Copy link
Member

bjorn commented May 10, 2023

Closing as superseded by #3679.

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.

5 participants