Added do_clamp to vector lerping #3610
Conversation
📝 WalkthroughWalkthroughAdds an optional boolean parameter Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant VectorLerp as Vector.lerp (C)
Note over Caller,VectorLerp: New optional parameter `do_clamp` (defaults True)
Caller->>VectorLerp: lerp(self, other, t[, do_clamp])
VectorLerp->>VectorLerp: Parse args (self, other, t, [do_clamp])
alt do_clamp == True
VectorLerp->>VectorLerp: If t < 0 or t > 1 -> raise ValueError
VectorLerp->>VectorLerp: Compute clamped interpolation
VectorLerp-->>Caller: return new Vector
else do_clamp == False
VectorLerp->>VectorLerp: Compute interpolation without range check (may extrapolate)
VectorLerp-->>Caller: return new Vector
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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)
docs/reST/ref/math.rst (1)
880-890: Missing documentation update for Vector3.lerp.The
Vector3.lerpmethod documentation has not been updated to reflect the newdo_clampparameter, but it should be sinceVector3.lerpuses the same C implementation (vector_lerpfunction insrc_c/math.c) asVector2.lerp.Add the corresponding documentation updates for
Vector3.lerp:.. method:: lerp | :sl:`returns a linear interpolation to the given vector.` | :sg:`lerp(Vector3, float, /) -> Vector3` + | :sg:`lerp(Vector3, float, bool, /) -> Vector3` Returns a Vector which is a linear interpolation between self and the given Vector. The second parameter determines how far between self an other the result is going to be. It must be a value between ``0`` and ``1``, where ``0`` means self and ``1`` means other will be returned. + .. versionchanged:: 2.5.7 The ``do_clamp`` parameter is added with a default + value of ``True``. When it's ``True``, the second parameter is limited + to the closed interval ``[0, 1]``. When it's ``False``, the second parameter + is not limited and the lerp will extend beyond the original two vectors. + .. ## Vector3.lerp ##
🧹 Nitpick comments (1)
src_c/math.c (1)
1720-1724: Note: Behavioral difference from scalar lerp.The vector
lerpraises aValueErrorwhentis out of range anddo_clampis true, while the scalarlerpfunction (lines 4514-4524) silently clamps the value to[0, 1]. This means:
pygame.math.lerp(0, 10, 1.5, do_clamp=True)returns10.0(clamped)Vector2(0, 0).lerp(Vector2(10, 10), 1.5, do_clamp=True)raisesValueErrorWhile this maintains backward compatibility for vector lerp (which previously always raised errors), it creates an inconsistency in the API where the same parameter name has different enforcement behaviors.
Consider either:
- Documenting this behavioral difference clearly in user-facing docs
- Or harmonizing the behavior in a future major version
For now, the implementation is acceptable since it preserves backward compatibility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src_c/doc/math_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (4)
buildconfig/stubs/pygame/math.pyi(1 hunks)docs/reST/ref/math.rst(1 hunks)src_c/math.c(1 hunks)test/math_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
buildconfig/stubs/pygame/math.pyi (2)
buildconfig/stubs/pygame/typing.pyi (1)
SequenceLike(31-39)src_py/typing.py (1)
SequenceLike(31-39)
test/math_test.py (2)
buildconfig/stubs/pygame/math.pyi (2)
lerp(84-90)lerp(338-338)src_c/math.c (1)
lerp(4369-4373)
⏰ 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: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: Pyodide build
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: x86
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: AMD64
- GitHub Check: dev-check
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
🔇 Additional comments (5)
src_c/math.c (1)
1711-1724: LGTM! Implementation correctly adds optional clamping control.The changes properly implement the optional
do_clampparameter with the following correct behaviors:
- Defaults to
1(true) for backward compatibility- Uses
|pformat specifier to parse boolean parameter- Only validates range
[0, 1]whendo_clamp > 0- Provides clear error message mentioning the default value
docs/reST/ref/math.rst (1)
389-400: LGTM! Vector2.lerp documentation is clear and accurate.The documentation correctly:
- Shows both signature overloads
- Explains the
do_clampparameter behavior- Uses appropriate version change notation
- Clarifies default behavior
buildconfig/stubs/pygame/math.pyi (1)
84-90: LGTM! Type stub correctly reflects the C implementation.The updated method signature accurately represents:
- The new optional
do_clamp: bool = Trueparameter- Position-only parameters with
/- Generic return type
_TVec- Proper parameter ordering
Since
_GenericVectoris used by bothVector2andVector3, this correctly applies the signature to both classes.test/math_test.py (2)
1142-1143: LGTM! Correct extrapolation tests for Vector2.lerp.The new test cases correctly verify that
lerpwithclamp=Falseallows extrapolation beyond the [0,1] range. The expected values are mathematically correct and the tests complement the existing ValueError test at line 1141 for the default clamped behavior.Also applies to: 1148-1148, 1153-1153
2899-2900: LGTM! Correct extrapolation tests for Vector3.lerp.The new test cases properly verify that
lerpwithclamp=Falseenables extrapolation for Vector3, mirroring the Vector2 tests. All expected values are mathematically correct and the tests work in conjunction with the existing ValueError test at line 2898 for default clamped behavior.Also applies to: 2905-2905, 2910-2910
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/reST/ref/math.rst(2 hunks)
⏰ 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: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: build (macos-14)
- GitHub Check: dev-check
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Pyodide build
|
Here's a piece of test code I wrote: import pygame
pygame.init()
win = pygame.Window()
screen = win.get_surface()
font = pygame.font.SysFont("Arial", 20)
vec1 = pygame.Vector2(screen.get_rect().center)
vec2 = pygame.Vector2(screen.get_rect().midright) / 1.5
def generate_lerped_values(vec1, vec2, do_clamp):
points = []
x = -1.0
DX = 0.01
while x < 2.0:
try:
points.append(vec1.lerp(vec2, x, do_clamp))
except ValueError:
pass
x += DX
return points
did_update = True
do_clamp = True
running = True
while running:
for event in pygame.event.get():
if event.type == pygame.QUIT:
running = False
if event.type == pygame.MOUSEBUTTONDOWN:
if event.button == 3:
vec1.xy = event.pos
did_update = True
elif event.button == 1:
vec2.xy = event.pos
did_update = True
if event.type == pygame.KEYDOWN:
if event.key == pygame.K_SPACE:
do_clamp = not do_clamp
did_update = True
screen.fill("black")
if (did_update):
points = generate_lerped_values(vec1, vec2, do_clamp)
did_update = False
for point in points:
pygame.draw.circle(screen, "blue", point, 2)
pygame.draw.line(screen, "red", (0, 0), vec1)
pygame.draw.line(screen, "green", (0, 0), vec2)
screen.blit(font.render(f"{do_clamp = }", True, "white"))
win.flip()Left click will move the target, right click will move the source, and spacebar will toggle |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/reST/ref/math.rst (1)
390-398: Document the range restriction conditionally ondo_clamp.This paragraph still states the second parameter “must be a value between 0 and 1,” even though
do_clamp=Falsenow allows values outside that interval. Please call out that the [0, 1] requirement only applies whendo_clampremainsTrue, and that disabling it permits extrapolation.- given Vector. The second parameter determines how far between self and - other the result is going to be. It must be a value between ``0`` and ``1`` - where ``0`` means self and ``1`` means other will be returned. + given Vector. The second parameter determines how far between self and + other the result is going to be. When ``do_clamp`` is ``True``, it must + be a value between ``0`` and ``1`` where ``0`` means self and ``1`` means + other will be returned. When ``do_clamp`` is ``False``, this parameter + may fall outside ``[0, 1]`` and the interpolation can extrapolate beyond + the original vectors.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src_c/doc/math_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (1)
docs/reST/ref/math.rst(2 hunks)
⏰ 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_64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: dev-check
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: i686
- GitHub Check: Pyodide build
oddbookworm
force-pushed
the
vector_lerp_noclamp
branch
from
ecca3f2 to
72a6719
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reST/ref/math.rst (1)
118-118: Fix wording and capitalization: clamp applies to the parameter, not the result.Change to state that
valuemay exceed [0,1] and capitalize False.- If ``do_clamp`` is false, the result can exceed the range 0.0 to 1.0. + If ``do_clamp`` is ``False``, the parameter ``value`` is not limited to the closed + interval ``[0, 1]`` and the interpolation may extrapolate beyond ``a`` and ``b``.
♻️ Duplicate comments (1)
docs/reST/ref/math.rst (1)
389-389: Show the named default in the overload signature.Use
do_clamp=True, /for clarity and to match scalarmath.lerp.- | :sg:`lerp(Vector2, float, bool, /) -> Vector2` + | :sg:`lerp(Vector2, float, do_clamp=True, /) -> Vector2`
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src_c/doc/math_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (1)
docs/reST/ref/math.rst(2 hunks)
⏰ 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). (19)
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: dev-check
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: Pyodide build
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
🔇 Additional comments (3)
docs/reST/ref/math.rst (3)
391-397: Range clarification reads well now.The conditional [0,1] requirement when
do_clampis True and extrapolation note when False are clear.
888-894: Good clarification of clamping behavior.The explanation of [0,1] when clamped and extrapolation when not clamped is consistent and clear.
398-401: Confirm version number in.. versionchanged::.Please verify
2.5.7is the intended release tag for this change across both Vector2 and Vector3 sections.Also applies to: 895-898
oddbookworm
force-pushed
the
vector_lerp_noclamp
branch
from
72a6719 to
73ed3dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reST/ref/math.rst (1)
115-118: Clarify whatdo_clampactually affects.The current text says the result can exceed 0.0–1.0, but the clamp actually applies to the
valueparameter. Please describe the clamping/extrapolation behavior in terms ofvalueso readers know what is constrained.- If ``do_clamp`` is false, the result can exceed the range 0.0 to 1.0. + When ``do_clamp`` is ``True``, ``value`` is clamped to the closed interval + ``[0.0, 1.0]``. When ``do_clamp`` is ``False``, ``value`` is not clamped and + the interpolation may extrapolate beyond ``a`` and ``b``.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src_c/doc/math_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (1)
docs/reST/ref/math.rst(2 hunks)
⏰ 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: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-latest)
- GitHub Check: aarch64
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: dev-check
- GitHub Check: Pyodide build
| given Vector. The second parameter determines how far between self and | ||
| other the result is going to be. It must be a value between ``0`` and ``1`` | ||
| where ``0`` means self and ``1`` means other will be returned. | ||
| when ``do_clamp`` is ``True`` where ``0`` means self and ``1`` means other | ||
| will be returned. When ``do_clamp`` is ``False``, the lerp will extend out | ||
| linearly when the second parameter is outside of the nominal range. |
There was a problem hiding this comment.
Align lerp docs with clamping behavior.
These paragraphs still claim the factor “must be” in [0, 1], which contradicts the actual behavior (the value is clamped when do_clamp is True and allowed to extrapolate when False). Update the prose to describe the clamping instead.
- other the result is going to be. It must be a value between ``0`` and ``1``
- when ``do_clamp`` is ``True`` where ``0`` means self and ``1`` means other
- will be returned. When ``do_clamp`` is ``False``, the lerp will extend out
- linearly when the second parameter is outside of the nominal range.
+ other the result is going to be. When ``do_clamp`` is ``True``, the second
+ parameter is clamped to the closed interval ``[0, 1]`` so that ``0``
+ returns self and ``1`` returns other. When ``do_clamp`` is ``False``, the
+ lerp will extrapolate linearly when the second parameter is outside of that
+ nominal range.Also applies to: 887-891
Starbuck5
added
the
New API
|
Is lerping between two vectors without respecting the endpoints / clamping useful? |
|
I think I remember wanting to use such a feature before. But in general, it is just a math operation that you can use to multiply and then apply the offset between two vectors, so it would be good to allow any multiplier, not just within |
3 participants
Someone on discord asked why we force clamp with vector lerping, but have an option to disable clamping on vector lerping. This PR brings the vector lerp into parity with the scalar lerp