In the previous posting I said it “was a good place to stop for the night”. And I had decided to, but as sometimes happens, I changed my mind and worked for about another 45 minutes until I had solved the problem.

I knew I had a problem in at least some DDS files. I checked the old version of Braid to narrow down one particular file causing a problem. Then I downloaded the DDS plugin for Photoshop, installed that, viewed the DDS file, and … there’s a big gray band across the bottom of every DDS image, just like the gray bands I am seeing in-game. At first I presumed this was a bug in nvdxt and looked into how to use nvcompress, but that doesn’t handle batch files. But then I just clicked on one of the BMP files I had outputted orignally, and saw the gray stripe there too:

Note that you can see these gray bands very clearly in the file listing from yesterday’s blog post!

After presuming this was a bug in my BMP saving code (which is going to be deleted in the next step of refactoring anyway, so I didn’t really want to debug it), I tried switching to stb_image_write to save BMP files, and PNG files, and neither of those really did what I want: BMP didn’t save the alpha channel, and put purple everywhere instead:

PNG came out all wacky, I don’t even know what is going on:

But hey, this was an old version of stb_image_write, I should upgrade that to a new version when I am back to a stable point. In the meantime I should just figure out a quick fix…

Then I got lucky and some asserts started going off because I was repeatedly re-running this image loading stuff in the debugger. And I realized … heyyyy, jpegs are probably loading asynchronously … and I am not waiting for them to be finished loading before I save them. Weird that this would insert the same-sized gray band every time, but let’s switch back to my BMP saving code and try waiting …

… and it worked perfectly.

Oops! In Braid there is no such thing as synchronously loading a jpeg, even though the API looks like there is. This is very error-prone which is one reason I want to get rid of it!

Come to think of it… I bet that gray color is 0xcdcdcd, the pattern that the Windows C Runtime fills uninitialized memory with! (Let’s verify this in Photoshop):

Well, it’s close enough for government work. I am not sure why it turned into 0xcecdce, but it is probably something to do with gamma correction and something something. I don’t really trust Photoshop to be precise about anything.

Anyway. I re-save all the BMPs, run nvdxt on them, copy the new DDS files over, and … huge success!

Whew. So, I am still typing this up “last night”. Time to go to bed. See you tomorrow.

Okay, it’s tomorrow.

Next task: eliminate all jpeg files from the game’s data so that I can remove jpeg support from the game completely. I’ll get it back later, with the new bitmap loading code, but in the meantime this is a good way to be sure all the threading shenanigans are really gone.

After removing the jpeg version of the in-game graphics, a search reveals there are 9 small jpegs that are used as icons in the editor. Rather than batch-converting in Photoshop, I just repurpose the bitmap conversion code from yesterday. Delete the jpegs. Open the editor to make sure it works, it does.

Do searches for .jpeg and .jfif (alternate .jpg extensions) but there aren’t any. Case closed for now.

Re-disable the bitmap conversion code in main.cpp.

Remove module_jpeg.cpp. Fix compile errors, modifying pieced_image_maker tool to save bmp instead of jpg. Remove all async_jpeg stuff from the code. (Leaving the concept of unloaded maps in for now, since we may add an asynchronous bitmap processing mechanism back later .. but it will be much better-structured than the jpeg thing was).

Test the game; it runs! Test in Release also. Still runs.

Now it’s time to remove the Bitmap_Loader and replace it with the simplified Witness version, as I was trying to do yesterday. Remove the old files, copy over the new files (there are many fewer new files than old files, due to simplified structure and ability to use stb_image to do more).

Upgrade stb_image_write from 0.92 to 1.02.

Go to the office, read and deal with Witness-related emails.

Change usage of stbi_write_png_to_mem (which seems to be deprecated) to stbi_write_png_to_func. Oh wait, no, stbi_write_png_to_mem still exists, it is just not exposed in the header file, and using to_func would be slower. Okay, expose to_mem.

Change usage of load_bmp_info (get dimensions + number of channels without fully decoding bmp) to stbi_info_from_memory, which is also better since it handles all formats.

