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 documentation to PhysicsDirectBodyState2DExtension #87030

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 10, 2024

Related to #87018

This..
This PR fills in all of PhysicsDirectBodyState2DExtension's class reference. What a mouthful. All of the descriptions end up just pointing back to the original PhysicsDirectBodyState2D. See #87018 for reasoning.

If both this PR and the prior one are acceptable, I will make a future PR for the 3D equivalents. Otherwise... I guess writing tailored documentation for these wouldn't be nearly as bad, given there's much fewer methods to talk about?

@Mickeon Mickeon requested a review from a team as a code owner January 10, 2024 09:00
@akien-mga akien-mga requested a review from a team January 10, 2024 09:04
@akien-mga akien-mga added enhancement documentation cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 10, 2024
@akien-mga akien-mga added this to the 4.3 milestone Jan 10, 2024
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 10, 2024

I tested this prior to making this PR but the test does not seem to agree.
image
Directly pointing to setters and getters in the documentation does work correctly, even if they are grouped with their property, at least in the built-in documentation. I can only assume that the online documentation does not in fact support this, so I need to change the descriptions accordingly.

@Mickeon Mickeon force-pushed the documentation-PhysicsDirectBodyState2DExtension branch from f3815cb to 9bbb2bc Compare January 10, 2024 13:33
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 10, 2024

Applied the new description to all methods that are setters/getters of a property in PhysicsDirectBodyState2D.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 10, 2024

For the rest "Overridable version of ..." still stands out to me. So I did some lookup, and what we have is:


Virtual method to be implemented by the user. ...
Virtual method to be overridden by the user. ...
Virtual method which can be overridden to ...
Virtual method to override ...
Virtual method which can be overridden to ...

Override this method to ...

Implement to ...
Implement this in GDExtension to ...


Looks like my suggestion above wasn't the most consistent option. The most popular unique variation seems to be "Override this method to". And "Implement this in GDExtension to" is used specifically by another extension class.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 10, 2024

Looks like my suggestion above wasn't the most consistent option. The most popular unique variation seems to be "Override this method to". And "Implement this in GDExtension to" is used specifically by another extension class.

Indeed, "Override this method to" is the most consistent and also my favourite, I always try to word it that way.
But for all of these *Extension classes, which already begin their description with at least something along the lines of:

"This class extends [PhysicsDirectBodyState2D] by providing additional virtual methods that can be overridden. When these methods are overridden, they will be called instead of the internal methods of the physics server."

One could also argue that introducing the method that way is needlessly verbose, too.

"Override this method to customize the behavior of"...
"Override this method to customize the behavior of"...
"Override this method to customize the behavior of"...

@YuriSizov
Copy link
Contributor

One could also argue that introducing the method that way is needlessly verbose, too.

True, I would say that we almost don't even need descriptions, we just need a link to the non-extension class to explain what each methods connects to. Which can probably be automated somehow.

I would add that calling it an overridable version is a bit of a misnomer, since that's not exactly how virtual methods work (at least not in every case). But eh, we can live with this as is for now, we shouldn't really be using this PR to harmonize all these descriptions.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 10, 2024

I'm okay with coming up with a standard description as a good starting point, so that it can be applied to #87018, as well. If it has to be with "that" description, so be it.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 22, 2024

How do we feel about this? Should the description be reverted back to what it was for consistency, at least?

@YuriSizov
Copy link
Contributor

@Mickeon Ultimately it doesn't matter much as we're all over the place with these descriptions. So if content-wise this is fine, then we should just merge it as is.

@akien-mga akien-mga merged commit d250e6d into godotengine:master Apr 8, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Apr 8, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.1.4.

@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Apr 8, 2024
@Mickeon Mickeon deleted the documentation-PhysicsDirectBodyState2DExtension branch April 8, 2024 14:09
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.

5 participants