godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.05k stars 65 forks source link

Make remap be fault tolerant of a 0 istart istop range #9680

Closed Frozenfire92 closed 1 week ago

Frozenfire92 commented 1 week ago

Describe the project you are working on

A game that displays a hand of cards that changes the rotation of the card depending on the index in the hand. If there is an odd number of cards the center card should have no rotation

Describe the problem or limitation you are having in your project

To determine the rotation I am using remap in the following way. (i is the index in the hand)

var rot = remap(i, 0, hand.size() - 1, -maxRotation, maxRotation)

This works well except in the case of hand.size() returning 1 (and thus i being 0). This has necessitated special case handling by checking for nan

if is_nan(rot): rot = 0

Describe the feature / enhancement and how it helps to overcome the problem or limitation

This proposal is to determine when istart and istop are the same, and to return the halfway point of ostart and ostop

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Update the remap functions in core\math\math_funcs.h to detect when istart and istop are the same and use 0.5 in its place

static _ALWAYS_INLINE_ double remap(double p_value, double p_istart, double p_istop, double p_ostart, double p_ostop) {
    if p_istart == p_istop {
        return Math::lerp(p_ostart, p_ostop, 0.5);
    } else {
        return Math::lerp(p_ostart, p_ostop, Math::inverse_lerp(p_istart, p_istop, p_value));
    }
}
static _ALWAYS_INLINE_ float remap(float p_value, float p_istart, float p_istop, float p_ostart, float p_ostop) {
    if p_istart == p_istop {
        return Math::lerp(p_ostart, p_ostop, 0.5);
    } else {
        return Math::lerp(p_ostart, p_ostop, Math::inverse_lerp(p_istart, p_istop, p_value));
    }
}

If this enhancement will not be used often, can it be worked around with a few lines of script?

you can guard against this with is_nan() but that requires knowing about this case

Is there a reason why this should be core and not an add-on in the asset library?

This is about improving the core engine API to be friendlier to work with

Frozenfire92 commented 1 week ago

After thinking on this for a while, I'm not sure if my proposal of using the midpoint is going to be intuitive. I could see cases where people would expect ostart and others where they would expect ostop. Maybe NaN is the intended/intuitive behaviour and should be treated as such. If so it might be worth adding to the docs about this NaN case 🤔

RedMser commented 1 week ago

Protecting against NaN would only make sense if remap also clamped values in its input range. So like you said, instead of changing behavior, this should be documented accordingly.

Frozenfire92 commented 1 week ago

Good call, started a PR https://github.com/godotengine/godot/pull/91615