Game now compiles! Doesn’t run properly, though. Wait, it is running fine in Debug, just very slowly in Release and with letterboxing for some reason? Oh joy. Full Rebuild doesn’t help.

Oh look, I am reloading all texture maps from disk every frame. That would definitely make things slow. When loading bitmaps, flag them as definitely not unloaded. The way the whole ‘unloaded’ thing works will have to be audited later. For now we are happy.

By the metrics we’ve been using to count code, line count is now 81,086, but a lot of that is enlarged stb_image stuff. Let’s move those out to an external folder so it’s easier not to count them. Also delete a very old stb_image.c that was laying around. New count: 71,654 lines.

But, I have not ever been counting the source for the pieced_image_maker tool or for the shaders. Whip up a hacky count script to do this:

cloc --no3 --by-file-by-lang --force-lang="C",fx gamelib_core gamelib_render  game tools game\run_tree\data\shaders

(I pretend .fx files are C because cloc doesn’t know about HLSL, or at least, this version of cloc doesn’t.)

More accurate line count: 80,017.

Oh, I am still linking to libjpeg and libpng and have the includes and libs hanging out in the external libraries folder. All this stuff is taken care of by stb_image now, so delete all that. (The API to libjpeg is pretty terrible). This stuff doesn’t affect line count since I don’t count the external dependencies. Verify game still compiles and runs.

While doing the new line counts I had noticed a spurious file puzzle_assembly_screen_sean.cpp that is not compiled into the game and is mostly a copy of puzzle_assembly_screen.cpp. It looks like it was some kind of safety backup I made back in 2008 when I wanted to integrate some of Sean’s changes more closely into the puzzle code. We don’t need this any more, and if we ever do, it’s in source control. Deleted. New line count: 78,830.

The game still has a copy of BMP saving code, that we can remove in favor of using stb_image; then the pieced_image_maker tool has another set of BMP saving code, this one written by Casey Muratori in 2002. Let’s get rid of both of these.

First, on the game side: Delete bmp_save.cpp, bmp_save.h, bmp_internal.h. Actually that was the last stuff in the entire gamelib_core/bitmaps folder, so let’s delete that folder and simplify our file structure, which is a nice win. Make it compile. Now I need to test writing a bmp… In developer mode, the Print Screen key writes out a screenshot, so I try that, and:

Well, it’s flipped vertically, and RGB are byte-reversed. The conventions in our engine are totally opposite of what stb_image expects. Maybe I can figure out how to get stb_image to do this efficiently (looking at the Witness code, we flip everything and byte-swap it when we load it, which isn’t as bad as it sounds since all texture assets are prebuilt for the local platform when running in production so we don’t do this work at runtime). I will ask Sean about this on Twitter, but in the meantime I just write some quick code to do the job:

bool save_bmp_file(const char *filename, byte *data, int width, int height, int bits_per_pixel) {
    assert(bits_per_pixel % 8 == 0);
    auto bytes_per_pixel = bits_per_pixel / 8;

    // Flip in place, swap byte order. @Cleanup: Can we do this in stb_image?

    for (int y = 0; y <= height/2; y++) {
        auto y_flipped = height - 1 - y;

        for (int x = 0; x < width; x ++) {
            byte *dest   = &data[(y_flipped * width + x) * bytes_per_pixel];
            byte *source = &data[(y         * width + x) * bytes_per_pixel];

            auto tmp0 = dest[0];
            auto tmp1 = dest[1];
            auto tmp2 = dest[2];

            dest[0] = source[2];
            dest[1] = source[1];
            dest[2] = source[0];

            source[0] = tmp2;
            source[1] = tmp1;
            source[2] = tmp0;
        }
    }

    int success = stbi_write_bmp(filename, width, height, bytes_per_pixel, data);
    return success ? true : false;
}

and it works:

Update from the next morning: Actually there’s a bug, as you can see if you actually look at the image carefully, which I didn’t: There’s a line of pixels across the middle that has been swapped twice, and is therefore the wrong color! Of course I had the sense yesterday that I should stop and think about the loop condition carefully, but I didn’t want to – I wanted to go fast. Changing it to

