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

Improve Object.get_property_list() method description #81093

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

mateuseap
Copy link
Contributor

@mateuseap mateuseap commented Aug 28, 2023

What I did

Bugsquad edit: Fixed method name and formatting.

@dalexeev
Copy link
Member

dalexeev commented Aug 28, 2023

godotengine/godot-docs#7533:

Godot property is different from a C# property (not sure if "property" is a concept in gdscript that could also cause confusion). Would be great if this was clarified in the docs.

This is not correct in general, for example in GDScript get_property_list() returns all variables:

var a: int
@export var b: int

func _ready() -> void:
    for p in get_property_list():
        print(p)
...
{ "name": "a", "class_name": &"", "type": 2, "hint": 0, "hint_string": "", "usage": 4096 }
{ "name": "b", "class_name": &"", "type": 2, "hint": 0, "hint_string": "int", "usage": 4102 }

I think it should be a note at the end of the method description explicitly mentioning C#.

@mateuseap Please update also the commit message.

@dalexeev dalexeev added this to the 4.2 milestone Aug 28, 2023
@dalexeev dalexeev changed the title Improve Object.get_method_list method description Improve Object.get_property_list() method description Aug 28, 2023
@mateuseap mateuseap force-pushed the docs/objectClassReference branch 3 times, most recently from 51053e3 to b5a4bef Compare August 28, 2023 22:06
@mateuseap
Copy link
Contributor Author

godotengine/godot-docs#7533:

Godot property is different from a C# property (not sure if "property" is a concept in gdscript that could also cause confusion). Would be great if this was clarified in the docs.

This is not correct in general, for example in GDScript get_property_list() returns all variables:

var a: int
@export var b: int

func _ready() -> void:
    for p in get_property_list():
        print(p)
...
{ "name": "a", "class_name": &"", "type": 2, "hint": 0, "hint_string": "", "usage": 4096 }
{ "name": "b", "class_name": &"", "type": 2, "hint": 0, "hint_string": "int", "usage": 4102 }

I think it should be a note at the end of the method description explicitly mentioning C#.

@mateuseap Please update also the commit message.

@dalexeev thanks for helping me! I removed my previous modifications, added a note in the end of get_property_list() method description and adjusted the commit message. I hope that's ok, if it's not, I'm open to modify it. Again, thanks for the help! :)

@YuriSizov
Copy link
Contributor

I'm not sure what the added note is saying, and so I feel that it's wrong. It is true that while in GDScript all class members would be properties, in C# properties need to be explicitly opted in. But I'm not confident the added note helps.

The list of properties is not related to scripting languages. It's an overall engine concept. Built-in objects also contain properties and this method returns all of them. Properties can be added dynamically by overriding _get_property_list on your object too. This is true for any language.

I think that the note could simply state that C# and 3rd party languages may require you to explicitly mark class members as Godot properties.

@mateuseap mateuseap force-pushed the docs/objectClassReference branch from b5a4bef to 4494743 Compare August 28, 2023 23:18
@mateuseap
Copy link
Contributor Author

mateuseap commented Aug 28, 2023

I'm not sure what the added note is saying, and so I feel that it's wrong. It is true that while in GDScript all class members would be properties, in C# properties need to be explicitly opted in. But I'm not confident the added note helps.

The list of properties is not related to scripting languages. It's an overall engine concept. Built-in objects also contain properties and this method returns all of them. Properties can be added dynamically by overriding _get_property_list on your object too. This is true for any language.

I think that the note could simply state that C# and 3rd party languages may require you to explicitly mark class members as Godot properties.

@YuriSizov done! I still mantained the statement that says that in GDScript all class members are treated as properties, but I've clarified that C# and 3rd party languages may require you to explicitly mark class members as Godot properties, as you've pointed out.

@mateuseap mateuseap force-pushed the docs/objectClassReference branch 7 times, most recently from d204462 to 367c515 Compare August 29, 2023 02:30
@@ -651,6 +651,7 @@
- [code]hint[/code] is [i]how[/i] the property is meant to be edited (see [enum PropertyHint]);
- [code]hint_string[/code] depends on the hint (see [enum PropertyHint]);
- [code]usage[/code] is a combination of [enum PropertyUsageFlags].
[b]Note:[/b] While in GDScript all class members are treated as properties, in C# and some 3rd party languages may be necessary to explicitly mark class members as Godot properties using decorators or attributes.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[b]Note:[/b] While in GDScript all class members are treated as properties, in C# and some 3rd party languages may be necessary to explicitly mark class members as Godot properties using decorators or attributes.
[b]Note:[/b] In GDScript, all class members are treated as properties. In C# and GDExtension, it may be necessary to explicitly mark class members as Godot properties using decorators or attributes.

Copy link
Member

Choose a reason for hiding this comment

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

Should we clarify "member variables" to avoid any confusion?

@mateuseap mateuseap force-pushed the docs/objectClassReference branch from d87dc95 to 6020e34 Compare August 29, 2023 10:11
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

LGTM now!

@akien-mga akien-mga merged commit 8dfd9d3 into godotengine:master Aug 29, 2023
@mateuseap mateuseap deleted the docs/objectClassReference branch August 29, 2023 10:59
@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
None yet
Development

Successfully merging this pull request may close these issues.

get_property_list method documentation unclear on what properties are returned
6 participants