Implemented the Line.project #3402
Conversation
damusss
added
New API
|
Not my hideous drawings used as official documentation 😭 |
|
(discussion) Personal API opinion: the positional/keyword clamp argument should be made keyword-only because positional boolean arguments are bad - see https://docs.astral.sh/ruff/rules/boolean-type-hint-positional-argument/ for reasoning. |
|
I tried to satisfy every request I got here but I made one choice that might should be evaluated. def project(self, point: tuple[float, float], / clamp: bool = False) -> tuple[float, float]: ...aka saying that the first arg is positional only but I want to get this approved I guess |
| def flip_ab(self) -> Line: ... | ||
| def flip_ab_ip(self) -> None: ... | ||
| def project( | ||
| self, point: tuple[float, float], clamp: bool = False |
There was a problem hiding this comment.
Sorry I wasn't clear. Only the return type should be changed to tuple[float, float]; the argument should stay Point.
The reason is that while the input argument should be as broad and abstract of a type as possible, the return type needs to be more concrete and narrow to allow easy usage.
There was a problem hiding this comment.
Now when talking about this, not related to this PR, but maybe we should rename the typing.(Int)Point to typing.(Int)PointLike, just to be the same as the rest of typing module, and a little bit more clear to avoid stuff like that in the future. On the other hand, it is hard to do that now since the typing module is exposed in pygame API, so maybe in pg3
There was a problem hiding this comment.
I would normally agree with increasing naming consistency.
However, Point is not necessarily like the other types in that it is not a union of similar, acceptable types. It is also not a raw protocol like SequenceLike.
The main reason of course is verbosity; originally, CoordinateLike would be too long which might be the original reason, but PointLike is still long too.
clarify the docs for the Line.project methdo Co-authored-by: aatle <168398276+aatle@users.noreply.github.com>
grammar fix in the docs for the Line.project method Co-authored-by: aatle <168398276+aatle@users.noreply.github.com>
small update to the docs for Line.project Co-authored-by: aatle <168398276+aatle@users.noreply.github.com>
Yes, if clamping is on you can just return A (not any different from B) and that makes sense. If the clamping is off, we can't lie. We should throw a similar error to what vector2 does when normalizing a zero-length vector which is to throw a ValueError. ZeroDivisionError makes it seem like we have a bug, also because users don't know there's any division, value error seems consistent. and of course both paths have to be mentioned in the docs, yeah. |
|
I need help I dont understand why the github test aren't passing |
Do you need those extra imports in lines 4/5. Because I can't see them anywhere else |
yeah I will delete them my editor added them I didnt need them thanks I didnt know that can cause an issue |
|
all new requested changes anything else? |
ankith26
left a comment
ankith26
left a comment
There was a problem hiding this comment.
Left a few more code quality related reviews, but assuming those are solved this PR is looking good to me!
docs/reST/ref/geometry.rst
Outdated
|
|
||
| WARNING: This method has to have some length or the clamp parameter must be true for it to work and not throw a `ValueError` | ||
|
|
||
| .. versionadded:: 2.5.4 |
There was a problem hiding this comment.
this would need to be updated to 2.5.6 after we have consensus on this PR
Co-authored-by: Ankith <itsankith26@gmail.com>
Co-authored-by: Ankith <itsankith26@gmail.com>
Co-authored-by: Ankith <itsankith26@gmail.com>
Co-authored-by: Ankith <itsankith26@gmail.com>
…lper in src_c/line.c
ankith26
left a comment
ankith26
left a comment
There was a problem hiding this comment.
Left a point up for API discussion but apart from that this PR LGTM. I played around with the script from starbuck and got no funny nan-behaviour.
Thanks for contributing to pygame-ce and putting up with all our reviews! 🎉 😄
Removes the error for projecting to a zero length line with no clamping. Instead it behaves just like a clamped line.
WalkthroughAdds a new Line.project method to pygame geometry: type stubs, C implementation and docstrings, reST docs with figures and versionadded, and unit tests. Updates method table and documentation header macro. Tests include projections with and without clamping and a degenerate line case. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User code
participant L as Line (Python)
participant C as pg_line_project (C)
participant H as _line_project_helper (C)
User->>L: project(point, clamp=False)
L->>C: parse args (point, clamp)
alt invalid args
C-->>User: raise TypeError/ValueError
else valid args
C->>H: compute projection(point, clamp)
alt line length == 0
Note over H: Degenerate line<br/>return start point
else clamp == True
Note over H: Clamp to segment endpoints if outside
else clamp == False
Note over H: Return orthogonal projection on infinite line
end
H-->>C: (x, y)
C-->>User: (x, y)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src_c/doc/geometry_doc.h (1)
48-49: Docstring is incorrect: it says “projects the line onto the given line” (should be point onto line).This string becomes the Python help() text and is misleading. It should describe projecting a point onto the line, and the parameter type should match our public docs/stubs.
Apply this diff:
-#define DOC_LINE_PROJECT "project(point: tuple[float, float], clamp=False) -> tuple[float, float]\nprojects the line onto the given line" +#define DOC_LINE_PROJECT "project(point: Point, clamp=False) -> tuple[float, float]\nprojects a point onto the line"
♻️ Duplicate comments (2)
docs/reST/ref/geometry.rst (1)
744-752: Fix method one-liner and signature to reflect “point onto line” and the agreed types.
- The short description currently says “projects the line onto the given line” — that’s wrong.
- The signature uses
point: tuple[float, float]; to align with stubs and prior review, usePoint.- The paragraph says “boolean keyword argument clamp” but the implementation accepts clamp positionally too. Either enforce keyword-only in C or drop “keyword” here to avoid implying keyword-only.
Proposed edits:
.. method:: project - | :sl:`projects the line onto the given line` - | :sg:`project(point: tuple[float, float], clamp=False) -> tuple[float, float]` + | :sl:`projects a point onto the line` + | :sg:`project(point: Point, clamp=False) -> tuple[float, float]` - This method takes in a point and one boolean keyword argument clamp. It outputs an orthogonally projected point onto the line. + This method takes in a point and an optional boolean argument ``clamp``. It outputs the orthogonal projection of the point onto the line.src_c/line.c (1)
251-262: Clamp logic ordering is correct; avoids the squared-comparison pitfall flagged earlier.Checking
dot_product < 0first ensures we clamp toafor behind-start projections; only then compare the squared magnitudes to clamp tob. This fixes the earlier “lost sign” issue after squaring. LGTM.
🧹 Nitpick comments (5)
buildconfig/stubs/pygame/geometry.pyi (1)
200-200: Keep argument type as Point (broad) but align public docs to match; consider clarifying keyword-only intent.The stub correctly uses
point: Pointand returnstuple[float, float]. However, the reST/docs macro still showpoint: tuple[float, float], which is inconsistent and was previously requested to bePoint. Either:
- Update the docs to
Point(preferred), or- Change the stub to
tuple[float, float]if you explicitly want to narrow input (unlikely).Also, docs describe “boolean keyword argument clamp,” but the C binding currently accepts it positionally too. Decide whether you want clamp to be keyword-only; if yes, enforce it in C and then mark the stub as
def project(self, point: Point, /, *, clamp: bool = False) -> tuple[float, float]. Otherwise, keep the current stub and adjust doc wording to just “argument clamp.”docs/reST/ref/geometry.rst (1)
760-768: Minor clarity/consistency nits in examples and warning.The examples read well. To be fully consistent with the rest of the page:
- Keep “closest point” phrasing once in the body and avoid repeating it.
- The WARNING is good; consider explicitly stating it applies both when clamped and unclamped to close the loop with the implementation decision.
Optional tweak:
- WARNING: If the line has no length (i.e. the start and end points are the same) then the returned point of this function will be the same point as both ends of the line. + WARNING: If the line has no length (i.e. start and end points are the same), this function returns that point (both when ``clamp=False`` and ``clamp=True``).test/geometry_test.py (1)
2234-2236: Assert both coordinates for the degenerate, unclamped case (and add an unclamped-outside case).Right now only
xis asserted for the zero-length, unclamped path. Assertytoo to fully validate the behavior, and add one assertion covering the “outside-the-segment, unclamped” path.projected_point = bad_line.project(test_bad_line_point) self.assertEqual(math.ceil(projected_point[0]), 0) + self.assertEqual(math.ceil(projected_point[1]), 0) + # unclamped projection beyond the segment should lie on the infinite extension + projected_point = line.project(test_clamp_point1) # clamp=False by default + self.assertEqual(math.ceil(projected_point[0]), 200) + self.assertEqual(math.ceil(projected_point[1]), 200)src_c/line.c (2)
227-234: Handle near-degenerate lines using an epsilon threshold.Comparing to
0.0only catches exactly equal endpoints. If users tweak coordinates incrementally, length may underflow but not hit 0 exactly. Consider an epsilon to treat near-zero lengths as degenerate.Apply this diff (and include
<float.h>once at the top of the file):- if (squared_line_length == 0.0) { + if (squared_line_length <= DBL_EPSILON) {Additional include needed (outside this hunk):
#include <float.h>
308-309: Consider METH_FASTCALL | METH_KEYWORDS for consistency and perf with the rest of geometry.Most geometry methods use FASTCALL. While
projectis cheap, switching toMETH_FASTCALL | METH_KEYWORDSwould align with the module and avoid a one-off. Not a blocker; do it only if you have a convenient keyword parsing helper.If you choose to change it later, remember to update the signature accordingly and adapt parsing to vectorcall semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
docs/reST/ref/code_examples/project.pngis excluded by!**/*.pngdocs/reST/ref/code_examples/project_clamp.pngis excluded by!**/*.png
📒 Files selected for processing (5)
buildconfig/stubs/pygame/geometry.pyi(1 hunks)docs/reST/ref/geometry.rst(1 hunks)src_c/doc/geometry_doc.h(1 hunks)src_c/line.c(2 hunks)test/geometry_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
buildconfig/stubs/pygame/geometry.pyi (1)
buildconfig/stubs/pygame/math.pyi (2)
project(111-111)clamp(18-18)
src_c/line.c (1)
src_c/base.c (1)
pg_TwoDoublesFromObj(733-782)
test/geometry_test.py (1)
buildconfig/stubs/pygame/geometry.pyi (3)
line(21-21)Line(141-200)project(200-200)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: dev-check
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-14)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
🔇 Additional comments (2)
src_c/line.c (2)
230-236: Degenerate line handling: correct and matches consensus (return A for zero length).Returning
line->awhensquared_line_length == 0.0avoids NaNs and matches the documented behavior. Good.
271-294: Argument parsing: good validation; consider aligning error text with other geometry methods.Parsing via
PyArg_ParseTupleAndKeywordsis clear; the error messages are fine. For consistency with the rest of the module, you may want to reuse the phrasing used in similar helpers (“expected a sequence of two numbers” is already consistent forpoint). No action required.
8 participants
This PR should add the feature requested in #3019
I added the
project()method to thepygame.geometry.Lineobject I added the docs and test for the methodSummary by CodeRabbit
New Features
Documentation
Tests