y < height / 2

will work for images with even heights, but what about odd heights? Rather than do some weird math that is harder for people to understand in the future, I think the simplest and most robust way is to add

if (y >= y_flipped) break;

to the loop body. Interlude over; now back to yesterday’s post.

So, one redundant copy of BMP code is gone. Check in. Line count: 78,583.

Ask Sean about this on Twitter.

Now to the pieced_image_maker tool. Delete pieced_image_maker/image/bmp.h, bmp.cpp, image.h, image.cpp. There is still PSD loading code in there but we’ll ignore that until later. Replace Chris Hecker’s old ‘image’ struct with the Bitmap_Data used everywhere else in the engine. (I had put together pieced_image_maker based on some code Chris sent me waaaaay back when.) Lots of errors because the interface was different. This is feeling like a relatively unrewarding conversion, especially because the old code was very self-contained so the mess of functionality duplication was limited, but let’s stick with it.

OKAY, after a bit of a slog, pieced_image_maker.cpp compiles, but the psd code does not. Hmm, I can’t substitute stb_image’s psd loading, because we actually are doing a process where we step through the layers and output a file for each layer, so we need to maintain our understanding of layer structure and stb_image does not do that. So instead I get to do more converting of image -> Bitmap_Data, in the psd code this time. Yaaaay. Oh wait actually that didn’t take too long. I’ll have to test this tool a bunch though, later on, but that will happen naturally.

Current count: 76,178 lines (savings: 2,408 lines). But wait, there is more we can do – because this originally came from someone else’s codebase, there is a bunch of code that I mostly or completely don’t need: utils.h, utils.cpp, math1d.h, math2d.h, math3d.h, math4d.h, image_operations.cpp, image_operations.h, checker_assert.cpp.

image_operations.* was being compiled in, but apparently was no longer used at all, so that’s gone now. utils and the math stuff was not even being compiled in! (utils.h was being included, but provided no use). So BOOM, all that stuff is gone, along with the ‘misc’ folder. And there’s still an ‘image’ folder, but it only contains psd.cpp,h, so let’s move that out and further simplify our folder structure.

OH WAIT, psd.cpp actually needed some stuff from math1d and math2d, but because of broken Visual Studio caching, I was still able to compile. Well, I need to bring some of it back, but not much, just defining some basic types, I’ll just put it into psd.h. At first glance it also wants stuff from math2d, but looking more closely, all it wants is the existence of a ‘vector_2’ that it can construct and assign, and I can just do a drop-in replacement with my ‘Vector2’.

It compiles again. Current source count: 73,612. Savings: another 2,366 lines! That’s a total of 4,714 lines removed, in the approximately two hours I have been banging on pieced_image_maker.cpp. The tool itself is down to 2,123 lines – much smaller than it was before, which is a really good thing for tools since those are the things most likely to break / rot by the time you need to figure them out years later.

Sean on Twitter says I have to reformat the data to use stb_image, just as I am doing now. So that’s fine.

Lunch time, break time!

Okay, next: “Get rid of Pool, use Isolated_Heap_Pool.” As mentioned in part 1, the only place using the old Pool is the font code. Thing is, I am going to replace the font code at some point later (the newer version used in The Witness uses Harfbuzz to do layout and supports more languages, and has some other modernizations). But this is a small enough change that it probably doesn’t count as wasted work. Font stuff is in gamelib_render, but Isolated_Heap_Pool is in game, which is a higher-level piece, so Font can’t see it. Move Isolated_Heap_Pool down to gamelib_core. Notice in the process that the non-Windows/Xbox ports of Isolated_Heap_Pool (not written by me) do not actually use separate heaps. Yuck. This is not too terrible if they only run on high-end platforms, but it’s not ideal. Comment this. There is an ‘isolated_heap_pool_std’ that is used on other OSs; we will come back to this because it is annoying. (This is one of many reasons why cleanup passes are good; you notice all kinds of things you wouldn’t have noticed otherwise.)

