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

Add : to node properties, to differentiate them from node paths #93890

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

hakro
Copy link
Contributor

@hakro hakro commented Jul 3, 2024

Fixes #93844

It seems that adding a : before a property is mandatory in some cases, and optional in others. So this PR adds it to the doc in order to cover most cases.

An example where this is mandatory is in the MultiplayerSynchronizer.replication_config.add_property (see issue #93844)

On the other hand, the : is optional in Object.get_indexed() and Tween.tween_property()

E.g: those 2 lines yield the same result

get_tree().create_tween().tween_property(self, "position", Vector2(100,100), 5)
get_tree().create_tween().tween_property(self, ":position", Vector2(100,100), 5)

@hakro hakro requested a review from a team as a code owner July 3, 2024 10:04
@dalexeev dalexeev added this to the 4.3 milestone Jul 3, 2024
@AThousandShips AThousandShips changed the title Add : to node properties, to differentiate them from node paths Add : to node properties, to differentiate them from node paths Jul 3, 2024
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Approved already in RocketChat... chat.

As seen by the issue above, It's really worth paying attention to this when both "node" and "property path" matter.

I reiterate briefly for future reference: for some methods (Object.get_indexed(), Tween.tween_property(), etc.), the : prefix can be optionally omitted. This is because these methods only expect their given NodePath to refer to a property, and use get_as_property_path() internally to ensure it does.

Although Godot users are pretty much used to it (it's really nice), this should be seen as an exception, and ideally should be briefly stated for each method that does this.

@dsnopek
Copy link
Contributor

dsnopek commented Jul 3, 2024

If one form is preferred (and it sounds like including the leading : is preferred, since that's accepted in more places?), it would be nice for the docs to explicitly say that somewhere.

@hakro
Copy link
Contributor Author

hakro commented Jul 3, 2024

If one form is preferred (and it sounds like including the leading : is preferred, since that's accepted in more places?), it would be nice for the docs to explicitly say that somewhere.

Thank you for the review !
I have added a small paragraph explaining that. Lemme know if you see a better way to word it :)

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks, that sounds great to me!

@akien-mga akien-mga merged commit 447cbde into godotengine:master Jul 4, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@hakro hakro deleted the nodeprop-vs-nodepath branch July 4, 2024 09:55
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.

Can't add property to MultiplayerSynchronizer via code right after it spawns
5 participants