Shortening duplicate code for left and right direction

This is an archive of a topic from NESdev BBS, taken in mid-October 2019 before a server upgrade.
View original topic
Shortening duplicate code for left and right direction
by on (#176535)
In my code, I have a few places where I have to implement the same thing twice, only mirrored for when the character looks to the right and where it looks to the left.
The algorithm is exactly the same, but there are a bunch of variables, constants and comparisons that are different.

Here is an example. Do you have any creative idea how I can make this shorter, so that it occupies less ROM space?
Code:
if (Chrs.Direction[IndexScarlett] == DirectionRight)
{
   for (i = 0; i < ScarlettPixelsPerMovementRight; ++i)
   {
      if (Chrs.X[IndexScarlett] < Chrs.X[IndexAmy]
       && Chrs.X[IndexScarlett] != ScarlettRightmostX)
      {
         ++Chrs.X[IndexScarlett];
      }
      else
      {
         InitializeScarlettMovementUp();
         break;
      }
   }
}
else
{
   for (i = 0; i < ScarlettPixelsPerMovementLeft; ++i)
   {
      if (Chrs.X[IndexScarlett] > Chrs.X[IndexAmy]
       && Chrs.X[IndexScarlett] != ScarlettLeftmostX)
      {
         --Chrs.X[IndexScarlett];
      }
      else
      {
         InitializeScarlettMovementUp();
         break;
      }
   }
}
Re: Shortening duplicate code for left and right direction
by on (#176537)
Besides the usual, dual array comparisons like "Chrs.X[IndexScarlett] > Chrs.X[IndexAmy]" generate absolutely terrible code in cc65. Work around that by placing one of those to a var right before the comparison, and compare with that.
Re: Shortening duplicate code for left and right direction
by on (#176538)
In this specific case, it shouldn't be a problem because IndexScarlett and IndexAmy are compile time constants (created with #define) and not variables. Therefore, the assembly access should be something like LDA Chrs_X + IndexAmy, i.e. no runtime calculation of the address with a register, but compile-time calculation.
Re: Shortening duplicate code for left and right direction
by on (#176540)
I'd say if it's under 256 bytes of code, just eat the loss.
Re: Shortening duplicate code for left and right direction
by on (#176541)
I agree with Dwedit.

It's not just the movement that differs (++ vs --), it's the logic (if x position != max left vs max right).
Keep it.

Maybe refactoring for speed. If possible.
Re: Shortening duplicate code for left and right direction
by on (#176547)
Speed is not an issue.

And there are a good bunch of these kinds of code. And I try to save space for my voice sample.

I'll probably declare some global variables and put all the different values into these variables first. Then I'll use the algorithm with the variables instead of the constant values.

++ vs. -- will then be represented as += either with a positive or with a negative value.

Then I'll check which of the two is smaller and if the saved space is big enough to make an impact.
Re: Shortening duplicate code for left and right direction
by on (#176549)
Actually I have a similar "problem" in my game. I have to fit a fairly large game in 32kb, so I already optimized every single routine and level data for size and compressed pretty much everything that could be compressed. Yet I often have 4 variations of my routine for left, right, up and down directions.

I guess trying to get rid of that would mean extra math and table lookup, and that would not necessarly save space. For example instead of having a "inc Hpos", "dec Hpos", "inc Vpos" and "dec Vpos" instructions, you'll have to read lookup tables and add +1 or -1 or 0 depending on the direction. The space that is saved by merging the routines is wasted in extra math calculation.

Now that is in assembly, and you code in C, so it's a whole different thing.
Re: Shortening duplicate code for left and right direction
by on (#176559)
DRW wrote:
Here is an example. Do you have any creative idea how I can make this shorter, so that it occupies less ROM space?

You seem to be hoping that there's some trick that will fold left+right into a single reusable operation. I think there are some cases where you can do this sort of thing, but probably most of the time it's not practical to do that.

I don't think I can optimize the code you provided for size in any way that's meaningful to your request. Like, you can rewrite any function in assembly to optimize for space or speed, but that's not what you're asking. (Though, you'd only need to rewrite as many as needed to fit your budget, you don't have to rewrite them all.)

Instead of suggesting some way to combine left+right, I'd probably suggest trying to find a way to update different objects with the same code.

Like in this case it seems that you have an array of characters. Probably many characters could use the parts that check direction, check a boundary, and move a pixel to the right or left if they're allowed. Instead of having a function just for Scarlett, maybe you can replace "ScarlettRightmostX" with a bounds parameter that every character has, so they can all use the same move code.

If Scarlett has something unique, e.g. in this case she seems to be chasing Amy, and then has some "InitializeScarlettMovementUp" routine, maybe you can move these things to a separate function. Put all the generic move left/right stuff in one function call that all the characters can use, then put the stuff that only applies to Scarlett in her own function that is called separately. Maybe Amy's position could even be used as the bounds parameter, instead of whatever ScarlettRightmostX is, eliminating the need for an additional test?

That's the kind of thing I'd suggest if you're looking to save code size. Find ways to reuse code, rather than hoping there is a small scale C optimization trick that you've overlooked. Even if there was an applicable trick in C that could fold left+right together, rewriting directly in assembly would most likely cut the size down a lot better.
Re: Shortening duplicate code for left and right direction
by on (#176566)
rainwarrior wrote:
You seem to be hoping that there's some trick that will fold left+right into a single reusable operation.

Yeah, something that I haven't thought of until now.

I guess the first thing that I'll try is something like this:
Code:
if (Chrs.Direction[IndexScarlett] == DirectionRight)
{
   temp[0] = ScarlettPixelsPerMovementRight;
   temp[1] = ScarlettRightmostX;
   temp[2] = 1;
   intTemp[0] = Chrs.X[IndexScarlett];
   intTemp[1] = Chrs.X[IndexAmy];
}
else
{
   temp[0] = ScarlettPixelsPerMovementLeft;
   temp[1] = ScarlettLeftmostX;
   temp[2] = -1;
   intTemp[0] = Chrs.X[IndexAmy];
   intTemp[1] = Chrs.X[IndexScarlett];
}

for (i = 0; i < temp[0]; ++i)
{
   if (intTemp[0] < intTemp[1]
    && Chrs.X[IndexScarlett] != temp[1])
   {
      Chrs.X[IndexScarlett] += temp[2];
   }
   else
   {
      InitializeScarlettMovementUp();
      break;
   }
}

I'm not sure in how far this will save space, especially when I do this method every time such a code appears.

rainwarrior wrote:
Instead of suggesting some way to combine left+right, I'd probably suggest trying to find a way to update different objects with the same code.

The characters behave very differently from each other. For example, one character walks left and right like a red Koopa Troopa. The other opponent does nothing but jumping. And Scarlett is a flying opponent that's totally independent from the platforms.

The common stuff, like jumping physics, collision checks with the platforms, behavior when the character is stunned, are already done in generic functions that are shared by everybody.
So, I don't think that I can greatly reduce the code any further here.

But this whole if DirectionRight DoStuff else DoSameStuffOnlyMirrored is something that I have very often in my code, so I thought there might be some general solution for this situation.

rainwarrior wrote:
rewriting directly in assembly would most likely cut the size down a lot better.

Yeah, this would of course make things shorter. But I like the idea that my game is completely written in C and it's still a never-lagging fast scrolling game with platforms, jumping physics and four characters on the screen at once (player character + two opponents + small flying opponent) and not just some single-screen puzzle or turn-based game.

Well, completely written in C apart from low level stuff (crt0 startup code, NMI, access to the NES ports, sprite updates) and some totally generic functions (randomizer, memcopy, array comparison, array shift and of course the music with FamiTone)
But I never got to the point where I had to use assembly for normal game logic.
Also, I'm pretty glad that all the assembly stuff is finally over. That language is a pain in the ass.

The funny thing is that I'd still have more than enough ROM space (and enough CPU time). So, if I scrapped the company logo to get some tiles back, I could even include a whole new opponent with his own movement pattern if I wanted to. But it's that voice sample that would occupy almost 4KB in the highest quality.

I haven't optimized the functions for the final boss, so I guess I'll still get back a good bunch of ROM space. But I fear that it won't be enough for the sample. So, of course I'm looking for more stuff to optimize (without resorting to Assembly).
Re: Shortening duplicate code for left and right direction
by on (#176569)
DRW wrote:
The characters behave very differently from each other. For example, one character walks left and right like a red Koopa Troopa. The other opponent does nothing but jumping. And Scarlett is a flying opponent that's totally independent from the platforms.

What I mean is to separate characters into component operations. Each of them may be different, but do any of them do some of the same things?
Code:
update_swimmer(unsigned char index)
{
   load_character(index); // copy character into temporary parameters
   follow_player(); // sets direction to follow the player
   move_horizontal();
   move_vertical(); // vertical bounds parameter stored in character prevents going above water line
   if (overlap_player()) hurt_player();
   pack_character(index); // return temporaries to the indexed character
}

update_walker(unsigned char index)
{
   load_character(index);
   // only need to move horizontally, if stopped by bounds or collision, reverse direction
   if (move_horizontal()) temp_direction = -temp_direction;
   if (overlap_player()) hurt_player();
   pack_character(index);
}

update_jumper(unsigned char index)
{
   load_character(index);
   if (!is_jumping() && player.y < temp_y) start_jump();
   move_vertical();
   apply_gravity();
   if (overlap_player()) hurt_player();
   pack_character(index);
}

Possibly you have too few or too varied characters for this to make sense, but in a situation where code reuse is an advantage, you can get a lot of mileage out of generic reusable pieces. If you look for things that more than one character is doing, you might be able to find enough similarities to share. Like, if more than one character jumps, could you make a generic jump routine that is good enough to handle them both? It usually involves a tradeoff of RAM (e.g. every character gets a "jump" variable) or ROM (a table of jump values, one for each character type) or CPU (none of the characters takes advantage of all the code at once).

Also, if it's not too late in the process for this, if you take a small-code approach when designing the characters, this can help a lot in ways that trying to do it as an optimization after design can't. Asking "if I add the ability to jump, what kinds of characters can I make with it?" instead of "which of my characters need to jump?" might be better suited to smaller/reusable code.

e.g. the hazards in VVVVVV (aside from the static spikes) are probably all the same code with different parameters, they are all just things that move in a straight line with a repeating pattern (back and forth, or in line), and various sprites applied. It probably wasn't written this way for the sake of small code, but I think the designer was interested in seeing what kind of interesting problems they could make from a very small set of enemy capabilities.

e.g. the collection of enemies in Earthbound is huge, because they share so much code. Adding an enemy is a matter of adding a few attributes to a table (hit points, etc.), choosing some abilities/behaviours from a list, and drawing a single sprite. This is an extreme example, but just trying to show how when you approach design from a small-code perspective it enables you to do things that you couldn't otherwise, like have a hundred different enemies.
Re: Shortening duplicate code for left and right direction
by on (#176580)
Much of the stuff that you listed is of course already done in generic functions in my game:
Collision check isn't even done in the movement functions, but in the loop that iterates through all characters.
Jumping is handled in generic functions, you just need to pass the character's index as a parameter.
Collision with the platforms is a generic function as well.

So, yeah, I already did a good amount of general-purpose code.
Re: Shortening duplicate code for left and right direction
by on (#176608)
Don't need to do
Code:
   intTemp[0] = Chrs.X[IndexAmy];
   intTemp[1] = Chrs.X[IndexScarlett];
because you can just change the if (Chrs.X[IndexScarlett] > Chrs.X[IndexAmy]to !=, as you're only moving it 1px at a time anyway, and <, >, and != all stop being true at the same time, on equality.
(also, that'll break the function; the intTemps are not updated as you have it set up.)

More typically, you'd just use the "start moving up/down/left/right" functions to (if you've got RAM to spare) set these variables for that actor (Scarlet), and you just add the velocity each time (one pixel at a time if you prefer) and check bounds.
Re: Shortening duplicate code for left and right direction
by on (#176705)
Myask wrote:
you can just change the if (Chrs.X[IndexScarlett] > Chrs.X[IndexAmy]to !=, as you're only moving it 1px at a time anyway, and <, >, and != all stop being true at the same time, on equality.

Huh? Do I understand anything incorrectly here?
I cannot check for !=. How shall this work?

If Scarlett moves to the right and Amy is still located to her right, Scarlett moves on.
As soon as Amy is behind Scarlett, which can happen either by Scarlett reaching Amy or by Amy moving towards Scarlett (in this case, Amy's X position has become smaller than Scarlett's X position when the MoveAmy function was called), Scarlett's movement is changed.

How shall I do this with a simple !=?

Myask wrote:
(also, that'll break the function; the intTemps are not updated as you have it set up.)

Oops. Yeah, good thing I haven't actually implemented anything of that yet. I would have to see if it's shorter to update intTemp every time or to do [if]if (Direction == Right && Amy > Scarlett) || (Direction == Left && Amy < Scarlett[/tt] here.

Myask wrote:
More typically, you'd just use the "start moving up/down/left/right" functions to (if you've got RAM to spare) set these variables for that actor (Scarlet), and you just add the velocity each time (one pixel at a time if you prefer) and check bounds.

This one I didn't understand.
Re: Shortening duplicate code for left and right direction
by on (#176783)
DRW wrote:
Myask wrote:
you can just change the if (Chrs.X[IndexScarlett] > Chrs.X[IndexAmy]to !=, as you're only moving it 1px at a time anyway, and <, >, and != all stop being true at the same time, on equality.

Huh? Do I understand anything incorrectly here?
I cannot check for !=. How shall this work?

If Scarlett moves to the right and Amy is still located to her right, Scarlett moves on.
As soon as Amy is behind Scarlett, which can happen either by Scarlett reaching Amy or by Amy moving towards Scarlett (in this case, Amy's X position has become smaller than Scarlett's X position when the MoveAmy function was called), Scarlett's movement is changed.

How shall I do this with a simple !=?
Whoops, right, that doesn't work because both move.
Quote:
Myask wrote:
More typically, you'd just use the "start moving up/down/left/right" functions to (if you've got RAM to spare) set these variables for that actor (Scarlet), and you just add the velocity each time (one pixel at a time if you prefer) and check bounds.

This one I didn't understand.

[/quote]You'd set a variable that says "Scarlett is moving right" [as you already do], set the "pixels per move" [velocity], a variable that's ±1 (to replace the increment with an add) and the boundary past which she's not going to move, which you CAN do != for, since you're doing 1px at a time and (presumably) the boundaries aren't going to move. It's silly to do temporary variables for the comparison to target (Amy), though, as mentioned.

Really, there's nothing wrong with what you're doing if it works.