gbdev / rgbds

Rednex Game Boy Development System - An assembly toolchain for the Nintendo Game Boy and Game Boy Color
https://rgbds.gbdev.io
MIT License
1.35k stars 172 forks source link

Incorrect fixed-point truncation #908

Closed ISSOtm closed 3 years ago

ISSOtm commented 3 years ago
issotm@sheik-kitty ~% rgbasm -V
rgbasm v0.5.1
issotm@sheik-kitty ~% rgbasm - <<EOF
rsset 32
PERCENT rb 1  ;\ Same with offset constants
VALUE = 20
RESULT = MUL(20.0, 0.32)
; Prints "32% of 20 = 6.40"
PRINTLN "{d:PERCENT}% of {d:VALUE} = {f:RESULT}"
heredoc> EOF
32% of 20 = 6.40015
issotm@sheik-kitty ~% rgbasm - <<EOF
rsset 32
PERCENT rb 1  ;\ Same with offset constants
VALUE = 20
RESULT = MUL(20.0, 0.32)
; Prints "32% of 20 = 6.40"
PRINTLN "{d:PERCENT}% of {d:VALUE} = {.2f:RESULT}"
EOF
32% of 20 = 6.41
ISSOtm commented 3 years ago

According to the standard, printf rounds the floating-point number itself (and I checked, it also specifies that the default rounding mode is "nearest").

https://github.com/gbdev/rgbds/blob/20fd6eabbb574a31b559c2149315d3771aa66b14/src/asm/format.c#L241-L242

Therefore, this + 0.5 is causing incorrect rounding.

@aaaaaa123456789 suggests the following function (adapted to RGBDS style):

void print_fixed_as_float(uintmax_t fixed, unsigned offset,
                          unsigned precision) {
    if (!precision) {
        fixed += (uintmax_t) 1 << (offset - 1);
        printf("%ju", fixed >> offset);
        return;
    }

    printf("%ju.", fixed >> offset);
    while (--precision) {
        fixed *= 10;
        putchar('0' + (fixed >> offset));
    }
    fixed = 10 * fixed + ((uintmax_t) 1 << (offset - 1));
    putchar('0' + (fixed >> offset));
}
Rangi42 commented 3 years ago

Every Q16.16 fixed-point number ought to be convertible to a floating-point value with zero loss of precision, since they're both internally fractional powers of 2. Would this be sufficient? (Maybe it wouldn't; I think I had a good reason for not writing it this way from the start.)

    snprintf(spec, sizeof(spec), "%%.0%zu.f", fracWidth);
    snprintf(valueBuf, sizeof(valueBuf), spec, (double)value / 65536.0);
aaaaaa123456789 commented 3 years ago

I'm still trying to parse that format specifier: what's "%%.0%zu.f" supposed to do? Wouldn't that emit something like %.02.f?

Rangi42 commented 3 years ago

Oops, the second . was a typo.

aaaaaa123456789 commented 3 years ago

In that case, is there a good reason to not simply do snprintf(valueBuf, sizeof valueBuf, "%.*f", (int) fracWidth, (double) value / 65536); in the first place?

Rangi42 commented 3 years ago

That sounds like it would work; I'll test it later (combined with the other format spec parts). Edit: yes, it alters the final digits of some test result values (as expected) but remains the correct width even combined with + # - etc.

RESULT = MUL(20.0, 0.32)
PRINTLN "32% of 20 = {f:RESULT}"
PRINTLN "32% of 20 = {.2f:RESULT}"
PRINTLN "32% of 20 = {.30f:RESULT}"
32% of 20 = 6.40015
32% of 20 = 6.40
32% of 20 = 6.400146484375000000000000000000
    unsigned ax = 20 << 16;
    unsigned bx = (unsigned)round(32 * 65536.0 / 100);
    double a = ax / 65536.0;
    double b = bx / 65536.0;
    double c = a * b;
    unsigned cx = (unsigned)round(c * 65536.0);
    printf("%.16f [$%08x] * %.16f [%08x] = %.16f [%08x]\n", a, ax, b, bx, c, cx);
20.0000000000000000 [$00140000] * 0.3200073242187500 [000051ec] = 6.4001464843750000 [00066670]