-
-
Notifications
You must be signed in to change notification settings - Fork 23.7k
GDScript: Add Trait System #107227
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
base: master
Are you sure you want to change the base?
GDScript: Add Trait System #107227
Conversation
|
We're considering naming for trait inclusion syntax. Please react to this comment with your preference: note: The GDScript team will have the final say |
|
Please note that PRs are generally not the right place to discuss feature design, we should only discuss implementation here. This is usually not a problem for small features and enhancements, but a significant problem for large features like this. Unfortunately, GitHub Issues and PRs do not support tree-like or even two-level comments. This makes it difficult to follow long discussions in case of large and popular feature requests. For this reason, we locked the original proposal, but users started discussing and suggesting ideas in the previous PR instead. This may have been one of the factors why the previous PR grew to a practically unreviewable state and included features that should have been moved to the next iterations. We would not like to repeat this situation with the new PR, so I suggest to move the discussion of the feature design to a discussion in the |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Created a discussion. Please move your comments there if needed. |
Not sure if default methods are allowed in the future. Hope to see it after this get merged |
|
From a quick look at some of the PR files (specifically the keywords), it looks like traits can only be declared as class members. How does referencing traits work from another class? For example, if I have a file # my_traits.gd
class_name MyTraits
trait Damageable:
func take_damage():
print("Ouch, that hurt!")
trait Movable:
func move():
print("You're moving me")And then I have another file # player.gd
class_name Player
extends CharacterBody2D
# Which one do we need to use?
uses Damageable, Movable # Easier to read.
uses MyTraits.Damageable, MyTraits.Movable # Consistent with inner classes and such.
func _ready():
...In other words, do we need to reference the Personally, I think it'd be nice to bring back (I'm posting this here and not in the discussion thread since it seems to me like a detail of this implementation, but I recognize the line is blurry. If you think it'd fit better there, let me know and I can certainly move over there to continue.) |
In this pr , we will not. It may come back in the future if this pr is successful . Also For the case this pattern is appropriate : uses MyTraits.Damageable, MyTraits.Movable |
|
Great, thanks for the clarification! I'm working on updating my docs PR to match how this version of traits works, so that hopefully the feature and its documentation can launch simultaneously 😄 It is still posted at godotengine/godot-docs#10393 |
Meorge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a few grammar suggestions for trait-related errors.
8f6cf99 to
da9dfc2
Compare
|
Since this alters the token enum value, when you rebase next, please bump the
|
This is a bit premature advice. As long as there is a small chance that this might be included in 4.5, there is no need to increase the tokenizer version (dev versions are not considered releases). If this PR will be included in 4.6 and later, then increasing the version makes sense. But it is not necessary, we can check before each Godot release if there were changes in the tokenizer, and if so, increase the version by 1. |
c216308 to
cd44e2d
Compare
|
My uttermost personal thanks for making the leanest PR possible for this feature. Should we encourage people to test this PR to ensure it all works as intended? |
100% go for it, |
|
Great job! I tried to migrate my project which heavily depends on traits from older PR, found some problems:
t_parent.gd: trait TParent:
func print_parent() -> void:
print("print_parent")t_child.gd: trait TChild:
uses TParent # <--- Parse Error: Could not find trait "TParent".
func print_child() -> void:
print("print_child")When putting traits in same file or with each having separate class_name and referenced with
That's quite problematic for my project. I have: class_name TTargetableTeamMember
trait Trait:
uses TTeamMember.Trait, TMapObject.Trait
I understand that those limitations might be coming from limited scope of this new PR, so no worries if those things will come in later. The 2nd thing blocks my further tests because the codebase doesn't parse correctly. |
There no global traits in this pr. To get to a trait go through global class if it is in another file
The errors are correct. trait Movable extends Node2D: # this trait can not be used in class that is not a descendant of Node2D
func move_right():
position.x += 10 # position is from Node2D
class Player extends Sprite2D: # here extends can also be Node2D. It will work
uses Movable # usable because Sprite2D is a descendant of Node2D
func _ready():
move_right() # Call method from the trait |
|
ok, my problem was that the TTargetableTeamMember didn't have Got another ones for you ;)
class_name MultiTest
extends PhysicsBody3D
uses Trait1
trait Trait1:
extends PhysicsBody3D
func trait1_method() -> void:
print("trait1")
func do_something_with(something: MultiTest.Trait1) -> void:
#var a := something as MultiTest # Doesn't work
var a := something as Node as MultiTest # Works but awkwardSame with
Create empty node2d scene, add script to that node: extends Node2D
func _ready() -> void:
var a := self as TNestedObject.Trait
if a:
print("Wrong!")It will print the messsage even if it shouldn't. Also, sometimes I get |
|
what happens when |
|
See #107227 (comment) - a trait that uses another trait should work fine, I guess that extending trait is invalid in this context. |
|
How does the traits system interact with Also, now that 4.5 has released it would be awesome to get this into 4.6 early for testing coverage :) |
5b82727 to
0a325c8
Compare
It's all good now , @AndreiSchapov thanks for you contribution
No it doesn't, it's a GDScript only feature |
|
@SeremTitus can you describe your collision handling approach for this implementation? I would like to drill down the details on what it currently handles and how it handles it. I'll try to lay out the cases but not yet imply what the behavior I expect is. The "cases" of collisions are, given a class C, and traits T1 and T2:
I would like to know what your implementation currently accounts for, and what it considers "resolvable" (i.e. allowable collisions, where things can merge happily) and what it considers "unresolvable" (i.e. an error state). Also, I would like to reiterate @eyalzus12 's question:
|
|
I can anwer partially:
The answer is that you can't extend a trait, you can have a trait that's using another trait or few of them at once - it then acquires the used trait(s) contents. Ideally, it would also make it work with is/as operators when matched with used traits, but that's not the scope of this PR (was in the original one but apparently it was too much at once). |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Very excited for this PR! It seems that when the engine is referring to trait code, it points to the GDScript file that This behavior crops up anywhere that the source code would be referenced -- so looking up the symbol definition with Ctrl+Click, and more notably, trying to step through the code or reference stack frames with the debugger will open the wrong script. Example Codeclass_name UtilTraits
# ...
trait ByteCounter:
var bytes: int
func add_bytes(count: int) -> void:
assert(count >= 0) # Line 15
bytes += countclass_name FileReader
uses UtilTraits.ByteCounter
func read_file() -> void:
add_bytes(-1)extends Node
func _ready() -> void:
var reader := FileReader.new()
reader.read_file() |
I’m not including some user-facing features such as breakpoints pointing to traits and Ctrl+Click navigation, which were dropped from the previous PR. However, they can be reintroduced easily if this PR gets merged |
4010844 to
df5f9e9
Compare
df5f9e9 to
0c9aa83
Compare
0c9aa83 to
bc0b0f5
Compare
|
Delayed again? 😢 painful. |
|
Since GDScript is weakly typed, could it instead be an editor hint? |
Focusing on: