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

GDScript Built-in: add inverse_lerp & range_lerp #10225

Merged
merged 1 commit into from Aug 21, 2017
Merged

GDScript Built-in: add inverse_lerp & range_lerp #10225

merged 1 commit into from Aug 21, 2017

Conversation

ghost
Copy link

@ghost ghost commented Aug 10, 2017

WIP

It's useful to me and some other people.
Fixes #10195

@vnen
Copy link
Member

vnen commented Aug 10, 2017

I don't like the rlerp name, it's too cryptic. I would prefer range_lerp or simply remap.

@ghost
Copy link
Author

ghost commented Aug 10, 2017

remap sounds nice as it has the original map name and might be harder to get mixed-up with other uses of "Map". Will change to that.

Edit: With inverse_lerp, I decide to use range_lerp for consistency.

@groud
Copy link
Member

groud commented Aug 10, 2017

Also, I figured out that Unity has an InverseLerp function. Maybe you could add this function too ?
In that case I think range_lerp would make more sense, so that all three functions end with "lerp", it helps understanding that those finctions works together.

Also that would mean that you could implement range_lerp with something like:

static _ALWAYS_INLINE_ double rlerp(double p_value, double p_istart, double p_istop, double p_ostart, double p_ostop) {
return lerp (ostart, ostop, inverse_lerp (istart, istop, p_value)); 
}

(Don't copy as is, it might not work)

@ghost
Copy link
Author

ghost commented Aug 11, 2017

I don't find this function useful, and I don't agree that it should be in the core for reasons I've reiterated several times in #10195.

@ghost
Copy link
Author

ghost commented Aug 11, 2017

@groud I'm not sure what InverseLerp would return if, InverseLerp(min,max,val)

  1. min is equal to max
  2. All params are equal

@ghost ghost changed the title GDScript Built-in: add rlerp (map) function GDScript Built-in: add inverse_lerp & range_lerp Aug 11, 2017
@fracteed
Copy link

I agree with @vnen that remap is a much better name for it, or even rangemap which is the name I tend to use.

@groud
Copy link
Member

groud commented Aug 11, 2017

@Noshyaar min should not be equal to max. Just raise an error if so with ERR_COND...(i don't remember the exact name) and return 0 or something like that.

@bojidar-bg
Copy link
Contributor

bojidar-bg commented Aug 11, 2017

@groud Well, if they are equal, you get a division by zero, but since you use floating-point numbers, you get either +Infinity or -Infinity, no? (on NaN, in case you try 0/0)

@groud
Copy link
Member

groud commented Aug 11, 2017

Well in any case having min==max is not a valid argument, an error should be raised.
The result of a division by 0 is by definition undefined, so the function can return anything, this will allways be wrong and the error sould be handled. :) The best would be to have a NaN value but this is not possible in C++.

My bad, you're right:
https://stackoverflow.com/questions/42926763/the-behaviour-of-floating-point-division-by-zero
Stilll it should raise an error I think.

@Zylann
Copy link
Contributor

Zylann commented Aug 11, 2017

Just a note, working with Unity for years and never used InverseLerp, even though I know it exists. Must be the habit of doing things explicitely though.

If they are added, I think inverse_lerp is better than remap, because it refers to that same well known existing operation, there is no map function, and it's a quite small name to be put in global space. Also, it only concerns floats (not ints, not vectors, not colors, nothing interpolable but floats).

@akien-mga akien-mga added this to the 3.0 milestone Aug 17, 2017
@groud
Copy link
Member

groud commented Aug 18, 2017

Could you rename the parameters of lerp and inverse_lerp to make them a little bit more explicit ?
Maybe like what you did with range_lerp ?

@akien-mga
Copy link
Member

akien-mga commented Aug 21, 2017

As discussed on IRC (from 0:14), since we provide lerp() it makes sense to include inverse_lerp, which is lerp's inverse operation and can be used to undo a lerp().

It's also a common operation found in other math libraries (Unity, Arduino).

@akien-mga akien-mga merged commit 135027a into godotengine:master Aug 21, 2017
@ghost ghost deleted the map branch August 22, 2017 01:19
@ghost ghost mentioned this pull request Feb 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants