Braid Code Cleanup (part 2)
Here’s what I have done on Braid since the previous blog posting. It’s about a day’s worth of work, maybe a little more (it consists of a few sessions spread out over the weekend). In case you ever wanted to know what it’s like being an indie game developer … …
Found and removed about 3kLOC more of unused source files. New source count: 78,127 lines.
Fixed header file mess (all headers are appropriately registered in the Visual Studio IDE despite the fact that they make that about as hard as possible).
Put the Visual Studio 2015 redistributable in the file tree and set the Steam installscript to use it. (Steam itself has a method of installing redistributables via server configs, but if I do it this way I at least know I am installing the same thing on Steam, GoG, Humble, wherever else.)
Visual Studio 2015 / Windows XP Compatibility: Verified that Platform Toolset is set to v140_xp for all project files. figure out whatever other options I need to turn off to disable telemetry, prevent weird undesired dependencies).
Visual Studio 2015 / Add notelemetry.obj to link line to reduce Microsoft adware potential.
Make Launcher compile in VS2015: Delete ‘fl_call_main.obj’ from precompiled dependencies, add fl_call_main.c to source.
Update Steam scripts so the game packages and uploads to a new test depot.
Set up the Steam app configuration so that Steam knows how to start the game.
Due to Visual Studio bugs I am not currently able to run the static code analyzer. (It is not supported on the v140_xp ‘Platform Toolset’ [whatever that is], and the IDE hits a bug whenever I try to change the ‘Platform Toolset’. So if I ever want anything besides v140_xp I am going to be in a tough spot!)
Use C++11 initializers for all 48 entity types, and remove unneeded methods. Total savings: 658 LOC. (This is just on entities; have not done it to the rest of the code yet!)
Get rid of goofy copying of gamelib_core/gamelib_render’s headers and libs.
Delete a few more unused files from gamelib_core. Remove os_net_* from gamelib_core/win32. Savings: 331 LOC.
Use C++11 initializers in gamelib_core, gamelib_render. Effect of this is intermittent based on the type of code; some just isn’t that way. Find + remove some more routines that aren’t used. Savings: 161 LOC. Current line count: 76,972.
Refactor pieced_image_maker tool so it now compiles without requiring gamelib_render.
Fight with the compiler for a while, trying to get Debug builds working post-VS2015 upgrade. “Fix” this by giving up on the idea of linking against debug versions of 3rd party libraries. Deal with confusing brokenness of VS’s “precompiled header” implementation, which has been terrible for decades yet never fixed.
Remove debug Windows libs from the 3rd party library folder.
Call SetProcessDPIAware on startup to avoid Windows’ pixel scaling.
Start stripping out the YCbCr texture formats; still loading jpgs, just into classic ARGB textures now. At some point I see a crash has been introduced (having to do with the parallel universe effect) that I need to debug. Run Debug build. Wait, why can’t I jump or go through doors or skip levels in Debug build?? Oh, I hadn’t properly saved Event_Manager.h after switching it to use C++11 initializers (but after taking them out of the body) so now things were uninitialized. Figure out how to list unsaved buffers in emacs so I can see if I did this anywhere else. (I didn’t, seemingly).
Realize that an Update 3 for VS2015 was released a couple of weeks ago. Download and install. It won’t install unless I update Windows and reboot. Update Windows and reboot. Install the update – it takes a really long time for some reason (this is on a very fast laptop with SSD – Razer Blade 14” (2015 model)). After about 20 minutes, realize I’m not going to be able to work any time soon, stick laptop in car and start driving across the bridge back to San Francisco, hope it finishes before battery dies. One hour and 5 minutes after start of install, battery is about to die, but progress bar for install is not done (but looks close to done) so I sleep the laptop, but later when I get to the cafe, it seems to have rebooted for some reason. A new install dialog has popped up but the “Update” button is grayed out. I guess I have a mostly-upgraded copy of Visual Studio now. Shrug. It seems to compile Braid okay.
Finally, go back to trying to debug whatever error I introduced to the World 5 parallel universe effect. It is crashing in calling a method on D3DXEffect, but the pointer looks valid, and of course Microsoft makes everything inside the pointer a completely undebuggable black box because it is a COM object instead of a regular C struct. The number of hours of my life wasted by this would be rage-inducing if I stopped to think about it. The “DirectX Control Panel” used to help you debug this stuff, but now for DirectX9 everything is grayed out and I can do nothing there. Verify that shader is compiling just fine and that everything works as long as both the ‘regular’ shader and parallel universe shader are set to be the same (regardless of which is chosen) but that the problem happens when they are different. Game is 100% reproducibly crashing in ID3DXEffect::Begin whenever I switch from one effect to the other mid-frame.
It seems I am not allowed to call certain D3DX things in certain orders, but for some reason this worked before. Juggle around the order a bit.
While running in Debug I got a new crash “Invalid address supplied to RtlValidateHeap” when cleaning up the diffed entities in World 5 – had better look into that later!
D3DX still seems touchy whereas before it was not. I must have disturbed some delicate balance but since this is no longer supported by Microsoft it is totally undebuggable now. I guess “get the hell off D3DX” is now on the to-do list, but in the meantime I am going to revert back to working code from this morning and re-do everything in smaller steps until I figure out what happened or else just mysteriously succeed this time. (I’ve already wasted 90 minutes - 2 hours on this, reverting will take less time).
Revert. Re-do the other fixes I did today not having to do with CbCr. Hmm, as soon as I disable the #define SEAN_YCBCR, the crash happens, I just didn’t notice before because I hadn’t been on World 5. This should help narrow it down.
It takes only 10 minutes to figure out the bug this way, as opposed to the 2 hours I spent of “legitimate” debugging of the horror that is D3DX in 2016. The problem was here:
inline ID3DXEffect *get_effect() {
#ifdef SEAN_YCBCR
Display_System_D3D *sys = (Display_System_D3D *)gamelib_render->winsys;
return sys->fx_shader_effect;
#else
Fx_Shader *shader = get_shader();
if (shader) return shader->effect;
return NULL;
#endif
}
And simply was that this should say #ifdef SEAN, instead of #ifdef SEAN_YCBCR. See, back when Sean worked on Braid, he did basically two layers of stuff: one was a general set of postprocessing effects to make the game look cooler when you rewind; the other was the YCbCr texture handling. The effects came first and were all #ifdeffed SEAN, and the texture thing came later and was #ifdeffed SEAN_YCBCR. Since if you turn off YCBCR but not SEAN, it only crashes in World 5, it is not surprising that neither he nor I ever noticed. (Normal operation of the game is to have both of these turned on). This use of sys->fx_shader_effect should always be happening if SEAN is turned on, which should be always. But I just turned off SEAN_YCBCR, and, well, it is bad news. This was hard to spot because in the process of stripping out YCbCr stuff I had simplified this routine to:
inline ID3DXEffect *get_effect() {
Fx_Shader *shader = get_shader();
if (shader) return shader->effect;
return NULL;
}
Which is what it would have been before any SEAN changes, but we want the SEAN changes. Ugh. Well, now I un-revert all my reversions, fix the #ifdef and move on.
Update from FreeType-2.3.5 to FreeType-2.6.1 (current FreeType seems to be 2.6.5, but hey, I already have 2.6.1 compiled because that is what we shipped in The Witness, so we’ll go with that for now).
Tweak the level design on that one level to move that one spot up and to the right. (Finally a little bit of actual work!)
Tried to run Static Analysis again, figured maybe it would work after the update, but I still can’t change the ‘Platform Toolset’ yet; here is what happens every time I try:
Here’s looking forward to the joy of installing some forthcoming Update 4.
Oh, I should finish taking out remaining CbCr code (in the OpenGL build of the game, which is used for Linux + Mac, and which I am not testing now, but might as well clean it up and take a line count). Current line count is 76,544. Looking up the file, last night’s was 76,972; that’s a savings of 428 lines. I am not sure why it’s not more, but maybe it just seemed like more than it is due to how pervasive the CbCr handling is. (Also, we are not counting LOC in shaders, and we probably saved at least 100 LOC there).
Update stb_image.h to v2.12 (old version was 1.13 and was not a single-file header!) This appears to have worked perfectly and only took a minute, proving once again that single-file headers are the way to build and distribute libraries in C++.
Line count went up because I guess I have been counting stb_image this whole time (it is embedded in the code, not tucked into a faraway place like bigger libraries). I guess I will keep counting it. Current count is 78,192.
Test a while to see if I can repro that RtlValidateHeap thing. It seems to always be happening when deleting a temporary copy of one particular Mimic on one level (the Mimics are the rabbit guys). It’s happening because I took out a hack in the old code where I was setting a Mimic’s texture name to NULL after freeing it in the destructor – which is nonsensical – but it was preventing this bug, so I guess I am double-deleting the entity or just double-freeing the texture_name somehow (ugh!) and somewhat irresponsibly I did not leave a comment about what I was doing before and how bad it was (double ugh!):
Entity_Mimic::~Entity_Mimic() {
gamelib_free_string(texture_name);
texture_name = NULL;
}
Because of ASLR this is now really hard to debug because I don’t have a way to uniquely identify this guy (in a world with no ASLR I can just set a breakpoint when we allocate that particular guy). If I had a robust time-travelling debugger I could go backward from the double-free to figure out how it happened … but I don’t.
If it’s a double-free, then this assert should fire, but mysteriously, it doesn’t:
Entity_Mimic::~Entity_Mimic() {
assert(texture_name != NULL);
gamelib_free_string(texture_name);
texture_name = NULL;
}
I guess I am going to just log every deleted Mimic and see what I can reconstruct from that. Wait, when I do that, there’s only one. AHA, the bug is that texture_name was actually a field of Entity, not Entity_Mimic, but Entity_Mimic’s destructor was freeing it, and of course it runs before Entity’s destructor. So the code with ‘texture_name = NULL’ was technically valid, but really not good. Fixed.
Next appealing item on my list is simpifying Bitmap_Loader as mentioned in the previous posting. Looking through other code … it looks like I already did this for The Witness in October 2009 (the time I was beginning to get 100% serious on the project!) Copying that code over to Braid. Add ‘defer’ implementation to gamelib_core.h, switch weird Witness macros for deferred freeing to better general ‘defer’ implementation. There is no longer a ‘Bitmap_Loader’ class, we just load stuff via flat functions, so change all the calls made by the game to the new format.
Oh dang, the old Bitmap_Loader had weird stuff for dealing with the loading-jpeg-alpha-layers-into-rgb-textures-all-in-one situation. Even though I am going to get rid of that later (probably some other day), it’s prudent to paste it back in for now and verify everything works. Oh, and yuck, this is snarled up with the async_jpeg stuff, obviously. Red alert, this is going to be nasty. We are going about this in the wrong order. Proper order is to clear out all the async jpeg stuff in game code, ensure the game works, then pull over the Witness bitmap loading code. Revert!!
Need to load all the jpeg files and their corresponding alpha files, compress them into single DXT5 textures, and save. Installing nvidia texture tools both old and new. I don’t see any obvious way to load multiple source files into different channels with either of these. Maybe there is a way, or maybe I could figure out some tool to download on the internet and learn how it works, or maybe I can break out Photoshop and try to do something with action recording (may be difficult because of the filename correspondences, I don’t really know how to do that, it would take some research) … but absurdly the simplest/easiest path will be for me to just write C++ code to do the put-together – say, I can load the jpegs, save a 4-channel bmp for each jpeg rgb+alpha pair, and then run the NV texture tools on the BMPs.
Whoops, I reverted the defer implementation along with the Bitmap_Loader stuff, let’s copy that back from the compiler project.
So I bang out the following totally lovely one-time-use code, right into Braid’s main.cpp:
#include "bitmap_loader.h"
#include "bitmap_data.h"
#include "bmp_save.h"
void do_bmp_craziness() {
Array <char *> filenames;
Array <int> sizes;
bool success = os_get_filenames_and_sizes("data/pieces", &filenames, &sizes);
assert(success);
Array <char *> rejects;
for (int i = 0; i < filenames.items; i++) {
auto name = filenames[i]; // 'name' has no directory name info; it is just the filename.
defer { gamelib_free_string(name); };
auto s = strstr(name, ".jpg"); // Should be strrstr but Visual Studio doesn't have it; doesn't matter for this, eh.
if (!s) continue;
auto delta = s - name;
assert(delta < strlen(name));
auto name_copy = gamelib_copy_string(name);
defer { gamelib_free_string(name_copy); };
name_copy[delta] = '\0';
auto rgb_name = mprintf("data/pieces/%s.jpg", name_copy);
defer { gamelib_free_string(rgb_name); };
auto alpha_name = mprintf("data/pieces/alpha/%s.jpg", name_copy);
defer { gamelib_free_string(alpha_name); };
Bitmap_Data result;
auto success = gamelib_core->bitmap_loader->load_bitmap(rgb_name, &result, alpha_name);
assert(success);
auto result_name = mprintf("data/pieces/output/%s.bmp", name_copy);
defer { gamelib_free_string(result_name); };
auto f = fopen(result_name, "wb");
assert(f);
defer { fclose(f); };
if (result.format != TEXTURE_FORMAT_ARGB8888) {
rejects.add(gamelib_copy_string(name));
} else {
success = save_bmp_file(f, result.data, result.width, result.height, 32);
assert(success);
}
}
// Break here to examine rejects.
int k = 1;
}
So that didn’t take long (after fixing a couple of small bugs) and I even found several files that shipped with the game but should not have (they were redundant bitmaps that didn’t have corresponding alpha images, because they had been generated in earlier iterations of running the image-packer tool).
Now I have a folder full of BMPs:
And it should be pretty straightforward to use nvdxt (an old program but I know how it works) to convert them to dxt:
nvdxt -dxt5 -file *.bmp
(nvdxt is a bit slow even on today’s computers so I go brush my teeth but not before eating half a Lake Champlain Five Star Peanut Bar oh wait I mean three quarters).
I come back and the .dds files are done.
I comment out the beautiful one-time-use code but leave it in the file just in case.
I move the jpegs to the side, move the .dds files over to where the jpegs were, and the game should just load them fine, fingers crossed, moment of truth …
Nuts. Well, it loaded them fine. My theory is that they are flipped. nvdxt has a -flip option and I try that quickly just on World 5 images just to see if it works:
nvdxt -dxt5 -flip -file five*.bmp
That looks better! It’s still messed up, but some of the scene looks pretty okay, so it’s possible that everything messed up is just not world-5-specific assets. Let’s flip everything.
Looking much better! However, there are still some obvious problems. Glancing through the levels, I am not immediately sure what is going on. Will have to A/B this versus older Braid tomorrow sometime to figure out the problem (possibly a file converted incorrectly, possibly I am forgetting some files somewhere, etc!) In the meantime this is a good place to stop for tonight.
I should point out that these particular dds files that I just generated are temporary; once it is fully working, and the Bitmap_Loader code is simplified, I’ll go back to the Braid archival files and grab images that have not been through jpeg compression and back (provided I saved the post-packed images and don’t have to regenerate them from Photoshop PSDs, which could get hairy for totally other reasons.)
- Previous Post:
Braid Code Cleanup (part 1)
- Next Post:
Braid Code Cleanup (part 3)