camera clamping fix

[ArshvirGoraya]

Fix Camera pitchMin and pitchMax Clamping

Problem:

Expected behaviour of pitchMin: limits how far you can look down.
Expected behaviour of pitchMax: limits how far you can look up.

Current Behaviour: Whichever value is closer to 0, sets both the minimum and maximum of how far the camera can look up/down.

Example:

• pitchMax = 10 and pitchMin = -90: look up to 10, but can only look down to -10 (even with pitchMin set to -90).
• pitchMax = 90 and pitchMin = -10: look down to -10, but can only look up to 10 (even with pitchMax set to 90).

Why its Happening:

This happens because the code clamps the pitch value twice. Inside the update() and in Pitch's setter function.
This wouldn't be a problem if we weren't also switching the value's sign before the setter function, which results in the value being clamped in its sign-flipped state as well as its normal state.

Examples

Positive Value:

lookTarget.y = 90
pitchMax = 90
pitchMin = -20
... // Update:
lookTarget.y = Mathf.Clamp(lookTarget.y, pitchMin, pitchMax) // lookTarget.y = 90
Pitch = -lookTarget.y // In actual code we use -lookCurrent.y, which is just lookTarget.y, after smoothing is applied
... // Setter:
public float Pitch
{ set {
    // value = -90: sent here after its sign is flipped
    value = Mathf.Clamp(value, pitchMin, pitchMax) // value = -20. Should be 90.
  }
}

Negative Value:

lookTarget.y = -90
pitchMax = 90
pitchMin = -20
... // Update:
lookTarget.y = Mathf.Clamp(lookTarget.y, pitchMin, pitchMax) // -20.
... // Setter:
// value = +20
value = Mathf.Clamp(value, pitchMin, pitchMax) // value = 20. Should be -20.

Solution:

Remove the Mathf.Clamp() call inside the setter function. The update function clamps this value anyway, so the clamping is still working, but just on Update().

A problem with this solution is a mod may rely on the clamping in the setter function, not the Update(). So, an alternative solution may be better. Otherwise, those mods would have to update (e.g., clamp the value themselves before assigning to Pitch).

[petchema]

A problem with this solution is a mod may rely on the clamping in the setter function, not the Update(). So, an alternative solution may be better.

That looks like a good reason to keep the clamping inside the setter indeed, so why pick the other solution?

[ArshvirGoraya]

That looks like a good reason to keep the clamping inside the setter indeed

Two solutions that keep the clamping in the setter function are here.
I have implemented solution 2 in my last commit as it has the least problems.

Solution 1:

• Use the clamping in the setter function instead and remove the one in the update function. This has 2 problems.
• Problem 1: the min and max are swapped. Mods that set the pitchMin and pitchMax will have to swap their values. May not seem like a problem right now as currently the one with the lowest magnitude applies to both. However, if this change is implemented, modders may need to change the values around.
  • This is no longer a problem. See solution 2.
• Problem 2: this does not clamp the lookTarget value. This will make it so that the player can set the target out of range if they move their mouse too far up or down. If they move the mouse upwards, the camera will be clamped correctly, but if they watn to move the camera down, they will have to move the mouse down much more so that the target is back within range before the camera can move down again. Here is a video of this problem. You can see that the target can go out of range and the camera cannot move in the opposite direction until the target is back within range: https://streamable.com/v1hgfn

Solution 2:

• Clamp within the update function and the setter function but don't change the sign.
• Problem 1: This has the same problem with pitchMin and pitchMax being swapped, but doesn't have the lookTarget problem of solution 1. With this pitchMin controls how far you can look up, and pitchMax controls how far you can look down.

Reversed pitchMin and pitchMax solution:

• Change: lookTarget.y = Mathf.Clamp(lookTarget.y, pitchMin, pitchMax);
• To: lookTarget.y = Mathf.Clamp(lookTarget.y, -pitchMax, -pitchMin);
• This isn't as readable though, so let me know if there is a better way to do this.

I have implemented solution 2 in the previous commit and everything works as intended. I also tested with inverted mouse setting and everything works fine. Feel free to suggest any changes!!

Here is footage of everything working if anyone is curious: https://streamable.com/ujrcak