Month 3 - Week 1 - Preperations for Audio Bug

 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.

Month 2 - Week 4 - Working on Event Reporting in Linux

 This week has been rather extensive. There was a lot of work involved with not just this class, but my other class as well. I wanted to pour in more time to get this specific bug we'll be talking about fixed and working correctly, however, I was not able to devote as much time as I would've liked. This is primarily because I had to spend a lot of time in the other class working on final assignments. So the majority of my time was spent ensuring that those assignments were handled and that they were turned in on time. As a result, the progress on this bug which I will be discussing in particular is rather slow, but I was able to make enough progress to be able to sufficiently know how to fix it next week.

To give a brief description of the bug, I want to put the key focus on the type of bug that we're dealing with. So whenever an average developer creates a window using our library, they tend to want to keep track of certain events that occur with that window. If you move a window, resize it, or maximize, or minimize it, you preferably want an event to be reported so that way the developer can create certain functionalities that can occur upon any of these events happening. So something I wanted to focus on was trying to determine why this bug occurred. A brief description of the bug is rather simple, if the user maximizes the window and then minimizes the maximized window, no minimize event is reported. It sounds simple on the surface but is actually much harder once you look into the nitty-gritty of it all.

One of the main issues in particular revolves around how these events are determined. I would also like to note that this bug is only occurring on Linux under the X11 library. Whenever an event occurs, the underlying architecture will report over to GWindow to handle it. This carries a couple properties, the main one in particular being the actual property return value. There are a lot of values this variable can be, but there are a few in particular that point to whatever is happening to the window. For instance, if the property value were equal to 384, then we can determine whatever happened to the window resulted in the window's property going to the max vertical height, this would normally mean a resize, maximize, or minimize event.

The first step that needed to be taken was trying to determine why the event wasn't being detected. There were a couple theories that I had, mostly revolving around the conditional logic of how the minimization event was being detected. I eventually concluded that none of the actual conditional logic was the problem. From my findings, it was a preliminary check that was acting up. So the whole reason the minimization event wasn't happening was due to that. The specific preliminary check that I'm referencing is a check that tries to determine if the window has changed all. It checks if there's anything different about the window's property, and by looking at it, if there's no difference, it will break out and report nothing. This is rather problematic though, because a minimization event doesn't inherently change the window. Whoever initially wrote the condition for this if statement also thought the same thing, as there is an additional condition applied to it, ensuring that the window isn't being minimized. However, this is where we face the actual bug.

