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 area/body_shape_entered/exited signals parameter names and doc #53054

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

MaxLap
Copy link
Contributor

@MaxLap MaxLap commented Sep 25, 2021

This is improving the documentation (and, I believe, the signal templates) for these signals:

  • area_shape_entered
  • area_shape_exited
  • body_shape_entered
  • body_shape_exited

Here are the improvements, I made a separate commit per improvement:

  • Some doc used body_id or area_id instead of the _rid that is written elsewhere
  • I added a _index suffix to the body_shape, area_shape and local_shape parameters. This has 2 benefits:
    • More intuitive when reading the name
    • Allows to then use var body_shape: Node = body.shape_owner_get_owner(body_shape_index). Before, body_shape would have been taken by the int parameter.
  • Added code to get the actual node from those _index arguments.
    It took me a while to find it when I needed it, and when I did, I wasn't sure if that was valid for more recent Godot versions until I tried it.

All of those changes would also be great improvements for Godot 3.3 documentation (and maybe farther back, but I am not familiar with older versions)

@MaxLap MaxLap requested review from a team as code owners September 25, 2021 13:23
@mhilbrunner
Copy link
Member

Hey! Thanks for contributing.

  1. Can you squash the commits into one, please? :)
  2. The commit message and PR title should clarify that this also changes the signal/naming, not only the documentation.

@MaxLap
Copy link
Contributor Author

MaxLap commented Oct 1, 2021

Sure, squashing is done.

@mhilbrunner mhilbrunner changed the title Improve *_shape_* signals documentation Improve area/body_shape_entered/exited signals parameter names and doc Oct 7, 2021
@mhilbrunner
Copy link
Member

mhilbrunner commented Oct 7, 2021

Thanks for the squash and fixup!

All of those changes would also be great improvements for Godot 3.3 documentation (and maybe farther back, but I am not familiar with older versions)

It is IMO unlikely to be backported to 3.x as all these changes break compatability by changing method argument names, which we generally try to avoid in minor releases AFAIK.

(This PR is somewhat relevant to #16863)

@MaxLap
Copy link
Contributor Author

MaxLap commented Oct 7, 2021

Alright, thanks for the feedback.

I'm surprised that the name of arguments is considered to be part of the signature. Languages usually only work by position, unless they have named arguments. I mean, I can change the parameter name of a signal's function and everything seems to still work.

Anwyays, it's very minimal so probably not worth the time for the possible merge conflicts

@mhilbrunner
Copy link
Member

No, thats actually a good point - I spent too much time with Python recently, where this is more of a concern. :)

I'll add a backport request label for now optimistically, let's see if someone disagrees :P

@mhilbrunner mhilbrunner added cherrypick:3.x Considered for cherry-picking into a future 3.x release and removed breaks compat labels Oct 7, 2021
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Just left a comment for a minor improvement in the documentation, otherwise it should be good to go.

doc/classes/Area2D.xml Outdated Show resolved Hide resolved
@pouleyKetchoupp
Copy link
Contributor

Also, some class names have changed on master, so we won't be able to cherry-pick the commit directly. Feel free to open a separate PR with the equivalent changes on the 3.x branch.

@MaxLap MaxLap requested review from pouleyKetchoupp and removed request for a team October 14, 2021 19:11
@MaxLap
Copy link
Contributor Author

MaxLap commented Oct 14, 2021

Added the change and squashed everything together

@pouleyKetchoupp
Copy link
Contributor

@MaxLap Thanks! My comment was probably not clear enough, but I meant to suggest the same change for all the occurences of actual node with either [CollisionShape2D] or [CollisionShape3D].

@MaxLap
Copy link
Contributor Author

MaxLap commented Oct 14, 2021

Lol, my bad. Your comment was perfectly fine and I understood it, but I forgot in 5 seconds. Not my proudest moment xD

Done. Changed all "actual node" this time :)

Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

No worries :) I've added comments on a few last things to fix before merging.

doc/classes/Area2D.xml Show resolved Hide resolved
doc/classes/Area3D.xml Outdated Show resolved Hide resolved
doc/classes/RigidDynamicBody3D.xml Outdated Show resolved Hide resolved
Fix some typoed names from the doc
Add _index to "index" parameters of *_shape_* signals, this is both in doc and in the template. This makes the code, signature and doc easier to understand
Add method to get Node from the _index params of those signals. This was not as easy to find as one would expect. Putting this information where it is needed will help.
@MaxLap
Copy link
Contributor Author

MaxLap commented Oct 14, 2021

Good catches. Fixed the 3D and the missing "node".

Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Looks all good!

@pouleyKetchoupp pouleyKetchoupp merged commit 342c1bf into godotengine:master Oct 14, 2021
@pouleyKetchoupp
Copy link
Contributor

pouleyKetchoupp commented Oct 14, 2021

Congrats on your first merged PR!

Please let me know if you're up for doing the 3.x version, otherwise I can take care of it.

@MaxLap
Copy link
Contributor Author

MaxLap commented Oct 14, 2021

I can do it tomorrow. Is there any diff other than CollisionShape3D being just CollisionShape?

@pouleyKetchoupp
Copy link
Contributor

Thanks! Some file names are also different, but I think that should be it.

@MaxLap
Copy link
Contributor Author

MaxLap commented Oct 15, 2021

Here is the PR for 3.x: #53848

@Calinou Calinou removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Oct 15, 2021
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.

4 participants