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

Clarify documentation for Geometry2D.line_intersects_line #92001

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

GrahameGW
Copy link

line_intersects_line has parameters from and dir to define each line that implies the intersecting lines are rays (rather infinite lines, which they actually are). As this caused me confusion, I wrote some clarifying examples and attempted to improve this description. It would also be a good idea to rename these parameters from from and dir to point and slope respectively to further eliminate this confusion, and even further amend this description to point users to a function that does check rays for intersection.

@GrahameGW GrahameGW requested a review from a team as a code owner May 15, 2024 23:37
@Calinou Calinou added enhancement documentation cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels May 15, 2024
@Calinou Calinou added this to the 4.3 milestone May 15, 2024
Comment on lines 136 to 138
var intersect_right = Geometry2D.line_intersects_line(from_a, dir_a, from_b, Vector2(1, -1)) # returns Vector2(1, 0)
var intersect_left = Geometry2D.line_intersects_line(from_a, dir_a, from_b, Vector2(-1, -1)) # returns Vector2(-1, 0)
var intersect_none = Geometry2D.line_intersects_line(from_a, dir_a, from_b, Vector2.RIGHT) # returns null
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines are too long and will not display as nicely as you imagine. I would suggest putting the comment on a new line.

For this kind of example, what I have more often seen is "Variable is value", over "Returns value". The latter is completely fine, too, but it could be done only when not actually assigning the return value. It's a very opinionated suggestion, though.

Suggested change
var intersect_right = Geometry2D.line_intersects_line(from_a, dir_a, from_b, Vector2(1, -1)) # returns Vector2(1, 0)
var intersect_left = Geometry2D.line_intersects_line(from_a, dir_a, from_b, Vector2(-1, -1)) # returns Vector2(-1, 0)
var intersect_none = Geometry2D.line_intersects_line(from_a, dir_a, from_b, Vector2.RIGHT) # returns null
# intersect_right is Vector2(1, 0)
var intersect_right = Geometry2D.line_intersects_line(from_a, dir_a, from_b, Vector2(1, -1))
# intersect_left is Vector2(-1, 0)
var intersect_left = Geometry2D.line_intersects_line(from_a, dir_a, from_b, Vector2(-1, -1))
# intersect_none is null
var intersect_none = Geometry2D.line_intersects_line(from_a, dir_a, from_b, Vector2.RIGHT)

Copy link
Author

Choose a reason for hiding this comment

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

I went with the latter in the interests of further shortening the lines. I think it's more readable and it doesn't really do much for readability to assign the results and add an example comment

doc/classes/Geometry2D.xml Outdated Show resolved Hide resolved
doc/classes/Geometry2D.xml Outdated Show resolved Hide resolved
@GrahameGW
Copy link
Author

These are good suggestions, thanks guys. I'll clean things up a bit and push changes. I was struggling w/ the pre-commit hooks so I've been focused on that but now I've got it working (at least locally) so I'll go back and review this stuff

Co-authored-by: Micky <66727710+Mickeon@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release documentation enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants