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.

Sign up for GitHub

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

Jump to bottom

Add docs note about remap returning undefined when istart == istop #91615

Merged
merged 1 commit into from
May 8, 2024
Merged

Add docs note about remap returning undefined when istart == istop #91615

merged 1 commit into from
May 8, 2024
+4 −0

Conversation

Frozenfire92
Copy link
Contributor

@Frozenfire92 Frozenfire92 commented May 6, 2024 •
edited

Closes godotengine/godot-proposals#9680

It does not implement my initial proposal, and instead updates the documentation as suggested in the comments godotengine/godot-proposals#9680 (comment)

I haven't tested any other cases that could lead to NaN, might be worth just mentioning it can return NaN and to use is_nan, open to feedback here It was updated to be a bit more generic based on findings in the comments of this PR

@Frozenfire92 Frozenfire92 requested a review from a team as a code owner May 6, 2024 12:25
@Frozenfire92 Frozenfire92 mentioned this pull request May 6, 2024
Closed
kleonc
kleonc reviewed May 6, 2024
View reviewed changes
doc/classes/@GlobalScope.xml Outdated
@@ -1092,7 +1092,7 @@
<param index="3" name="ostart" type="float" />
<param index="4" name="ostop" type="float" />
<description>
Maps a [param value] from range [code][istart, istop][/code] to [code][ostart, ostop][/code]. See also [method lerp] and [method inverse_lerp]. If [param value] is outside [code][istart, istop][/code], then the resulting value will also be outside [code][ostart, ostop][/code]. If this is not desired, use [method clamp] on the result of this function.
Maps a [param value] from range [code][istart, istop][/code] to [code][ostart, ostop][/code]. See also [method lerp] and [method inverse_lerp]. If [param value] is outside [code][istart, istop][/code], then the resulting value will also be outside [code][ostart, ostop][/code]. If this is not desired, use [method clamp] on the result of this function. If [param istart] and [param istop] are the same, the function will return NaN, use [method is_nan] to guard against this.
Copy link
Member

@kleonc kleonc May 6, 2024 •
edited

Choose a reason for hiding this comment

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

Actually for istart == istop case it can also return INF or -INF (these and NaN can be checked against with is_finite method).

Basically1:

  • value < istart == istop then result is lerp(ostart, ostop, -INF),
  • value == istart == istop then result is lerp(ostart, ostop, NaN),
  • value > istart == istop then result is lerp(ostart, ostop, INF).

Footnotes

  1. Assuming the hardware/compiler implement correctly the IEEE 754 standard. ↩

@kleonc kleonc added this to the 4.3 milestone May 6, 2024
@Frozenfire92
Copy link
Contributor Author

@kleonc thanks for diving in!

I will update this PR to add tests for those cases, as well as update the docs to mention them.

Is [1] an issue the godot codebase has run into before? Or should I just assume yes it will be done correctly and its safe to add tests for?

@kleonc
Copy link
Member

kleonc commented May 6, 2024

I will update this PR to add tests for those cases, as well as update the docs to mention them.

Is [1] an issue the godot codebase has run into before? Or should I just assume yes it will be done correctly and its safe to add tests for?

@Frozenfire92 I'm not aware of that, but just wanted to point out that in theory it could happen. I think the simplest would be to state something like:
"For istart == istop the return value is undefined (most likely NaN, INF, or -INF)."

I mean I don't see a point in getting into details about specifics of when exactly which value is returned, it seems very unlikely that user would want this specific behavior anyway. And saying it's undefined kinda already tells "handle it yourself (according to your needs)". 🙂
Also no need to mention a method like is_finite as for detection you'd rather check istart == istop before calling remap instead of checking its result after the call.

Thus in a similar manner I don't think tests are needed for the istart == istop case (if we agree it's undefined then there's nothing to test). If desired, a comment in the relevant tests saying why such case doesn't need testing could be of course added for clarity.

@Frozenfire92 Frozenfire92 requested a review from a team as a code owner May 6, 2024 21:33
@Frozenfire92
Copy link
Contributor Author

@kleonc thanks for the suggestion, I've updated the PR to match

@Frozenfire92 Frozenfire92 changed the title Add docs note about remap returning NaN Add docs note about remap returning undefined when istart == istop May 6, 2024
kleonc
kleonc approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

Overall LGTM!
(just small comment style nitpick)

doc/classes/@GlobalScope.xml Outdated Show resolved Hide resolved
tests/core/math/test_math_funcs.h Outdated Show resolved Hide resolved
@Frozenfire92 @akien-mga @kleonc
Add notes for remap's return when istart and istop are the same
26feefa 
Co-Authored-By: Rémi Verschelde <rverschelde@gmail.com>
Co-Authored-By: kleonc <9283098+kleonc@users.noreply.github.com>
@akien-mga akien-mga merged commit 4c30718 into godotengine:master May 8, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

@kleonc kleonc kleonc approved these changes

Assignees
No one assigned
Labels
documentation enhancement
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Make remap be fault tolerant of a 0 istart istop range
3 participants
@Frozenfire92 @kleonc @akien-mga