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

Solve discrepancy between code and class reference for Plane #63381

Merged
merged 1 commit into from
Jul 24, 2022

Conversation

jynus
Copy link
Contributor

@jynus jynus commented Jul 24, 2022

On #43310, class reference was automatically updated from source,
causing xml documentation to disagree with parameter naming
description on Plane.intersects_segment().

The issue was a misleading binding argument name: while
Plane.is_point_over() is defined on math.Plane as
"const Vector3 &p_point", it was bound with the "plane"
argument indentifier.

  • Update begin/end to from/to on Plane.intersects_segment(...)
    docs description to match source
  • Change Plane.is_point_over(plane) to Plane.is_point_over(point)
    AND its description

Fixes godotengine/godot-docs#5976

@jynus jynus requested a review from a team as a code owner July 24, 2022 09:56
@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 24, 2022

Note that you can't simply rename the argument in the XML documentation. You need to rename it in the source where the method binding happens. If you run doctool locally, you will see that it will erase your change. For the same reason the CI won't pass.

@Calinou Calinou added this to the 4.0 milestone Jul 24, 2022
On godotengine#43310, class reference was automatically updated from source,
causing xml documentation to disagree with parameter naming
description on Plane.intersects_segment().

Weirdly, it also changed the parameter for Plane.is_point_over()
from point to plane, when only the first has sense (and it is
defined on math.Plane as "const Vector3 &p_point"). Manual
mistake?

* Update begin/end to from/to on Plane.intersects_segment(...)
  docs description to match source
* Update Plane bindings to use points instread of plane for
  is_point_over(...)
* Change Plane.is_point_over(plane) to Plane.is_point_over(point)
  AND its description on docs

Fixes godotengine/godot-docs#5976
@jynus
Copy link
Contributor Author

jynus commented Jul 24, 2022

If you run doctool locally, you will see that it will erase your change

I see, thanks a lot for pointing it out. Apologies, I thought the doctool worked based on the cpp parameter, not its bindings- I am not super-familiar with the workflow. Now ./bin/godot.linuxbsd.tools.64 --doctool keeps the state after the patch (but I may have broken something else).

@jynus
Copy link
Contributor Author

jynus commented Jul 24, 2022

It works, at least locally?

tests

Screenshot_20220724_184155

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Yeah, all good now!

For the record, it seems begin/end were the names before, but were changed in #42780. Then #43310 corrected the docs, but not the descriptions.

@akien-mga akien-mga merged commit bd1d2fc into godotengine:master Jul 24, 2022
@akien-mga
Copy link
Member

Thanks!

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.

Plane, some parameters name are inconsistent
4 participants