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

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

Closed
kleonc opened this issue May 13, 2022 · 5 comments · Fixed by godotengine/godot#64661
Labels
breaks compat Proposal will inevitably break compatibility topic:2d topic:3d
Milestone

Comments

@kleonc
Copy link
Member

kleonc commented May 13, 2022

Describe the project you are working on

Godot.

Describe the problem or limitation you are having in your project

Naming of map_to_world()/world_to_map() confuses many users, world is often assumed to refer to the global coordinate space for the given TileMap/GridMap but in these methods world actually referes to the local coordinate space for the given TileMap/GridMap. Even though it's correctly stated in the docs what the actual behavior is, many people will probably assume the docs are just wrong (for example: godotengine/godot#60697).
Bad naming, even if the actual behavior/meaning is well explained, is still bad naming.

Another thing is: users might be assuming these methods refer to the global space because they need such methods. It's a common case to work with global values to avoid to_local()/to_global() or similar calls polluting the code all over the place.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

For both TileMap and GridMap:

  • Rename:

    • map_to_world -> X_to_local
    • world_to_map -> local_to_X
  • Add:

    • X_to_global
    • global_to_X

Where X is map/grid/ some other similar term. I'll leave it up to discussion.

Such names should be intuitive as they follow the nomenclature already used in to_local(), to_global(), global_transform, get_local_mouse_position() and many other already existing property/method names. Also seeing X_to_local and X_to_global for the first time would probably make you think something like "What's the difference and which one should I choose?" so I'd say it might be some added value in terms of discoverability.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

  • Simple renames of existing methods.
  • X_to_global(X_position) would be shorthand for to_global(X_to_local(X_position)).
  • global_to_X(global_position) would be shorthand for local_to_X(to_local(global_position)).

If this enhancement will not be used often, can it be worked around with a few lines of script?

Helper methods with expected behavior/naming can be added manually (again, and again for each project) but it won't rename/remove confusing built-in methods.

Is there a reason why this should be core and not an add-on in the asset library?

These are core class methods.

@kleonc kleonc added topic:2d topic:3d breaks compat Proposal will inevitably break compatibility labels May 13, 2022
@Calinou Calinou added this to the 4.0 milestone May 13, 2022
@Mbellsudteen

This comment was marked as spam.

@akien-mga
Copy link
Member

We discussed this in a proposal review meeting.

Renaming map_to_world to map_to_local and same for the other way around sounds good, it should be much clearer.

@groud wasn't convinced about adding the global equivalents. I don't have a strong opinion myself.

@akien-mga akien-mga moved this from Ready for Review to Ready for Implementation in Godot Proposal Metaverse Jun 30, 2022
@kleonc
Copy link
Member Author

kleonc commented Jul 3, 2022

@groud wasn't convinced about adding the global equivalents. I don't have a strong opinion myself.

As far as I can tell people usually work in the global coordinates so I think these would be used often. However, I see how adding such methods could make people demand similar helper methods for literally anything working in local coordinates (which I think is literally anything besides the physics stuff?:thinking:). So in case we want to avoid it it makes sense to do not add such methods. Instead teaching the user how to convert between local and global spaces makes probably more sense. So I think just renaming should be sufficient (it's easy to search for "godot how to convert global to local" etc. and find an actual answer). Eventually a note about to_global/to_local could be added to the docs of map_to_local/local_to_map.

@groud
Copy link
Member

groud commented Jul 4, 2022

So I think just renaming should be sufficient (it's easy to search for "godot how to convert global to local" etc. and find an actual answer). Eventually a note about to_global/to_local could be added to the docs of map_to_local/local_to_map.

I think that's a good idea. A note in the documentation could help.

@Mickeon
Copy link

Mickeon commented Aug 20, 2022

I've had frequent situations where I had to convert tile coordinates to global, as a result of making a bit more advanced usage of TileMap (e.g. converting Nodes to tiles and viceversa), so I wouldn't personally mind a method that looked tidy while doing just that, but I do understand the risk of bloating the Class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat Proposal will inevitably break compatibility topic:2d topic:3d
Projects
Status: Implemented
Status: Discussion Stalled
Development

Successfully merging a pull request may close this issue.

6 participants