In the song wheel, if you scroll back and forth over an MP3 song very fast, you might get some audible corruption at the beginning of the song preview, and it might even crash the game.
This fixes both of those problems.
Resolves#610.
`RageSoundReader_MP3` has three different methods of setting the position. The default choice of `SetPosition_toc` is unable to provide an accurate result. The reason Edit Mode has such a severe desync when working with MP3's is due to the preference of using `SetPosition_toc`.
We can prevent this by using `SetPosition_hard` which works very well. Judging by comments in the file, it seems like `SetPosition_hard` was too resource intensive to use as the default method, 20+ years ago.
The only downside is, if you start from an arbitrary position (which is **only** possible in Edit Mode), you have a few milliseconds of corrupted audio, but then the sync is perfect. You can hear this in the demonstration video below.
When we start from the beginning of a file, such as playing a song, or using `Play whole song` in edit mode, `MADLIB_rewind` will be used.
`SetPosition_hard` provides an accurate result, and is consistent with the offset provided by `MADLIB_rewind`. The result is a consistent sync experience whether the MP3 is started from the beginning, or from an arbitrary position midway thru the file.
The `m_bAccurateSync` boolean dependency has been removed from SetPosition since it does not seem to provide any benefit on modern hardware. It seems fine to remove it so that we can call `MADLIB_rewind` or `SetPosition_hard` as fast as possible.
In my testing I never got `SetPosition_estimate` to be used, but I've left it in as a backup option.
This is the fixed Merge from the 2025-1-26 test build. It will more strictly check the drift with each iteration of the m_aSounds loop, whereas the original implementation can allow small amounts of desync to accumulate over time if none of them exceed the threshold.
I've fixed the issues with the DirectSound driver which a number of commits from 2016 attempted to address. Additionally, build testers strongly prefer the fixed DirectSound to WaveOut (the current default driver for Windows) in terms of both game stability and sync stability, so I'm making it the default driver as well.
StepMania commit 75a9532 from 2006 had a subtle error, the declaration of len and locked_buf will hide the previous local declaration of these variables. The issues causing the DirectSound driver to be unreliable in terms of sync were due to this. This PR renames the variables from the 2006 commit to prevent this, and also ensures all variables are properly initialized.
Renamed `locked_buf` and `len` in `MixerThread` to `locked_buf_init` and `len_init` to avoid shadowing.
Prevent use of uninitialized member variables by initializing `m_pPCM`, `m_iSampleRate`, and `m_bShutdownMixerThread` in the constructor.
Preferred in Windows, especially when we can't guarantee that we are able to perform a proper clean-up before terminating. Halt may be called with the Kill flag which indicates everything must shut down. This is the preferred approach in the Windows API.
Another preferred option is to use WaitForSingleObject with a timeout, but Halt with the Kill flag is being called when the game is about to crash, so that's not a realistic option and may cause the program to hang indefinitely.
This file handles OS side interaction as well as the creation of threads within the game - std::unique_ptr and std::mutex are introduced here for their safety and memory management benefits.
A custom wrapper PortableSignalObjectAndWait was removed in favor of directly calling SignalObjectAndWait.
In SetThreadName, we change from EXCEPTION_CONTINUE_EXECUTION to EXCEPTION_EXECUTE_HANDLER as suggested by the compiler to avoid a potential infinite loop.
ThreadImpl_Win32::Halt has been refactored to unequivocably call SuspendThread instead of TerminateThread, as TerminateThread doesn't allow for proper thread cleanup.
Don't bother with the try-catch in SetThreadName unless this is a debug build (since exceptions are disabled in ITGm, so it won't work as expected).
DebugInfoHunt was also updated since GetFileInformation directly uses it.
- GetMemoryDebugInfo is updated as GlobalMemoryStatus is deprecated.
- GetWindowsVersionDebugInfo is updated to use RtlGetVersion.
- GetFileInformation updated to use vector and string, as well as const correctness and modern Windows methods. Mostly removing Win9x-XP compatibility code.
- Also gets rid of some workarounds aimed at XP era versions of Visual Studio.
Fix a mistake in Windows version detection logic
Bringing in algorithm for std::copy, and vector for automatic memory management.
Initializing the buffer with zeroes in the same operation as resizing the buffer. Going back to using an unsigned int for realsize, to prevent the buffer from becoming an unreasonably large size.
The constructor and destructor can be empty since the vector<float>'s own destructor will take care of freeing allocated memory.
The static_cast to an int in read(int16_t *pBuf) is changed to a static_cast to an int16_t, as audio data is expressed in 16 bit integers, and the fractional part is inherently rounded towards zero by truncation.
Speed up the loading window. SetPixelV is slow. No perceptible difference in loading window behavior, but is faster. The goal here is to alleviate the case where the loading window is the bottleneck when loading songs on a fast computer.
Replacing the SM5 power-of-two code with the implementation from Bit Twiddling Hacks, since the current implementation isn't correctly handling the edge case where the input is 0, and is a little overcomplicated.
Bring in std::optional<DEVMODE> to gracefully handle the case where the function may not return anything valid. This helps to avoid undefined behavior caused by unexpected/invalid return values, as an extra protection to supplement the check if we can identify the refresh rate.
This eluded the update to use `_WIN32` preprocessor macro across the codebase.
Also adding an error message so we don't fail silently if we can't get the command line arguments.
The DirectInput / DirectSound version defs need to be exposed to other parts of the engine, so they are moved to the header instead of the cpp file.
The includes can be consolidated after that.
I'm also replacing the usage of a raw array of char *'s with an RString and using c_str() to get its value - a small change, but raw arrays are unreliable.
Directly use CommandLineToArgvW, replacing legacy code. Support Unicode natively by natively using std::string and wchar. shellapi.h is brought in for CommandLineToArgvW
Mitigate some security concerns, update API calls (RegOpenKeyExW & ShellExecuteEx), support unicode natively, const correctness, etc. - not doing anything new, just updating what's here to be more modern.
Floating point time is being stored as a float here, we can prevent precision loss by changing some of these to double during the lifetime of the function to get a more accurate result.
Floating point time is being stored as a float here, we can prevent precision loss by changing some of these to double during the lifetime of the function to get a more accurate result.
Since the EPSILON variable in the header file was declared but not assigned any value, it could lead to undefined behavior or calculation errors when the variable is used without initialization. This ensures a consistent value is always used and is also defined at compile time for further safety.
This prevents a crash that occurs when trying to exit the game while a connection attempt is being made (such as closing the game while it's in the middle of trying to connect to GrooveStats).
Following the discovery of the underlying bugs in BitmapText causing issues with performance of Step Statistics, I wanted to revisit some changes I made to ActorMultiVertex in ae6d7cecb5. Some things were reverted to prefer original behavior.
Ideally this would also have an option to allow AutoSync, AutoPlay, and Assist Tick with the debug overlay disabled, but that would require migrating that functionality elsewhere.
Minor cleanup on `GetMainAndSubTitlesFromFullTitle` to use a range-based for loop, and also is slightly improved so that separators of any length will work, whereas the original code only works when the separator is one character long.
Migrate some very frequently called macros from global.h to global.cpp. This reduces Windows release executable size by 43 KB.
After moving ASSERT_M and FAIL_M into void functions, these two virtuals in RageFileBasic.h won't compile as they don't return a value. This changes them to return a default value.
The value was determined by profiling the `Cleanup` function as well as monitoring how frequent "Audio frame out of range" errors occurred. This provides an ideal balance of low latency and prevention of out-of-range errors.
Without this here, GCC 11 and 12 will print a warning during compilation about a possible fallthrough, but we're doing that on purpose, so this simply adds an indicator to the compiler that we know what we're doing.
SAFE_DELETE -> RageUtil::SafeDelete
SAFE_DELETE_ARRAY -> RageUtil::SafeDeleteArray
Update JsonUtil.h to include RageUtil.h - MSVC doesn't need it included for some reason, but GCC and XCode does.
This function indirectly determines the amount of average audio latency, this is why changes to BitmapText can affect global offset significantly. Aim to make the function as fast as possible to minimize latency.
1) make BitmapText::DrawPrimitives noexcept
- note: RollingNumbers::DrawPrimitives has to also become noexcept as a consequence
2) Define repeated calculations as constexpr functions defined within an anonymous namespace
3) Fix some formatting stuff
Currently we are storing the default driver lists as a macro, and splitting them into RString vectors every time they are needed. This commit changes the macros in `arch_default.h` into vectors of RStrings, so that they do not need to be split each time.
I have updated all references where the default driver lists are being called so that the vector is used directly.
A new function is added to RageUtil to make a compatible vector of RStrings from user input which may be separated with a comma.
RageSoundDriver was refactored to check user input in Preferences.ini against the default driver list, and in case of incorrect spelling or failure, provides a detailed error message (including a list of valid options) in logs so the user can resolve the problem.
`fix_bogus_sound_driver_pref` is no longer needed, since we are now telling the user what their valid options are if the user provides an incorrect entry for the `SoundDrivers` preference, instead of silently attempting to guess what the user wanted. Since I think it's rare that someone wants to specify a driver to begin with, we should let them know their exact options if they put something wrong here, instead of silently failing or using an unwanted driver.
I tested this on Windows by specifying `DirectSound-sw` in Preferences.ini, and it worked as expected. I also tried putting an unusable value in, and got the error in logs letting me know what my valid driver options were. Of course, input and movies all work as expected too.
- Use fast data types where possible so the compiler can optimize for speed based on platform
- for example, 128 bits might be fastest on ARM
- good future-proofing
- Refactor GetTimeSinceStart() to be a bit faster
- multiplication is much faster than division
- Implement a RageTimer method to get the seconds value as a plain int, for the places which cast the seconds value to an int
- Changing from GetTimeSinceStartFast() to GetTimeSinceStart() where accuracy is important
- Changing from GetTimeSinceStart() to GetUsecsSinceStart() for timestamp diffs
- Adjust RageThreads to accomodate an unsigned timestamp value
- a constant for the maximum value of `uint_fast64_t` replaces `-1` to accommodate the change from signed to unsigned for the `locked_at` variable
- i have separate constants for `std::numeric_limits<std::uint_fast64_t>::max()` and `static_cast<std::uint_fast64_t>(-1)`, so the reader understands -1 represents an error code, though they evaluate to the same value, so i could remove one of the two
- Add two methods to calculate the MMSSMsMs / MMSSMsMsMs time value from usecs directly instead of inferring it from a seconds value, in RageUtil
- Use a similar counter/modulo based method for WheelNotifyIcon, similar to what i did for text_glow in NoteField in 2eeee03
- Make `g_iStartTime` static const for safety
Rename two timer functions:
GetUsecsSinceStart -> GetTimeSinceStartMicroseconds
GetMicrosecondsSinceStart -> GetSystemTimeAsMicroseconds
Remove std prefix from uint_fast64_t
Increasing security and efficiency of math in order to prevent potential errors from occurring.
1. L is a macro made to equal 8. It's only used in conjunction with other `int`'s, so it's safe to make into a constant called FILTER_LENGTH.
2. Define a very tiny number to avoid a potential mistake in ApplyKaiserWindow.
3. Define constants for BesselI0, making the functions easier to read but also improve security of the math.
4. Make some variables const and prevent redundant calculations or variable creations in ApplyKaiserWindow and GenerateSincLowPassFilter.
5. Use `double` interally in GenerateSincLowPassFilter
GitHub mistakenly thinks this is the project license. the copyright and licensing details for libmad already exists in extern/libmad, so it's not needed to keep it here as well.
For the purposes of type safety, allowing the compiler to further optimize, and preventing possible name collisions, I've converted these 13 macros to constexpr's.
1) Remove Win SDK headers directory (src/archutils/Win32/ddk)
2) Update files including Win SDK headers to instead use the version installed by Visual Studio Installer
3) Update INSTALL.md
Note: this commit does not enforce using a specific version of the Windows SDK, but does everything needed to use the locally installed Windows SDK instead of files distributed with the source code to enforce using a particular version.
Users attempting to build the beta branch with GCC 12, 13 or 14 have a failure here due to the std::uint_8 data type.
The reason I changed it to an unsigned instead of including <cstdint> is because behavior on versions of GCC 12 and up were not consistent. I changed it to an unsigned because all versions of GCC were happy with that.
bApproximate is left over from long ago when the method to retrieve the system time might be 32-bit only or need correction. This is no longer needed, and already commented out of several sections of the sm5.1 base code. It can't be removed all at once just by deleting every instance of bApproximate, because it often refers to pointers that go elsewhere. so, this is the first part of a multi-stage removal process.
Other changes: remove samplerate() macro from RageSound, change some implicit casts to explicit.
Implements a static boolean to track the CheckGameLoopTimerSkips preference, so that it isn't calling PREFSMAN via CheckGameLoopTimerSkips in a tight loop. Since this setting is rarely used, and isn't expected to be changed while the game is open, it doesn't need to be continuously checked.
Reduce the input device check from every 500 frames to every 255 frames by way of replacing the modulo 500 with a wrapping uint8_t. A bit faster reaction during device plug-in is nice, and using a simple pre-increment instead of doing a modulo is a little more efficient.
This commit removes old translations from Translations.xml which are probably unneeded and unwanted, but also improves commentary so people will be better equipped to write their own translations.
Thanks @bkirz for pointing this out 👍
Attempt to fix the failing CI for the short term.
The existing workflow file still works on my personal fork, but for some reason doesn't on the main project.
Both come out to the same amount of target latency (as does 32x256, etc) however RageSoundDriver.h defines samples_per_block as 512, so this aims to have chunksize_frames equal 512.
It also defines a constant to define the quantity of buffers so it's not hardcoded in various places and puts the variables in an anonymous namespace.
The warning inside the loop makes the log too messy and is too much of a performance hit on very low end hardware.
I'm changing it to remove the mid-loop logging and only return a logged warning on a failure.
gluGetString, gluBuild2DMipmaps, and gluErrorString are all functions from the GLU library, which is not part of OpenGL.
This lets the game run without libGLU1 (or whatever it's called on a given distro).
Update README.md
sukibaby