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

Overhaul AABB's documentation #87114

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 12, 2024

Very much related to #69816

This PR almost completely overhauls AABB's class reference documentation, and makes it match the current state of Rect2 and Rect2i.

Some of the documentation is ported 1:1 from Rect2's, but there are notable differences from Rect2:

  • Say "bounding box" instead of "rectangle";
    • The current documentation likes to say "AABB" a lot but its quite confusing to imagine.
  • Reduce the scale of some examples;
    • With AABB we're usually talking about meters, which can be a lot larger than pixels. And besides, using 2 digits for every value, out of 6...? It's a lot to follow.
  • Avoid mentions of "bottom-left-back", "top-right-forward" corners.
    • With 3 dimensions envisioning and writing "bottom-left and back" corner is quite difficult to understand.
  • Change parameter names around:
    • For some reason, Rect2 and AABB have different parameter names for some methods (e.g. "b" becomes "with"). Descriptions had to be adjusted accordingly, and they may or may not sound more awkward as a result.

Not everything was copied word-for-word, however.

  • Clarify intersect_ray() and intersect_segment(), get_endpoints(), etc.;
  • Clarify has_volume:
    • "Returns [code]true[/code] if the [AABB] has a volume" thanks, I guess...
  • Aadd an example for all *_axis_* methods;
  • Fix misinformation in has_point:
    • "Points on the faces of the AABB are considered included, though float-point precision errors may impact the accuracy of such checks.". Oh the "inaccuracy" excuse.

      godot/core/math/aabb.h

      Lines 320 to 328 in 26b1fd0

      if (p_point.x > position.x + size.x) {
      return false;
      }
      if (p_point.y > position.y + size.y) {
      return false;
      }
      if (p_point.z > position.z + size.z) {
      return false;
      }

      If that statement were true, these would be >=, not >.

Feedback is very, very welcome. Thank you for the support in advance.

@Mickeon Mickeon requested a review from a team as a code owner January 12, 2024 16:20
@AThousandShips AThousandShips added this to the 4.3 milestone Jan 12, 2024
@AThousandShips AThousandShips added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 12, 2024
Copy link
Member

@AThousandShips AThousandShips 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 generally! Will take a focused look when I have the time, thank you this is a class that needs some love

doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
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.

I have only checked the C# examples.

doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 15, 2024

I modified the PR to change encloses after #87118

doc/classes/AABB.xml Outdated Show resolved Hide resolved
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Style and wording looks good, haven't verified the results

Note for cherry picking:
The encloses code is not applicable without cherry picking the changes to 4.1, e.g.

@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 17, 2024
@YuriSizov YuriSizov merged commit 649bcbd into godotengine:master Jan 17, 2024
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@Mickeon Mickeon deleted the doc-peeves-AABB branch January 17, 2024 17:59
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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