weather: Fix incorrect rounding for negative temperatures

[FintasticMan]

Thanks @tokiwee for reporting this issue!
Closes #2185 (if GitHub un-deletes it).

[github-actions]

Build size and comparison to main:

Section	Size	Difference
text	380116B	32B
data	944B	0B
bss	22544B	0B

Run in InfiniEmu

[mark9064]

Would it be worth extracting out rounding integer division to some shared code? It's somewhat annoying that this isn't part of the C++ stdlib really :(

Maybe an implementation similar to this? https://github.com/lucianpls/Rounding-Integer-Division
I haven't tried it though

[FintasticMan]

What do you think of this? I've made a RoundedDiv function in the Utility namespace, that uses C++20 concepts so that we can have keep the code type-agnostic, while making sure that it only works for expected values.

Didn't you also use a rounded division somewhere in the AoD code? Would this solution also work there?

[mark9064]

Wow that looks great. I didn't know that concepts allow this, I should read up on it. Yes I think this would work for AOD, I guess that would go in a separate PR though?

I want to double check correctness properly before approving, but otherwise LGTM

[mark9064]

I think this doesn't work as C++ demotes the dividend from signed to unsigned when the divisor is unsigned:

Suppose the following

   int a;
   // -1 / 5, rounded
   a = (-1 + (-1 >= 0 ? 5 : -5) / 2) / 5;
   // a = 0 as expected
   a = (-1 + (-1 >= 0 ? 5U : -5U) / 2) / 5U;
   // uh oh, a = 429496728

   // more simply
   a = -1 / 2u;
   // a = 2147483647

I didn't even know this was a thing??

[FintasticMan]

Oh yeah of course. Integer promotion rules are really stupid. This conversion to unsigned also only happens when the values are ints or smaller, because that makes sense. Let me think of a good way to solve this then...

[mark9064]

because that makes sense

of course... 😅 I love the C++ spec too

[jmlich]

I was curious about this piece of c++ code so I tried to check how it works, but

#include <cstdint>
#include <concepts>
#include <iostream>

    /*
    return dividend / divisor rounded correctly
    */
    static constexpr auto RoundedDiv(std::integral auto dividend, std::unsigned_integral auto divisor) -> decltype(dividend / divisor) {
      return (dividend + (dividend >= 0 ? divisor : -divisor) / 2) / divisor;
    }

int main () {
    std::cout << RoundedDiv((int16_t)250, 100u) << std::endl;
    std::cout << RoundedDiv((int16_t)150, 100u) << std::endl;
    std::cout << RoundedDiv((int16_t)50, 100u) << std::endl;
    std::cout << RoundedDiv((int16_t)20, 100u) << std::endl;
    std::cout << RoundedDiv((int16_t)-50, 100u) << std::endl;
    std::cout << RoundedDiv((int16_t)-150, 100u) << std::endl;
    std::cout << RoundedDiv((int16_t)-250, 100u) << std::endl;
    return 0;
}

It turned out it doesn't worked as I expected:

3
2
1
0
21474835
21474834
21474833

It looks like, you cannot use unsigned (i.e. 100u) as divisor, because -divisor will overflow and you will get incorrect number.

My knowledge of modern c++ is limited. It started to work when I have replaced std::unsigned_integral with std::integral in this line:

static constexpr auto RoundedDiv(std::integral auto dividend, std::integral auto divisor) -> decltype(dividend / divisor) {

and used 100 instead of 100u in those two lines

return Utility::RoundedDiv(PreciseFahrenheit(), 100);
return Utility::RoundedDiv(PreciseCelsius(), 100);

[FintasticMan]

@vkareh thanks for taking a look at this! The whole expression gets "promoted" to an unsigned integer if any of the terms are unsigned (and an int or smaller), which causes overflow for negative numbers. To solve this I've implemented the workaround you described, which is a bit less expressive in its typing, but at least works haha 😆.

[mark9064]

I think we should note the overflow properties in a docstring. abs(dividend) + abs(divisor) / 2 must be less than the type maximum I think? Also I think it would be useful to note whether both the dividend and divisor can be negative

[NeroBurner]

I think adding a docstring/comment what we expect from this function would be helpful. And as @mark9064 said what are the constraints on the inputs (if any)

[Andersama]

I'm all for ? operators but where things are strictly math based...might as well write using strictly math.

int16_t fixed_rounding(int16_t value, int16_t divisor) {
	int16_t signed_value = ((uint16_t)value >> 14u) & 0x2; //if signed 2 otherwise 0
	int16_t half_divisor = (divisor / 2);
    //add a half, subtract divisor conditionally on sign
	return (value + half_divisor - half_divisor*signed_value) / divisor;
}
//same as above except we don't calculate divisor*
int16_t fixed_rounding_b(int16_t value, int16_t divisor) {
	int16_t signed_value = (uint16_t)value >> 15u; //if signed 1 otherwise 0
	int16_t half_divisor = (divisor / 2);
	return (value + half_divisor - divisor * signed_value) / divisor;
}

[Andersama]

Or...if you trust bithacky madness, no * needed.

int16_t fixed_rounding(int16_t value, int16_t divisor) {
	int16_t signed_value = value >> 15u; // true evil: we use >>'s signed behavior to propagate the leading 1 across all bits, eg: we have 0xffff or 0x0000
	int16_t half_divisor = (divisor / 2);
	return (value + half_divisor - (divisor & signed_value)) / divisor; //we replace the * by "1" with an &
}

[mark9064]

How about these? https://github.com/lucianpls/rounding_integer_division

(LOL, just realised I proposed this right at the start and then forgot about it, sorry for the double post)

// Round half toward zero integer division
// If T signed, divisor cannot be std::numeric_limits<T>::min()
// Adapted from https://github.com/lucianpls/rounding_integer_division
// Under the MIT license
template<std::integral T> 
T RoundedDiv(T dividend, T divisor) {
    bool neg = divisor < 0;
    if (neg) {
        // overflows if divisor is minimum value for T
        divisor = -divisor;
    }

    T m = dividend % divisor;
    T h = divisor / 2;
    T res = dividend / divisor + (!(dividend < 0) & (m > h)) - ((dividend < 0) & ((m + h) < 0));

    if (neg) {
        res = -res;
    }
    return res;
}

Should work for everything other than dividing with the negative type minimum, which would be a totally fine edge case IMO

[JF002]

This implementation looks good and fixes the issue mentioned by @jmlich ! And it comes with documentation for free 👍

[mark9064]

Closing since #2390 merged :)