Whenever a window is maximized and then minimized, the property return value does not report a minimized event. It reports a burst of 4-5 events, all being 384, or 384 with the last being 491 (I may be wrong as it's been a few days since I viewed the exact number). For simplicity, 384 is a property vertical max event, and 491 is unknown. None of these events are reporting the proper property hidden event code. This causes a problem. Since the property return value is no longer returning property hidden, and the window doesn't change due to it only being minimized, this if statement above executes and exits the function without reporting a single event.

There are a couple of things we could do to try and check for a minimize event during a maximize. One of the main checks is seeing if the previous event that was reported is a maximize event. If the previous event was a maximize event, and the property return value is 384, then we could report that a minimize event has occurred. We could also flip this condition to check if the previous event was a minimize event, so that way if you bring the window back up after minimizing, it reports a maximize. This is where the main problem occurs though, because I've already done this code, the window, due to the fact it reports these events in a burst of 4-5 events, we face the issue of it reporting multiple minimize and maximize events. So that leads us to figure out how to lessen that.

To clarify, in its current state, we do have minimize and maximize events reporting properly. The issue we currently have is that it's reporting them multiple times. We only want these events to report once, if it's reporting these events repeatedly, it can cause issues for whatever functionality might be blinded to these events. Since this library is used mostly for game development, you don't want to burst of minimize and maximize events being reported, there could be functionality tied to that game that may break behavior if it's being spammed with events like this. So this is why we need to figure out how to lessen it to just one event being reported.

One of the possible fixes is to use a bitmask or an array that tracks the burst of events, specifically, the property return value. Let's frame it like this. If the window is minimized while it's also maximized, we'll receive the burst of events, around 4-5. If we capture these numbers, and then check if they're equal to 384 or 491, we can tick a bit in the bitmask. We can then check if the bitmask equals 0x11110 or 0x11111. The reason for the two checks is if we only get four events and not five, we will have the last bit be 0, otherwise, they'll all be 1.

This is merely pseudo code, but it does show the idea of how this could work. By doing these checks, and then using the bitmask as a way to track how many times these events are reported, we can use the bitmask as a condition for the minimize and maximize. It would also force the event to be reported only once. This bitmask wouldn't be used anywhere else in the code and would be tracked only for this specific event. However, a change like this will require discussion with Lari and will have to be approved as this introduces more variables, and could have possible side-effects that I may be overlooking. Currently, this is where I am for this bug, but I feel I'm getting closer to fixing it. I feel if I'm allowed another week to tinker with it, I'll be able to get a solution made.

Edit (2/5/2024): I'm going to add a small correction to this blog post as I felt it was needed to correctly explain the bitmask. I made a mistake in my explanation of it and failed to represent the data accurately, this is due to most of my experience with bit manipulation revolving around color. To better explain the actual values, they would need to be displayed in proper binary. So instead of 0x11110 and 0x11111, it would be `0001 1110` or `0001 1111`. The pseudo-code would also be different compared to what was originally shown.

This should hopefully show more accurately what the code is checking for. Here we are checking if the bitmask is equal to `0x0001E` or `0x0001F`, which are just 30 and 31, or `0001 1110` and `0001 1111` respectfully. So this code should actually be more accurate to what the final code would look like.

Month 2 - Week 3 - The GController Bug is Fixed

 Last week I talked about a bug I was facing involving GController. To summarize, if you have a controller already plugged in before the GController is initialized, GController fails to properly "see" the controller. This was a rather tedious issue at first, primarily due to the fact I didn't realize what kind of bug this was. It was only later during this week that I figured out the bug was around threading and event callbacks. This required two changes primarily. The first was the unit test needed alteration so that it could properly locate and trigger the bug consistently, and the second was the controller setup needed to happen manually before the event callback was called.

The unit test was rather easy, as all it needed was a simple addition to the code. This addition is to make the main thread sleep. If the theory is that the event callback works on a separate thread, then we can make our thread rest while the event callback thread does what it needs. So, if the unit test is being done correctly, it should start and initialize the GController, get the connected controller count through GController, wait for one second, and then do a manual count of controllers connected. What this should do is find an inaccuracy. If the user has a single controller connected, the count when GController is initially made should be one. If we wait for one second and then check again, the count from the manual check should also be one. However, with the bug present, what happens is the initial count is zero, and then after one second of waiting, the count in the manual check is one. Since both counts don't match, there is a discrepancy, resulting in the test failing. This means the test located the bug happening, and that there is a bug present.

The second change was easier, as all I needed to do, was take the event callback lambda that was being used and store it inside of an auto variable. This variable will effectively become a function built solely for this initialized function inside of GController. We can then pass this variable to the daemon to make the event callback, and then call the lambda function itself after. This should make it so that when GController is initialized, it will immediately have the information it needs to properly work with the controllers as intended.

From this, I waited for the next meeting and then started discussions with Lari about this bug fix. We eventually concluded that the bug was fixed through his testing, and we discovered something else that was also interesting. Lari mentioned to me that there was another bug with manual unit tests for GController. What would happen is that GController would initialize in one of the manual tests, and if the test were to fail and move to the next manual test, the next one would fail outright, refusing to see any input mapping on the controller, resulting in missed inputs. When he went to show me this bug in action, however, he couldn't recreate the issue. I started diagnosing the problem and reading through the code but came to the conclusion that the bug I had just fixed was also responsible for the bug he was experiencing with the manual tests.

My theory is that the previous test wasn't breaking the next test, but more so, the test that was failing was due to the event callback not happening in time, thus the controller is not seen nor initialized. This would result in the input mapping not being configured correctly, and thus, when the test is reading or looking for input, the controller isn't registered, making the input "miss" even if you press the right button. So, indirectly, I ended up fixing two bugs at the same time without knowing it. I'm happy I did, however, because the more stable the code is for consumers and developers, the better.

Now, this would normally be the end of the post, however, I need to also talk about my next bug. There is a bug right now that involves the infamously hard-to-work-with X11 framework in Linux.

This bug is specifically around event reporting, or lack thereof with X11. The way this bug happens is simple. Take a GWindow, maximize it, and then minimize it. If you do this, you will have no minimize event reported due to the window being maximized. This is mainly due to conditional issues in the code. I've already located the problematic source in particular and will be working on it soon. The only reason I haven't started yet is due to the setup and steps I'm taking to properly get everything working correctly.

I decided that to tackle this bug, I would install Arch Linux on my system, and get it configured for development, so that way, I can fix the bug on my main workstation. The conditional check in specific is already located and marked as a comment on the merge request.

So since this bug has already been located and marked, I just have to prepare my workstation to resolve this bug, which I'm thankfully almost done with.

So far, I'm using an Arch Linux manual installation with Fish as my shell, I have KDE Plasma running under Xorg as my main desktop GUI experience. I'm also prepping some specific IDE-related things to get all of the code work ready. I should be finished with all of the configuring tomorrow, I just have some things with Vulkan that I need to take care of, and then I will be good to go. This bug will be my primary focus next week.

Month 2 - Week 2 - GController Bug Investigation

 Last week I spent time correcting and fixing a bug relating to macro usage and the context of what the macros actually represented. I'm happy to say this bug has been fixed and is now merged. Stepping further into GController, however, there are other bugs present that I decided to investigate. Particularly, there is one I noticed that piqued my interest.

I read through this issue and immediately wanted to diagnose it. At hand, the issue is rather simple. If you have a controller already plugged in before the GController is initialized, GController fails to properly "see" it, resulting in inaccurately reported information. For instance, if you were to call the function `GetNumConnected` which returns the total connected controllers, it would return zero instead of one if you had a single controller plugged in before GController is initialized. I decided to make a little unit test that would check for this inaccuracy, and then determine how GController would react. GController initially reported the unit test passing, which didn't sound right. I updated the unit test a little more, this time to provide printouts for the reported number of controllers compared against a manually checked reported number. To my surprise, they both were reporting zero, meaning that GController didn't even register that the controller was there at all.

This misreported number is unsettling, especially because the manual check is supposed to catch the inaccuracy and fail the test. After around two hours of researching, I reran the test, and amazingly, it failed. It worked as expected. The controllers connected reported zero, however, the confirmed controllers connected reported one. So, off the unit test, we have a bug that may or may not show at all. I decided to dig into the code at this point and debug.

From the debugging, I came across code internally that seems to be the problem:

In the initialize function, we see an event callback that is set up for XInput. In this event callback, we can start to piece together how the function is working. We call onto XInput to get the state of each controller slot, if the controller is connected, it will then work on setting up the controller, mapping what's needed, and then revealing it's connected. Something of note is that this code is bug-free. It works as expected, and also detects my controller fine. The problem is it is an event callback. The reason this becomes an issue is simple.

What if, by chance, you initialize GController and test for a controller before the event callback is called? What happens is the bug we're experiencing. This callback is being handled on a separate thread (if my information is correct). So what happens is the code we call and execute is happening too fast, and due to that, XInput hasn't initialized the controllers yet, which results in GController showing that no controller is plugged in. This means one of two things. We either need to initialize the controllers manually before XInput does, or we delay the program long enough for XInput to initialize itself. There is possibly another way, which is initializing XInput, and then calling the callback itself after (this assumes the back-end is created for XInput to allow this), but currently, these are the options left to choose from.

There is no resolution for this issue currently as I'm still working on getting it completed and fixed. From what's inspected though, I have a pretty good idea of what's happening now. So all that is needed is to test more, make changes, and see if this can be corrected.

Month 2 - Week 1 - Fixing GController's Incorrect Macro Use

 This week consisted of a lot of work from the previous month. Some of it included more MSAA testing, finalizing changes, and last touches, but there were some new things. One in particular was a bug I had assigned myself to last month and haven't had the time to fix, at least until now. With MSAA implemented, I took a look at it again and decided I would work on getting it fully fixed this week. This bug is directly related to the GController, specifically a macro called `G_MAX_CONTROLLER_INDEX`. The problem with this macro was that it was being incorrectly used. In some cases, it was treated as the max count of controllers, but in others, it was treated as the max index for the controllers array. The name of the macro itself can cause confusion upon seeing how it's used in code, and I figured it would be a perfect bug to work on right after finishing my first major feature.

We can see in the issue that it had been sitting for over a year. Admittedly, this issue is rather small, and not a major problem. However, it has been a source of confusion. Due to the name it carries, users of the library could end up mistaking it as the max index for the array, when it's more a max count of controllers allowed. To approach the issue, I waited until the Wednesday meeting where I could talk with Lari, my mentor, and Colby, the discoverer and creator of the issue above. We discussed how the changes would have to be handled, what ways it could be fixed, and the possible problems that fall into editing GController itself.

The route we chose for this fix was a rename. Simple enough. However, we needed to correct parts of the code that were using the macro as a max index. The rename would also modify three macros: `G_MAX_CONTROLLER_INDEX`, `G_MAX_XBOX_CONTROLLER_INDEX`, and `G_MAX_XBOX_CONTROLLER_INDEX_XBOXONE`. For each of these macros, we will rename them so that `INDEX` is replaced with `COUNT`. We have to update these changes throughout the source code and ensure that they are being used properly. There are some problems with the changes being made, however.

Firstly, testing any changes to the code is going to be a process. Since I only have one controller, an Xbox One controller, I can't test the functionality of what's being edited. This testing has to be done by Lari, as he has four controllers that can be used. Secondly, we have to modify some logic in the code, primarily the array bound checking that is being done for `GetState` and `IsConnected`. These checks are currently using the old macros, and they're also using them incorrectly, checking one over the array length (due to zero-based indexing). Lastly, we have to ensure that it runs cross-platform. We can't do this through traditional means as we don't have the proper tooling for it. Our only workaround for this is the runners for GitLab, but they can't test physical controllers, so only the basics. This means every edit we do has to be thoroughly thought out and looked over before they are pushed. This is to ensure we don't indirectly introduce a bug to the codebase, only to repeat the process all over again.

So with the possible issues known, we approached multiple ways to fix them. One has already been mentioned, that being Lari testing the primary manual tests with controllers for Windows. This will cross out Windows and UWP from the list of platforms needing testing. Mac and Linux will have to be handled differently. For those platforms, we will focus more on ensuring code changes are light and efficient. The changes done for them have to be reviewed and have to be ensured to have no logical errors in them. It's not the best fix, but it's the best we have currently. Then, naturally, there are the runners, that will make sure the code compiles okay, and that nothing will slip through for a majority of other tests.

Another fix is the logic changes. I decided that after renaming the macros, I would introduce three new macros that have the old name, however, these macros will equal the max count by subtracting one for zero-based indexing. This means that max count exists for physical controller count, and max index is for array indexing. They are separated. Because of this, I can replace instances of index checking in the Win32, UWP, Mac, and Linux code with the new index macro. This will keep readability while also showing how both macros are intended to be used. 

After these changes are done, we can reflect them in the actual code itself.

Then we also update the needed logic checks to better fit the new index macros.

You can probably tell that since the index macros contain the same name as the old count macros, we don't have to replace the macro name, more the logic itself so it is more in line with expected functionality. If the controller index is equal to the max index macro now, it is on the last controller in the array, compared to before where it needs to be greater than or equal to the macro. There was also worry with the old macros since the index checks could be checking one over the array's size, possibly causing overflowing in the library code itself, which is no good. The replacements and logic changes here feel more grounded, and ensure that everything is being used for proper purpose.

With all of the changes done, the main thing to do now is await feedback, have manual testing done, and ensure that all of the code is correct by code reviewing. Once these are handled, then the bug fix can be merged, and I can move on to the next bug on the list. The general workflow for this was very easy, and I didn't find it all too hard to get into. After implementing my first feature, I understand much more about how this library functions, what each class does, and how the code is compiled and formed. After getting a grasp of everything needed, I find the bug fixing to be more of a cooldown, especially in comparison to the nearly 2,600 lines of code I added to tests alone for the MSAA implementation.