-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 Rect2 & Rect2i Documentation #69816
Conversation
0ad5a83
to
fc03271
Compare
a1a9117
to
86f5f37
Compare
86f5f37
to
3537ba5
Compare
3537ba5
to
4c9c2cf
Compare
4c9c2cf
to
41cf141
Compare
Rebased. |
7449ccc
to
e56c9cc
Compare
Pushed a commit that should hopefully address all of the aforementioned issues. |
e56c9cc
to
56d364d
Compare
56d364d
to
033ad84
Compare
Updated with the aforementioned suggestions (also mirrored in the Rect2 documentation) |
doc/classes/Rect2.xml
Outdated
</member> | ||
</members> | ||
<operators> | ||
<operator name="operator !="> | ||
<return type="bool" /> | ||
<param index="0" name="right" type="Rect2" /> | ||
<description> | ||
Returns [code]true[/code] if the rectangles are not equal. | ||
Returns [code]true[/code] if the [member position] and [member size] of the rectangles are not equal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returns [code]true[/code] if the [member position] and [member size] of the rectangles are not equal. | |
Returns [code]true[/code] if either [member position] or [member size] of the rectangles are not equal, respectively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now for this one I'm not sure if the... "the" is needed after "either" or not. I also don't know what the "respectively" is referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Respectively was meant to mean that positions are compared against each other and sizes are compared against each other (position is not being compared against size). But I think I've added it firstly, after the change to "either+or" maybe it's no longer needed indeed. 🤔
Actually making them plural should do the thing I think:
Returns [code]true[/code] if the [member position] and [member size] of the rectangles are not equal. | |
Returns [code]true[/code] if either [member position]s or [member size]s of the rectangles are not equal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid appending letters to property names. I like the former more, perhaps, but I could be trying something else...
doc/classes/Rect2.xml
Outdated
@@ -211,7 +244,7 @@ | |||
<return type="bool" /> | |||
<param index="0" name="right" type="Rect2" /> | |||
<description> | |||
Returns [code]true[/code] if the rectangles are exactly equal. | |||
Returns [code]true[/code] if the [member position] and [member size] of the rectangles are exactly equal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returns [code]true[/code] if the [member position] and [member size] of the rectangles are exactly equal. | |
Returns [code]true[/code] if both [member position] and [member size] of the rectangles are exactly equal, respectively. |
(I don't think "exactly" is needed but oh well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to specify here because there's 4 very precise floating-point numbers that have to be equal to return true
. I suppose the adjective is not really necessary for Rect2i, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I get the reasoning. I mean "equal" and "exactly equal" are the same thing. 🙃 But if it's clearer then sure.
(and I skipped / haven't looked at the Rect2i part)
033ad84
to
7d60bf8
Compare
Pushed a commit that accepts most of the suggestions above (also mirrored in the Rect2i documentation) Still unconvinced on how to describe the One sentence I had in mind is "Returns |
7d60bf8
to
2e5fba1
Compare
2e5fba1
to
d4fb905
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still unconvinced on how to describe the
==
and!=
operators without using the same generic one liner as before this PR.
Same here. OTOH let's be honest: probably everyone understands what these operators do anyway. 🙃 So these could probably be left even as before this PR indeed.
Anyway, overall LGTM.
(Maybe the merger will have some suggestion regarding the wording for ==
and !=
operators... 😉)
d4fb905
to
ce95c83
Compare
Thanks! |
@@ -70,41 +82,51 @@ | |||
<return type="Rect2i" /> | |||
<param index="0" name="to" type="Vector2i" /> | |||
<description> | |||
Returns a copy of this [Rect2i] expanded so that the borders align with the given point. | |||
Returns a copy of this rectangle expanded to include the given [param to] point, if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure this change is wrong. It was specifically modified because, when expanded to the right or bottom, the to
point is not included in the rectangle by the has_point
. (and this is on purpose)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agh, this reply is technically correct. Saying "expanded to include" does imply that by calling has_point
with the same point on the new rectangle , the result may or may not be true
. I would like to think of a way to say this in a simpler way. At worst, the description needs to be mostly reverted back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuation of bla-bla-bla... #68838, #68983, #69594, #69451... You get the gist.
[param]
and several other strong references more often;Feedback is very, very welcome.