The switch seems to work the first time (Pool and Isolated_Heap_Pool had the same API, so aside from messing around in the Visual Studio project and moving files, it was just a 1-word change to one file). Game runs great, and it still draws fonts just fine.

Okay, let’s take a look at isolated_heap_pool_std.cpp,h. It is basically just a separate implementation of Isolated_Heap_Pool with some code changed so it doesn’t use HeapCreate / HeapDestroy. In fact it is basically Pool, just with totally different code because someone at Hothead Games wrote it when he was porting Braid to OS X + PS3 I guess, and I guess he didn’t see Pool because that already did exactly what he did. Well, … let’s just make Isolated_Heap_Pool not try to use separate heaps when #ifndef WIN32, which essentially downgrades it back to Pool. Then we can get rid of Isolated_Heap_Pool_Std.

Use C++11 initializers in Isolated_Heap_Pool.

Delete silly global variable

Isolated_Heap_Pool *the_isolated_heap_pool;

Why was that there? Nobody uses it, thankfully.

Count: 73,451 LOC. Savings from pool stuff: 161 lines. This feels like it should have been more, but looking at the line counts in scrollback, it roughly checks out. Well, it’s also 4 files gone, that is pretty good!

Next on the list; there’s a file called byte_reading.h that defines a bunch of stuff like this:

inline bool read_u4b(char **data, int *len, int *result) {
    assert(sizeof(int) >= 4);

    if (*len < 4) return false;

    unsigned char *base = (unsigned char *) * data;
    unsigned int sum = 0;

    sum += *(base + 0) <<  0;
    sum += *(base + 1) <<  8;
    sum += *(base + 2) << 16;
    sum += *(base + 3) << 24;

    *result = (int)sum;
    *data += 4;
    *len -= 4;

    return true;
}

(Reading a 4-byte value from a memory buffer into an integer using application-defined endianness; of course this can be accelerated on platforms with known endianness and known instructions). There’s another file called binary_file_stuff.h that defines a bunch of the same routines, but operating on files. On my list was to check these for redundancy. Well, the functionality is pretty well divided, but I find it a little confusing that they’re two separate files, so I am going to combine them into one. Also, I see some procedures in binary_file_stuff that are no longer used because they represent higher-level transactions that no longer make sense to do that way (stuff like put_vector3, put_quaternion, etc). So, deleting those, putting the rest into byte_reading.h, getting rid of binary_file_stuff.h, change the includes of all using sites.

[In general the file stuff is not used by high-level code, because in a game you usually want to do data transactions in memory, because you often don’t know where your ‘file data’ came from, and it might come from different places during development and release – for example, during development you might be loading from loose files on disk, but during release you might be looking at a section of a huge pre-loaded package. It simplifies life if you just always deal in memory, and in the cases where you are explicitly loading files, first load the file then pass it to the back-end routines that parse memory. The binary_file_stuff.h is used mainly for … HMMMM ] Hey hold on. After auditing, it looks like put_u4b and the like are only used in one place, and that place could easily go to memory, so quite possibly all this stuff can go away. So before combining the files, I will just get rid of everything in binary_file_stuff.h that I can.

(Earlier in the history of the game, the file versions of these operations were used pretty heavily, but that changed as I did all the work to make things run with preloaded packages…)

The one place that still uses the file versions of these routines is in the routine that saves the ‘Type Manifest’ to a file during entity serialization; the type manifest tells us which entity types are in the file (listing them by name), so that we can use indices from then onward in order to make serialization faster and smaller, but also robust (it won’t break if whoever reads the file later on adds or removes entity types).

Here’s the code:

void save_type_manifest(File_Handle *f) {
    Portable_Type_Manager *manager = globals.portable_type_manager;
    int num_types = manager->types.items;

    Fast_Encoder encoder;
    pack_int32_fast(&encoder, num_types);

    Portable_Type *type;
    Array_Foreach(&manager->types, type) {
        int revision_number = 1;
        if (type->metadata) revision_number = type->metadata->revision_number;

        pack_string_fast(&encoder, type->name);
        pack_int32_fast(&encoder, revision_number);
    } Endeach;

    unsigned char *data;
    int len;
    encoder.get_result(&data, &len);

    put_u4b(len, f);
    fwrite(data, len, 1, f);
}

Basically, the only reason we are calling put_u4b to the file is because we don’t know the length until after we finish packing stuff, but the length is supposed to go into the file before the stuff we pack. But that’s fine, this can be handled in a very standard way – we can just pack a 0 and then overwrite that memory when we know the length.

(Note that this code implies there is even more redundancy here, since the so-called ‘Fast_Encoder’ has routines for packing various kinds of things, and pack_int32_fast is essentially just a wrapper around put_u4b that also ensures space exists. But we won’t worry about that now.)

So: pack the zero. Overwrite it with put_u4b to memory when done. Test this by opening the editor, editing a level, saving it back out, and making sure we can still play it!

The code has become:

void save_type_manifest(File_Handle *f) {
    Portable_Type_Manager *manager = globals.portable_type_manager;
    int num_types = manager->types.items;

    Fast_Encoder encoder;

    auto poke_position = encoder.output_len; // This will be 0, since we have not packed anything yet, but this could possibly change later, especially if we change this to pass in a Fast_Encoder as an argument rather than writing to a file. -jblow, 18 July 2016.

    pack_int32_fast(&encoder, 0); // This is where the length will go later. 4 bytes.

    pack_int32_fast(&encoder, num_types);

    Portable_Type *type;
    Array_Foreach(&manager->types, type) {
        int revision_number = 1;
        if (type->metadata) revision_number = type->metadata->revision_number;

        pack_string_fast(&encoder, type->name);
        pack_int32_fast(&encoder, revision_number);
    } Endeach;

    unsigned char *data;
    int len;
    encoder.get_result(&data, &len);

    char *cursor = (char *)&data[poke_position];
    put_u4b(&cursor, len - 4); // -4 because we don't include the length itself in the length computation.

    fwrite(data, len, 1, f);
}

And testing it, it works just fine. save_type_manifest is also cleaner, since it no longer mixes file and memory operations, so it’d be very easy to factor the file part elsewhere now.

Use C++11 initializers in Fast_Encoder, Fast_Decoder.

Finally, delete binary_file_stuff.h, rather than merging it with byte_reading.h.

Line count: 73,310. Total savings: 141 lines, 1 file!

I feel a bit tired, let’s take another break.

Dealing with email.

It looks like dlmalloc (a malloc implementation by Doug Lea) is still being included in the code. Back around the midway point of Braid development, I included this in the codebase as a way to get faster allocations and fewer problems with fragmentation than with Microsoft’s libc. Then I made the Isolated_Heap_Pool, and dlmalloc gave me a way to portably create separate heaps, and drastically reduce fragmentation. But later I switched on Windows to just using HeapAlloc. These days, dlmalloc is not considered a highly-performing malloc implementation any more, and anyway, all reasonable operating systems now ship with pretty-okay-quality malloc implementations so a substitute is not required (and if we did want a substitute, it would be something newer than dlmalloc!) But the code was still hanging around in case I ever wanted to bring it back. Also, there’s a bunch of overloads of operator new, delete, etc, that call malloc tracking routines so I can graph memory allocations in the profiler; but that stuff is ugly in C++ (at least back then, there was no actual good way of replacing allocators globally; I think there is now?) and I’d probably rather use external tools such as RAD Game Tools’ Telemetry instead (though that particular tool is not free, I am guessing/hoping there is something reasonable that is free now).

I’m going to delete the dlmalloc stuff, and then just comment out the malloc tracking stuff (but not delete it, just in case). Then delete dlmalloc.c.

Line count: 70,433. Savings: 2,877 lines, 1 file. (The bulk of this was dlmalloc.c).

There is still some stuff in the platform-specific code for unix platforms that I haven’t cleaned out (but did on Windows). Also, looks like nobody calls get_year_month_and_day() on any platform. Also, looks like fileutil.cpp,h were added to this folder by Hothead but are not used by anything (maybe it was for their launcher or something but we are using a different launcher now). Delete those.

Count: 69,842 lines; Savings: 591 lines, 4 files, 1 folder.

Now it’s time to attack that bullet point from Part 1, “Make Entities_By_Type use Array and not List. Remove List.” Though now that I have been looking around the code some more, maybe it won’t be prudent to remove List since it’s possible someone else uses it. But let’s see. In any case this is going to be kind of ugly because it’s going to involve a lot of search-and-replace and a lot of compiler errors.

Basically after changing the data structure, I will have to replace tons of little code snippets that look like this:

List *l = get_portables(&ptype_Puzzle_Piece);

Entity_Puzzle_Piece *e;
Foreach(l, e) {
    if (e->portable_flags & PORTABLE_KILLED) continue;
    if (e->piece_index == piece->piece_index) return e;
} Endeach;

with things that look like this:

Each (Puzzle_Piece) {
    if (it->piece_index == piece->piece_index) return it;
}

It’s a lot more concise, for sure. Each is a special macro just for iterating over entity types, so it does the check for PORTABLE_KILLED itself, which checks to see if an entity has been destroyed but not yet cleaned up at the end-of-frame. Experience shows that remembering to check this manually is as error-prone as you would expect, so it makes a lot of sense to build it into a macro or iterator.

This is going to be a big job and I may not finish today. Maybe I could automate this somehow but, ehh, it just seems unlikely. Let’s just see how it goes. A global search finds 230 uses of Foreach, most of which I will want to replace. Oooookay.

On Entity_Manager, replace

List *lists_by_type;

by

Array <Entity *> *arrays_by_type;

And now of course there are tons of errors all over the project. (Visual Studio reports 222 errors but these numbers tend to be unreliable). So, add the Each macro and find a file that only iterates a little bit in order to test it out. Looks like pieced_image.cpp only does it once. So let’s copy Each over from The Witness and modify it cosmetically. Look at this thing of ‘beauty’:

