weather: Fix incorrect rounding for negative temperatures

[FintasticMan]

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

[github-actions[bot]]

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

[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 :laughing:.

[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 :+1: