Fix missing birds

[techknight]

Summary

Commit bb02a2d introduced a bug that unpredictably eliminates birds during gameplay due to an undefined color mapping entry. This PR sets an explicit color palette for the birds, and fixes how their minimap dot color is referenced, so that they always appear when and as they're meant to.

What caused the problem?

src/swinit.c sets the object color to 9 when creating the flock of birds:

static OBJECTS *initflock(const original_ob_t *orig_ob)
{
.
.
.
ob->ob_clr = 9;

Before commit bb02a2d, src/vid_vga.c defined this set of palettes, 0 through 8:

static unsigned char color_mappings[][4] = {
    { 0, 1, 2, 3 },  // Cyan fuselage, magenta wings
    { 0, 2, 1, 3 },  // Magenta fuselage, cyan wings
    // New colors:
    { 0, 1, 3, 2 },  // Cyan fuselage, white wings
    { 0, 2, 3, 1 },  // Magenta fuselage, white wings
    { 0, 3, 1, 2 },  // White fuselage, cyan wings
    { 0, 3, 2, 1 },  // White fuselage, magenta wings
    // Now we're getting into boring territory...
    { 0, 1, 1, 3 },  // All-cyan
    { 0, 2, 2, 3 },  // All-magenta
    { 0, 3, 3, 3 },  // All-white
};

In the same file, under Vid_DispSymbol(), the color palette is chosen by checking the value of clr -- again, 9 in the case for birds -- and setting the mapping value to 0, which in this example corresponds to the player 1 palette:

color_mapping = color_mappings[clr == 2 ? 1 : 0];

From commit bb02a2d onwards, the color palette is instead chosen by direct lookup of color_mappings:

color_mapping = color_mappings[clr];

src/vid_vga.c has a slightly re-ordered list of palettes, but there are still only 0 through 8 of them:

static unsigned char color_mappings[][4] = {
    { 0, 3, 3, 3 },  // All-white                     - OWNER_NONE?
    { 0, 1, 2, 3 },  // Cyan fuselage, magenta wings  - OWNER_PLAYER1
    { 0, 2, 1, 3 },  // Magenta fuselage, cyan wings  - OWNER_PLAYER2
    // New colors:
    { 0, 1, 3, 2 },  // Cyan fuselage, white wings    - OWNER_PLAYER3
    { 0, 2, 3, 1 },  // Magenta fuselage, white wings - OWNER_PLAYER4
    { 0, 3, 1, 2 },  // White fuselage, cyan wings    - OWNER_PLAYER5
    { 0, 3, 2, 1 },  // White fuselage, magenta wings - OWNER_PLAYER6
    // Now we're getting into boring territory...
    { 0, 1, 1, 3 },  // All-cyan                      - OWNER_PLAYER7
    { 0, 2, 2, 3 },  // All-magenta                   - OWNER_PLAYER8
};

Since color_mappings[9] is not defined, this will sometimes cause the flock of birds to display on the minimap during play, but not be visible or interactable in the gameplay area:

Bringing back the birds

I chose to create a color_mappings[9] entry specifically for the birds. They're to appear in-game in white, and on the minimap in cyan:

static unsigned char color_mappings[][4] = {
    { 0, 3, 3, 3 },  // All-white                     - OWNER_NONE?
    { 0, 1, 2, 3 },  // Cyan fuselage, magenta wings  - OWNER_PLAYER1
    { 0, 2, 1, 3 },  // Magenta fuselage, cyan wings  - OWNER_PLAYER2
    // New colors:
    { 0, 1, 3, 2 },  // Cyan fuselage, white wings    - OWNER_PLAYER3
    { 0, 2, 3, 1 },  // Magenta fuselage, white wings - OWNER_PLAYER4
    { 0, 3, 1, 2 },  // White fuselage, cyan wings    - OWNER_PLAYER5
    { 0, 3, 2, 1 },  // White fuselage, magenta wings - OWNER_PLAYER6
    // Now we're getting into boring territory...
    { 0, 1, 1, 3 },  // All-cyan                      - OWNER_PLAYER7
    { 0, 2, 2, 3 },  // All-magenta                   - OWNER_PLAYER8
    // Color scheme for birds, set by initflock in swinit.c 
    { 0, 1, 0, 3 },  // Cyan dot on minimap, white bird in-game
};

Wait, what's that last line about the minimap?

Yes, there's a sneaky problem there. In the older code, the pixel color on the minimap is set in swstbar.c like this:

static void dispmapobjects(void)
{
.
.
.
                Vid_PlotPixel(x, y, ob->ob_clr);

That means we're sending the number 9 over to Vid_PlotPixel() in vid_vga.c:

void Vid_PlotPixel(int x, int y, int clr)
{
    unsigned char *sptr
        = vid_vram + (SCR_HGHT-1 - y) * vid_pitch + x;

    *sptr = clr & 3;
}

When dealing with the birds, that final line calculates 9 AND 3, which gives us 1, or a color of cyan. The problem is, if we change which palette entry the birds are using -- for example the "All-white" palette at color_mappings[0] -- the birds will then have a minimap dot color of black!

Setting the minimap color on purpose

So, instead of relying on the value of clr & 3 happening to be correct, dispmapobjects() in swstbar.c now explicitly uses the fuselage color from the color mappings in vid_vga.c.

            if (y < SCR_MNSH-1) {
                unsigned char* mapping = getColorMapping(ob->ob_clr);
                unsigned char map_color = mapping[1];

                Vid_PlotPixel(x, y, map_color);
            }

The birds are back!

• Birds now always appear when they're supposed to (their ob_clr is no longer used to reference an undefined array element)
• In-game bird color and minimap bird color can be set independently of player colors

[slicer69]

All looks good here. Tested and committed this change. Thank you!

[fragglet]

Saw this last night but didn't have time to respond. I might back this out and redo it because I'm not sure I like the approach. But @techknight thank you for the fix regardless and for the incredibly detailed write-up!

[techknight]

Saw this last night but didn't have time to respond. I might back this out and redo it because I'm not sure I like the approach. But @techknight thank you for the fix regardless and for the incredibly detailed write-up!

No prob! It's tricky - I think that at its simplest, one could probably just add an element [9] into the color mappings (ie { 0, 0, 0, 3 }, // Bird and call it a day, but when I realized that I couldn't just change ob->ob_clr = 9; to ob->ob_clr = 0; (the all white palette) without the birds disappearing from the minimap, it felt like a slightly bigger change could make sense.

I was thinking as well that depending on future multiplayer plans, it might be appropriate to have the ability to set the bird's minimap color to white to distinguish it from all the players flying around, though that as well would require other considerations.

[fragglet]

@techknight - I just pushed my alternate fix if you're curious to see. I retained one part of your fix which I really liked - ie. using the color mapping to pick the color on the map. I hope the fact that I went with a different fix doesn't deter you from any future contributions, because this is really is appreciated :)

[techknight]

@fragglet All good! 🙂