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 methods to get argument count of methods #87680

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jan 28, 2024

Added to:

  • Callables
  • Object
  • ClassDB
  • Script(Instance)s

Todo:

  • Document
  • Add tests
  • Implement for C# (needs confirmation)
  • Implement for GDExtension (confirmed)
  • Implement for godot-cpp (needs confirmation)

Help with how to do this in C# would be very welcome, I have some ideas but I'm not very familiar with the C# internals

To confirm the godot-cpp side, see:

Might split off the extension part into a separate PR if desired


core/object/object.h Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member Author

Unsure if the various methods should error instead of silently returning 0, but leaving as is right now

@AThousandShips
Copy link
Member Author

Tested the Extension side in godot-cpp, will open a PR for that too but can confirm it works with the relevant changes

@AThousandShips
Copy link
Member Author

I believe the GDExtension part is ready, but would appreciate some feedback on that

CC @godotengine/gdextension

Ditto for C# side

CC @godotengine/dotnet

@AThousandShips AThousandShips marked this pull request as ready for review January 28, 2024 19:18
@AThousandShips AThousandShips requested review from a team as code owners January 28, 2024 19:18
@AThousandShips
Copy link
Member Author

Lot of details to work out but ready for review by the relevant teams, will add tests maybe tomorrow, at least in GDScript

@Mickeon
Copy link
Contributor

Mickeon commented Jan 28, 2024

This may be the first time function arguments are acknowledged with method names. It has me pondering if this should be the standard across the whole class reference ("parameter" vs "argument")

@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 28, 2024

Not the first time, see get_bound_arguments_count etc.

Parameters is not used for this generally here as far as I can see

doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/ClassDB.xml Show resolved Hide resolved
doc/classes/Callable.xml Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 29, 2024

Re-added the GDExtension code, with new compatibility details, works well in my testing (but haven't tested the ScriptInstance extension part as godot-cpp doesn't utilize it)

The godot-cpp PR has been updated accordingly, if decided to keep both changes in this one PR for the extension part I'll re-add the details to the original post, but will await more feedback from the extension team

Comment on lines +716 to +736
const Script *scr = Object::cast_to<Script>(this);
while (scr != nullptr) {
bool valid = false;
int ret = scr->get_script_method_argument_count(p_method, &valid);
if (valid) {
if (r_is_valid) {
*r_is_valid = true;
}
return ret;
}
scr = scr->get_base_script().ptr();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Just calling it on the script itself and letting ScriptInstance handle the attached script as is done elsewhere

@akien-mga
Copy link
Member

So this adds GDExtensionScriptInstanceInfo3 for GDExtension. I'm not opposed to it, but it's worth considering whether we have other changes in the pipeline (implemented or planned) that would also need to change that struct, so we can bundle them all in the same release and avoid adding new structs all the time, making GDExtension full of backward compatibility structs. CC @godotengine/gdextension

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 1, 2024

Went ahead and split off the script instance part and will open a dedicated PR for it to make the review easier so we can merge this safely without timing it (Once GitHub decides to update it's database, seems things are a bit broken at the moment)

doc/classes/Callable.xml Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

I think this could now be squashed and merged?

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 9, 2024

As long as the C# side looks good I think we're good, I'll squash tomorrow if there's no questions (I have to go to bed anyway)

@AThousandShips
Copy link
Member Author

I'll squash it up soon and we can give it a spin, in my testing the c# code worked and was based on suggestions by raulsntos, so I feel confident in going forward, if there's any questions I can always split the C# off and leave the placeholders

Added to:
* `Callable`s
* `Object`s
* `ClassDB`
* `Script(Instance)`s
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

The C# changes look good to me, thanks!

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 10, 2024
@akien-mga akien-mga requested a review from KoBeWi March 10, 2024 20:03
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The implementation looks fine overall.

@akien-mga akien-mga merged commit a1c476f into godotengine:master Mar 13, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the the_angry_count branch March 13, 2024 21:35
@AThousandShips
Copy link
Member Author

Thank you! Will improve the docs and fixup the followup PR tomorrow

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.

Add a method to get a Callable's argument count
8 participants