-
-
Notifications
You must be signed in to change notification settings - Fork 224
Initial test of Godot duplicator with Area2D #1141
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?
Conversation
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.
Thanks a lot for your contribution, it looks really good for your first pull request! 😊
One thing I've noticed is that a lot of the test assertions aren't testing the duplicate()
functionality itself, but a lot around that. We don't need to check if setting a field works or if cloning Gd
functions correctly, as we have already dedicated tests for those things elsewhere. So it would be good if assert_eq
statements could focus on checks after Node::duplicate()
is invoked. Which you do at the end 👍
I also made a comment about Area2D
. Let's try to get this problem reproduced with Area2D
, and later we can see if we can reduce it even further, to show the problem with only Node
. But for now, focus on the first step that demonstrates the problem with Area2D
🙂
#[godot_api] | ||
impl INode for SomeDuplicator { | ||
fn init(_base: Base<Node>) -> Self { | ||
SomeDuplicator { | ||
int_val: 0, | ||
optional_node: None, | ||
export_area: OnEditor::default(), | ||
} | ||
} | ||
} |
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.
As this uses default values for every field, it can be generated.
This can be done on the struct declaration above, by adding init
:
#[derive(GodotClass)]
#[class(init, base=Node)]
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.
Addressed.
obj.bind_mut() | ||
.optional_node | ||
.as_mut() | ||
.unwrap() | ||
.set_name("renamed"); | ||
assert_eq!( | ||
obj.bind().optional_node.as_ref().unwrap().get_name(), | ||
StringName::from("renamed") | ||
); |
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.
Again, just tests what was set. The idea of adding this test is to check if Node::duplicate()
copies the values, not if manually setting them works.
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.
Understood. Addressed.
assert_eq!(duplicated_obj.bind().int_val, 5); | ||
assert_eq!( | ||
duplicated_obj | ||
.bind() | ||
.optional_node | ||
.as_ref() | ||
.unwrap() | ||
.instance_id(), | ||
obj_id | ||
); | ||
assert_eq!( | ||
duplicated_obj | ||
.bind() | ||
.optional_node | ||
.as_ref() | ||
.unwrap() | ||
.get_name(), | ||
StringName::from("renamed") | ||
); | ||
assert_eq!(duplicated_obj.bind().export_area.get_collision_layer(), 1); | ||
assert_eq!(duplicated_obj.bind().export_area.get_collision_mask(), 1); |
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.
You can extract this into a variable, and then use everywhere:
let duplicated = duplicated_obj.bind();
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.
Done.
Co-authored-by: Jan Haller <[email protected]>
@Bromeon I tried the changes to the codegen file but couldn't get it to compile. See previous comment thread : #1141 (comment) Even with this removed I still couldn't get the test to pass, even without |
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1141 |
Here's the code failure after the duplication:
Which is this line assert_eq!(duplicated.int_val, 5); |
Hm...
I wonder if this behavior is different in GDScript? |
Yeah I'm not too sure how to proceed from here. I haven't really used GDScript, so I don't think I can provide much input there. But I did get my previous information from the link you provided about it duplicated all the properties. |
This currently doesn't compile because of an importer area. So please don't merge lol.
But should hopefully be fixed soon. Still will take any feedback. Like I'm unsure if it's wise in test cases to run
unwrap()
on the things that should beSome
given the context.