Add missing class methods #216
Conversation
BastiaanOlij
added
the
enhancement
|
This should be ready now. |
|
What's the status of this? Is this still needed or have those changes already been made in the meantime? |
|
I've checked all the methods one by one and none have been implemented in master. |
|
@lupoDharkael what are you looking for? Your changes in this PR haven't been merged into master yet because you've marked it as WIP, so you won't find your changes in master. Or are you looking for other changes before you can finish this PR? edit owh, you're referring to the list of methods to add to the different core classes in the issue? |
|
Well, I marked this as WIP because I was looking for adding all the missing methods. I did this a few months ago so I don't remember what extra code I wanted to add but I guess I just wanted to remove the WIP after a review because I commented "This should be ready now." xD |
|
Merge it if you agree with the actual code. I think it covers almost all the missing methods. |
|
@lupoDharkael just remove the WIP, I think we should merge this, if there are any other missing bits and bobs we can add them later. @karroffel are you happy with this? |
| ERR_FAIL_COND(!axis.is_normalized()); | ||
|
|
||
| real_t d = axis.length(); | ||
| if (d == 0) |
There was a problem hiding this comment.
I don't quite like the style here. I would use {} for both branches.
|
Could you maybe rebase this PR as well so that it runs through CI and we see that there are no obvious problems? |
|
Is this still being worked on? EDIT: seems that no GDNative binding is available for Dictionary duplication... so this has to be implemented in the Godot repository itself... oh well EDIT2: I've started a pull request to implement the native binding for duplication here: godotengine/godot#28738 |
|
Similarly, I've started a discussion issue to implement importing functionalities from JSON, here: Whenever this gets through (probably in 3.2) it would be a good idea to implement both the .duplicate() and .from_json() class methods. This pull request (the one from lupoDharkael ) can already be merged now (after the rebase) without breaking now so I propose to leave implementing the missing Dictionary class methods for a different pull-request at a later data. (Somewhere after 3.2) EDIT: It seems like .to_json() is there for compatibility issues and shouldn't be used. So it's only the duplication that still has to be added. |
|
I wasn't at home the last few days. I'll update the PR later. |
|
I just saw I deleted my cloned repo after a github cleanup I did a few months ago. I guess I'll need to open a new PR and close this one. |
solves #189
Still a work in progress.