This week has been a busy one, possibly more so than even when I implemented MSAA for DX11. There was work that needed to be done for issues unrelated to the bug I had picked for this week and even more work that was related to the bug I picked. So, this post will focus on the issues encountered, and the steps I took towards preparing for next week when I begin tackling this bug in its full entirety.
So, let's start from the beginning. The bug. The bug is an audio bug that has been around for a while now on Linux. This bug was originally not a bug, but rather refactoring that was needed for loading WAV files. It became a bug I assume after most of the refactoring had been done. There are issues with how the audio is played back to the user, specifically a described "popping" sound that is rather easy to catch in the tests. My description of the bug refers to the sounds more as popping, scrambled, or garbled. My assumption from the very start is possible garbage data that's inside the buffer causing the messy sounds that I hear. This could also be the case since this happens with shorter audio clips that aren't filling the buffer completely, resulting in some empty data at the end. If that empty space isn't handled properly, it could result in that data still being played, which results in the sounds I hear.
So, I picked a new bug to tackle and one that seemed like it would be pretty fun to tackle, but then I was hit with a wonderfully surprising issue the following day. The library wasn't compiling.
It seems that some change slipped past into the release code, preventing the entire project from compiling. This isn't good, this is a rather large issue to have because it can heavily affect the stability of the library if something like this happens. So, I went to work on diagnosing it. The errors really piqued my interest, however. I haven't done anything with strings that would be considered an error to the compiler, and generally ensure that my usage of strings and c-strings fit the C++ Standard. However, I did remember some warnings I had gotten last month when I first moved over to Linux that I took note of. There were four of these warnings, all warning of non-standard usage of strings.
These wonderful little warnings are not ones you generally want to see in code. Generally, if the compiler warns about ISO C++, there can be errors later. I also had similar warnings with the MSVC compilers as well. So I took note of these issues and moved on.
At least until now. Looking at the errors being printed, I immediately thought back to these warnings and knew that they were the cause. There were four of the warnings I took note of being in the release code, specifically in GFile. I didn't waste any time going through, making a branch, and fixing these troublesome errors.
These little things are what caused the entire library to buckle. The moment I made the variables const for both Linux and Windows, the warnings went away, and the library started compiling just fine. Simple fix thankfully. But this left us needing to figure out how to prevent this issue in the future. I wanted a stern fix, something that would permanently prevent this from happening again. I dug around and eventually found this little argument, "/Zc:strictStrings". What this does is tell MSVC to conform to the ISO C++ Standard and outright fail to compile if it finds poor string usage. So instead of it being a warning about it, it will treat it as an error. A lot of the "Zc" commands are passed into blank default project templates like Console applications, which is why it failed to compile initially. So, I went through and added this flag to the list of debug and release flags passed for the library. Now, if you develop for the library, it will outright fail if you do poor string usage. But I wanted to go further because we support developing on Linux and Mac. So I also introduced flags for them as well.
I do want to note, that currently, this merge request for these changes hasn't been merged and is still in active development. This is because another merge request needs to be combined first, and because I need to ensure the Mac flag is correct before merging completely, as Mac uses a Clang-based compiler for its C/C++ compiling.
After I made these changes, I had a sudden want to implement a couple other additions, mainly due to the bug that I will be working on. I wanted some changes to the library to make bug fixing a little easier. One of these changes is the introduction of general tags to unit tests.
This was easily one of the heftiest changes I made. It was the first outright change to code I've done without being asked and involved a lot of testing to ensure systems were working right. The code edits themselves were easy, but I felt I needed to "sell" my changes, since on the surface, they can look pretty generic or even unneeded.
What does this merge request actually do? What it does is allow for IDEs that are capable to test on tests by a specific tag. What this means is if you have a suite of tests that have an "Audio" tag, and you have dozens of others with different general tags, you can target only the tests with the "Audio" tag. This makes it so that you don't have to test all 2,349 tests, but just 40 instead. This is extremely useful for me, as I'm going to be testing audio a lot, and this change will save me having to wait for a minute each time to test audio. It'll instead cost me only 10-20 seconds. Not too bad.
After doing this change and sitting a little more on the project, I decided to tackle another issue. There was a problem with CMake on Linux. There are a few problems with it admittedly. 1) CMake fails to work with the build directory if that directory is not populated with at least one file, which makes it extremely frustrating if you want to remove the build directory for a completely clean build. 2) CMake lacked support for multi-config generators, meaning that if you tried using something like Ninja Multi-Config, it outright wouldn't work. Lastly, 3) there are a few single configuration generators (namely, Ninja) that won't work with CMake at all. I started trying to diagnose this issue to figure out what exactly was causing the problem. I ended up finding the issue pretty quickly.
This code here is the entire configuration for Linux in the main CMakeLists file. Besides some of the hard-to-read formatting, there was something I noticed very quickly.
The CMakeLists file required that these two directories exist. That's not good. Let me explain. Multi-configuration generators are generators that allow for multiple configurations within the same build. What this means is instead of having your build separated into a debug and release folder like with single configuration generators, you can instead have them stored inside of a main directory that will additionally store its own debug and release folder. It saves you from having to maintain multiple directories and also makes the general process of having multiple profiles for a single project much easier.
What happens is if I try to use a multi-configuration generator and point it to a build directory of "build/Desktop", it will fail to even make the project because it is expecting a specific layout for the build directory. So, I set to work on this problem.
The very first thing I chose to do for this was to let CMake manage not only the build directory but also put an empty file inside of it. If we don't make an empty file, we open the door to CMake not understanding the directory, failing to find libraries, and even its own make program. I'm not sure what it's like this, but we do these steps to prevent it. Lastly, we make the project itself. I'm not sure why in the old code the projects were made with different names, but it is generally unneeded, even with single configuration generators.
The next part is important, this is what will help configure multi-configuration generators. What this is doing is checking if the variable "CMAKE_CONFIGURATION_TYPES" is defined. This variable is only ever defined if you are currently using a multi-config setup. So, if it is, then we will set the types to the ones we want (Debug and Release). We also have a message that will print providing information about what generator you are using. This is more debug, but also useful in general as it tells you what step CMake is in currently for the CMakeLists.
Lastly, the biggest change. You may have noticed in the old code that each of these build configurations had their separate flags for the compiler.
Now, if you have a keen eye, you can remove a majority of these arguments as they are mostly the same with each configuration. The only difference between the two was "-g" being included for the debug configuration. Now, in the change I made, I provided a rather long comment. I'm going to quote this comment verbatim.
There is no configuration needed for debug and release as CMake will already handle this for us through the CMAKE_CXX_FLAGS_DEBUG and CMAKE_CXX_FLAGS_RELEASE variables. On multi-configuration build systems, CMake will automatically use CMAKE_CXX_FLAGS_DEBUG_INIT and CMAKE_CXX_FLAGS_RELEASE_INIT to set the flags for the respective configurations. The debug will use the commands `-g` and the release will use `-O3 -DNDEBUG`.
You may ask, how can I be sure of this? Well, I debugged the CMakeLists file and noticed the variable, I got curious and decided to test my theory with multiple generators, and found it to be true. Here is a snippet of a debug configuration I have using Ninja Multi-Config.
This is Ninja's form of a makefile that you are viewing. I took the effort to parse this and highlight the main part for your viewing. You'll notice in the list of flags I highlighted, you can see "-g" is included, even though I don't explicitly add it in CMake. Let's look at the release configuration from Ninja.
If you look here, you'll notice there is a difference. Instead of "-g", we get "-O3 -DNDEBUG", this is rather important, because it means that the explicit inclusion of this in the CMakeLists file was unneeded, CMake does this for us already. Good to know.
A lot of the other changes for this merge request included removing an error message that was not important due to it only reporting a library not being used (this library was "threads" which was excluded from being included in the project at one point in time). There was also a variable that was set for CodeLite, which I took the time to wrap in an if statement so it would only be set when you're using the CodeLite generator.
In total, this was a week that resulted in over 3 merge requests being made on top of the merge request for the bug that I'll be working on next week. I want to get all three merged first if possible so that way I can build, configure, and test the fix for this bug as comfortably as possible. For now, this is all the work that I've done this week.
This isn't a traditional format for how I do these blog posts, but due to the volume of changes I did, I felt all needed to be covered as they were each just as important to me.
No comments:
Post a Comment