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

Rename TileMap/GridMap.world_to_map and opposite to local_to_map #64661

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 20, 2022

(Partially) closes godotengine/godot-proposals#4535.

For both TileMap and GridMap:

  • world_to_map -> local_to_map
  • map_to_world -> map_to_local

(This screenshot may not accurately represent the description in the PR)
Showcase

Also changes any mention of "world" in this context to "local" to avoid future confusion.

Finally, updates the Documentation of both methods for consistency.
In particular, adding a note on how to convert the returned values from local to global coordinates and vice versa.

Very glad of this one. I use this method frequently and I want more people to properly understand what it does.

groud
groud previously requested changes Aug 22, 2022
editor/project_converter_3_to_4.cpp Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the rename-tilemap-world branch 2 times, most recently from 85d31af to ecaccb4 Compare August 22, 2022 10:14
editor/project_converter_3_to_4.cpp Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the rename-tilemap-world branch 2 times, most recently from 737bd56 to e9a9338 Compare August 22, 2022 11:21
@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 22, 2022

Am a dummy.

This is off-topic, but this conversion logic could use some type checking. I was thinking back to #64377 and how hard it would be to properly recognise and convert due of how vague the original name is...

@groud
Copy link
Member

groud commented Aug 22, 2022

I think the conversion is still missing for the tilemap's functions though. Since it works with Vector3i, it should also work with a Vector2i I guess?

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 22, 2022

Indeed, it is missing. That's because unfortunately the conversion tool currently has no way to tell what Class the renamed method is associated to, so it looks like it'll convert every method name regardless of context.

If I had the time to concoct... basically a rework of the entire process I would've certainly done it, but actually detecting from what class type the method is from (accounting for user-defined methods too), with or without optional typing/type inference, looks to be insurmountable in my eyes. Certainly possible, but not nearly as simple as a String replacement.

@Mickeon Mickeon force-pushed the rename-tilemap-world branch from e9a9338 to 4100224 Compare August 26, 2022 00:14
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good.

@akien-mga akien-mga requested a review from groud August 26, 2022 06:34
@Mickeon Mickeon force-pushed the rename-tilemap-world branch 2 times, most recently from 655dfe5 to c135f81 Compare September 5, 2022 16:00
For both TileMap and GridMap:
- `world_to_map` -> `local_to_map`
- `map_to_world` -> `map_to_local`

Also changes any mention of "world" in this context to "local" to avoid future confusion.

Finally, updates the docs of both methods for consistency.
In particular, adding a note on how to convert the returned values from local to global coordinates and vice versa.
@Mickeon Mickeon force-pushed the rename-tilemap-world branch from c135f81 to 694190a Compare September 5, 2022 16:09
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 5, 2022

Rebased, and actually fixed the faulty world_to_map conversion.

@akien-mga akien-mga dismissed groud’s stale review September 6, 2022 14:04

Changes done.

@akien-mga
Copy link
Member

akien-mga commented Sep 6, 2022

Changes done.

Actually might need double checking by @groud, I assumed the above but Yuri tells me this was still be discussed on chat yesterday.

Edit: Confirmed good to merge by groud.

@akien-mga akien-mga merged commit 5fb84e5 into godotengine:master Sep 6, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

TileMap/GridMap: Rename map_to_world()/world_to_map() and add complementing methods for global space
5 participants