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 Position* nodes to Marker* #64370

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 13, 2022

Closes godotengine/godot-proposals#5199.

As brought up in #54161 (comment).

The naming of this Node is odd, as it shares its name with one of its properties. Thus, assuming its name is unchanged, when fetching the position, it's possible to be in this situation:

@onready var position_2d := $Position2D
...
position_2d.position += Vector2.ONE # awkward to read

Thus, the comment above proposes an alternative name that I personally really like:

  • Position2D -> Marker2D
  • Position3D -> Marker3D

I believe it suits them a lot more.
image

Also changes their respective file names.

This renaming is not as obvious as they may be. If there's need for a proposal I'll make one.

@Mickeon Mickeon requested review from a team as code owners August 13, 2022 17:18
@KoBeWi
Copy link
Member

KoBeWi commented Aug 13, 2022

You also need to do add_compatibility_class().

@Riteo
Copy link
Contributor

Riteo commented Aug 13, 2022

tbf I don't agree a lot with the choice of keeping the file names the same. If someone really cares looking for their history IIRC they should be able to see the first commit for these new files and notice the rename, then look for their previous history.

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 13, 2022

Oh, no, don't worry, I'll just change them in a following PR. It's to keep it one thing at a time. The reception to this PR is overwhelmingly positive, so I may do it, soon, as well.

@Riteo
Copy link
Contributor

Riteo commented Aug 13, 2022

@Mickeon sorry, I don't get it, why should you do it in a separate PR?

@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 13, 2022

I may be incredibly dumb, but you can look at this PR. I was able to tell Git that Position2D.xml and Position3D.xml have merely changed name, but the file is actually the same, and it recognised that. Because position_2d and position_3d have been modified internally, changing their file name would result in them being recognised as two wholy different files.

If there's a way to forcefully tell Git it's the same file, I don't know it.

Edit: I believe I still have to modify this PR, actually. My plan to keep history would not work, as there's position_2d comments around the place that would need to be changed, as well.

@Riteo
Copy link
Contributor

Riteo commented Aug 13, 2022

@Mickeon I don't think that's that big of a deal? The commit is still very clear and from the diff one should still be able to see the old file name with the corresponding history.

I think that the renaming thing is just some optimization done by git and it has no other special property.

@Mickeon Mickeon force-pushed the rename-marker-node branch from 51c8219 to c5bbc30 Compare August 13, 2022 22:07
@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 13, 2022

Got to have my cake and eat it, too. Updated the PR to change the file names, as well.

@Mickeon Mickeon force-pushed the rename-marker-node branch 2 times, most recently from 6b87474 to a467941 Compare August 13, 2022 22:42
@Mickeon Mickeon force-pushed the rename-marker-node branch from a467941 to e43a474 Compare August 15, 2022 17:37
- Position2D -> Marker2D
- Position3D -> Marker3D

Also changes their respective file names.
@Mickeon Mickeon force-pushed the rename-marker-node branch from e43a474 to 8bb3053 Compare August 23, 2022 17:50
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.

Seems fine to me. @reduz also appeared fine with that change.
And the PR and proposal are significantly upvoted, so this should be good to go.

@akien-mga akien-mga merged commit b556d8c into godotengine:master Aug 24, 2022
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the rename-marker-node branch August 24, 2022 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Position* Nodes to Marker*
4 participants