#define Each(_manager, _Type) \
    if (auto _a = get_portables(_manager, &ptype_##_Type)) \
        if (int count##__LINE__ = _a->items) \
            for (int i##__LINE__ = 0; i##__LINE__ < count##__LINE__; i##__LINE__++) \
                if (auto it = (Entity_##_Type *)((*_a)[i##__LINE__])) \
                    if (it->portable_flags & PORTABLE_KILLED) continue; else

All the LINE stuff is just there to let us name variables in a way that is very unlikely to conflict with anything in the user code that comes after the ‘else’.

Update from the next morning: The i##LINE stuff is not correct! It is fine in that the code will execute as intended, but LINE will not expand to the current line number unless it is wrapped inside another macro, because the C Preprocessor is a very special flower that way. Ignacio’s original implementation did wrap those in another macro, but I “cleaned that up” for this veresion because I found it hard to read. So the version that actually names the variables properly is:

#define NV_STRING_JOIN2(arg1, arg2) NV_DO_STRING_JOIN2(arg1, arg2)
#define NV_DO_STRING_JOIN2(arg1, arg2) arg1 ## arg2
#define Each(_manager, _Type) \
    if (Auto_Array<Entity *> *_l = get_portables<Entity>(_manager, &ptype_##_Type)) \
        if (int NV_STRING_JOIN2(count,__LINE__) = _l->count()) \
            for (int NV_STRING_JOIN2(i,__LINE__) = 0; NV_STRING_JOIN2(i,__LINE__) < NV_STRING_JOIN2(count,__LINE__); NV_STRING_JOIN2(i,__LINE__)++) \
                if (Entity_##_Type *it = (Entity_##_Type *)((*_l)[NV_STRING_JOIN2(i,__LINE__)])) \
                    if (it->portable_flags & PORTABLE_KILLED) continue; else

Why do we need two levels of indirection on the substitution? I have no idea. This kind of thing is why I would so much rather build a new programming language than keep working in C++. End of interruption; now back to yesterday.

Note also that we name the iterator variable ‘it’ in case we want to use that. That is my preferred form, but so that I don’t go insane renaming variables, I will also make a version that takes a name. (This version of Each takes an Entity_Manager as an argument, which is good practice, but most of Braid assumes you want to talk about a global entity manager, so it’s more terse; the global variable thing causes bugs sometimes, but that is how it is right now, and it is easy enough to change that later if I want to; one big change at a time please! So mostly I am going to use a version of Each that takes no Entity_Manager but an explicit iterated value name.)

After a little bit of the usual C++ macro typo hunt, pieced_image.cpp compiles just fine, so now it is time for me to work through all the files having compile errors, one by one:

cannon.cpp, collisions.cpp, entity_manager.cpp, entity_panel.cpp, game_view.cpp, greeter.cpp, gun_boss.cpp

(Visual Studio says 182 errors to go!)

guy.cpp, hud_hints.cpp, key.cpp, levels.cpp, main.cpp, main_menu.cpp, platform.cpp

(70 errors to go!)

prefetch.cpp, prince.cpp, princess_level.cpp, puzzle_assembly_screen.cpp, puzzle_frame.cpp, reverse_effect.cpp, rumble.cpp

(26 errors to go!)

shaders_common.cpp, sound_player.cpp, story_page.cpp, switch.cpp, text_screens.cpp, world_panel.cpp

And we’re DONE!! And it runs! And I can play!! And oh wait, when I go onto the story screens there’s a crash, but it takes just a few seconds to fix; there was some unintuitive code that changed meaning when I did the edit, and it’s better to make it more intuitive.

I wonder how many other bugs I introduced doing that. Hopefully none! I probably averted a few, and took note of a couple of potential issues to look into. Well, this is what testing is for.

Count: 69,447. Savings: 395 lines.

While doing that, I noticed that in world_panel.cpp (which is one of the main files for the in-game level editor) there is a bunch of single-use code that I used to batch-convert entities at various times during development, as we made different decisions about how those entities should be. I left the code in so that I would have quick examples to copy and paste when I needed to do new things. This is stuff like, “convert all grass entities that have one particular name to have some other particular name”, and so on. But these specific examples are not going to be useful, and I feel like it’s best to comment them out to show that they are no longer live code that’s of active use. (But as long as I comment it out instead of deleting it, it’ll be there for potential future resurrection). Also if I comment it out, the line count goes down.

Commenting out: convert_grass, rename_ledges, fix_ledges, move_particles, list_grass, fix_non_unique, fix_image, fix_images, convert_rain, do_detect, detect_and_replace, particles_detect_and_replace, convert_to_star, convert_to_platform.

Count: 69,213. Savings: 234 lines.

Hey, I just had an hour-plus-long situation where I wrote a bunch of stuff and it turned out to be wrong because I forgot how things work and of course I did it better than that.

It had to do with iterating over entities by type and whether you are allowed to schedule entities for destruction while doing this, or not. In various engines I have had various policies about whether this was okay. In Braid it is okay, that is part of the point of ‘scheduling’ entities for destruction rather than destroying them immediately. But I forgot it was okay, in part because I saw code in various spots that was not doing this – instead it was making a separate array of entities to destroy, then when done iterating over the array by type, it would then iterate over that to_destroy array and schedule everyone in it for destruction. This confused me so I wrote many paragraphs talking about the problem and how to solve it.

Well, I just deleted all those paragraphs. But anyway, an example:

EachName(debris, Debris) {
    if (!debris->inserted) continue;

    globals.entity_manager->schedule_for_destruction(debris);
}

Is this legal? If you mess with the array of Debris that EachName is implcitly accessing, then it is bad news. But Braid is more robust than this so it’s fine.

So on my to-do list for later is to go clean up all the times when I buffer a separate array of entities to destroy, which I presume date back to older code. (During development the policy of what is okay changed a few times until I landed on what it is now.)

This is also a sign that I am getting tired and can use a break / some sleep. Well, that’s fine, it has been a good day of work. So, good night!