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.
No comments:
Post a Comment