Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove pragma ignoring -Wnarrowing #48

Closed
Lgt2x opened this issue Apr 17, 2024 · 7 comments
Closed

Remove pragma ignoring -Wnarrowing #48

Lgt2x opened this issue Apr 17, 2024 · 7 comments
Labels
suggestion Suggestion open for discussion

Comments

@Lgt2x
Copy link
Collaborator

Lgt2x commented Apr 17, 2024

#21 turned off the -Wnarrowing compiler warning in the file libmve/snd8to16.h to enable building on Linux. The short array snd_8to16 uses values that exceed signed short limits.

Figure out a way to remove the pragma, either by increasing the array size to 4-byte ints or clamping values to [-32768;32767]

@kevinbentley kevinbentley added the suggestion Suggestion open for discussion label Apr 20, 2024
@JeodC
Copy link
Member

JeodC commented Apr 23, 2024

Will this work for clamping?

#include <stdint.h>

// Clamp values
int16_t clamp(int32_t value) {
    if (value < -32768)
        return -32768;
    else if (value > 32767)
        return 32767;
    else
        return (int16_t)value;
}

signed short snd_8to16[256] = {
    0,      1,      2,      3,      4,      5,      6,      7,      8,      9,      10,     11,     12,     13,
    14,     15,     16,     17,     18,     19,     20,     21,     22,     23,     24,     25,     26,     27,
    28,     29,     30,     31,     32,     33,     34,     35,     36,     37,     38,     39,     40,     41,
    42,     43,     47,     51,     56,     61,     66,     72,     79,     86,     94,     102,    112,    122,
    133,    145,    158,    173,    189,    206,    225,    245,    267,    292,    318,    348,    379,    414,
    452,    493,    538,    587,    640,    699,    763,    832,    908,    991,    1081,   1180,   1288,   1405,
    1534,   1673,   1826,   1993,   2175,   2373,   2590,   2826,   3084,   3365,   3672,   4008,   4373,   4772,
    5208,   5683,   6202,   6767,   7385,   8059,   8794,   9597,   10472,  11428,  12471,  13609,  14851,  16206,
    17685,  19298,  21060,  22981,  25078,  27367,  29864,  32589,  35563,  38808,  42350,  46214,  50431,  55033,
    60055,  65535,  -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768,
    -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768,
    -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768,
    -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768,
    -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768,
    -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768,
    -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768,
    -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768,
    -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768,
    -32768, -32768, -32768, -32768, -32768, -32768, -32768, -32768};

@JeodC
Copy link
Member

JeodC commented Apr 25, 2024

@Lgt2x

@Arcnor
Copy link
Collaborator

Arcnor commented Apr 25, 2024

Will this work for clamping?

That just clamps everything, but this needs careful testing to see if the code is actually clamping or wrapping around, even incorrectly.

@Lgt2x
Copy link
Collaborator Author

Lgt2x commented Apr 25, 2024

We need to figure out precisely what the array is used for, and what the best solution is: clamping, re-interpolate all values in the short range, controlling overflow etc. I opened the issue to point out the problem, but it needs analysis. For example jumping from 65535 to -32768 might not be what the game intends

@th1000s
Copy link
Contributor

th1000s commented Apr 25, 2024

The comment in snd8to16.h still has the original generation algorithm, and using it with std::clamp() like this

recreate_snd8to16.cpp
#include <stdio.h>
#include <math.h>

#include <algorithm>
#include <iostream>
#include <iomanip>

int main(void)
{
  signed int snd_8to16[256];
  int i;
  for (i=0; i<44; ++i) {
    snd_8to16[i] = i;
  }
  for (i=44; i<128; ++i) {
    snd_8to16[i] = std::clamp( (int)floor(pow(65535,i/127.0)+.5),
             (int)std::numeric_limits<short>::min(), (int)std::numeric_limits<short>::max());
  }
  for (i=1; i<128; ++i) {
    snd_8to16[256-i] = -snd_8to16[i];
  }
  snd_8to16[128] = snd_8to16[129];

  int c = 1;
  for (signed short s : snd_8to16) {
    std::cout << std::setfill(' ') << std::setw(6) << s << ", ";
    if (c++ % 14 == 0) {
      std::cout << std::endl;
    }
  }
  std::cout << '\n';
}

results in:

     0,      1,      2,      3,      4,      5,      6,      7,      8,      9,     10,     11,     12,     13, 
    14,     15,     16,     17,     18,     19,     20,     21,     22,     23,     24,     25,     26,     27, 
    28,     29,     30,     31,     32,     33,     34,     35,     36,     37,     38,     39,     40,     41, 
    42,     43,     47,     51,     56,     61,     66,     72,     79,     86,     94,    102,    112,    122, 
   133,    145,    158,    173,    189,    206,    225,    245,    267,    292,    318,    348,    379,    414, 
   452,    493,    538,    587,    640,    699,    763,    832,    908,    991,   1081,   1180,   1288,   1405, 
  1534,   1673,   1826,   1993,   2175,   2373,   2590,   2826,   3084,   3365,   3672,   4008,   4373,   4772, 
  5208,   5683,   6202,   6767,   7385,   8059,   8794,   9597,  10472,  11428,  12471,  13609,  14851,  16206, 
 17685,  19298,  21060,  22981,  25078,  27367,  29864,  32589,  32767,  32767,  32767,  32767,  32767,  32767, 
 32767,  32767, -32767, -32767, -32767, -32767, -32767, -32767, -32767, -32767, -32767, -32589, -29864, -27367, 
-25078, -22981, -21060, -19298, -17685, -16206, -14851, -13609, -12471, -11428, -10472,  -9597,  -8794,  -8059, 
 -7385,  -6767,  -6202,  -5683,  -5208,  -4772,  -4373,  -4008,  -3672,  -3365,  -3084,  -2826,  -2590,  -2373, 
 -2175,  -1993,  -1826,  -1673,  -1534,  -1405,  -1288,  -1180,  -1081,   -991,   -908,   -832,   -763,   -699, 
  -640,   -587,   -538,   -493,   -452,   -414,   -379,   -348,   -318,   -292,   -267,   -245,   -225,   -206, 
  -189,   -173,   -158,   -145,   -133,   -122,   -112,   -102,    -94,    -86,    -79,    -72,    -66,    -61, 
   -56,    -51,    -47,    -43,    -42,    -41,    -40,    -39,    -38,    -37,    -36,    -35,    -34,    -33, 
   -32,    -31,    -30,    -29,    -28,    -27,    -26,    -25,    -24,    -23,    -22,    -21,    -20,    -19, 
   -18,    -17,    -16,    -15,    -14,    -13,    -12,    -11,    -10,     -9,     -8,     -7,     -6,     -5, 
    -4,     -3,     -2,     -1

Then someone has to test-listen to the movies to verify :)

@JeodC
Copy link
Member

JeodC commented Apr 26, 2024

Since we are replacing libmve (really need to do that soon), is this issue needed?

@Lgt2x
Copy link
Collaborator Author

Lgt2x commented May 6, 2024

Closing, this code will go away anyway with #289

@Lgt2x Lgt2x closed this as completed May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion Suggestion open for discussion
Projects
None yet
Development

No branches or pull requests

5 participants