Hey folks, Im working on a clone of the Atari 2600 pacman (the one everyone hated, but was the most selling game for the console). Why? Well, why not. And the 128 doesnt have a lot of native games for it.

Anyway, I wanted to share the code and see if anyone had any thoughts on improving it. Sometimes when youre so deep in something like this, you wonder if your approach is just bad. Any thoughts or tips are welcome. As of today, the player movement is finally working. It was complicated by the fact that Im aiming for pixel matching of the original map.

https://github.com/Commodore64128/ataripac128

Just a few general comments, as I haven't tried to re-write your code. Just my $0,02 cents.

- on the 6502 it's generally better to count down instead of up so you can take advantage of the free zero flag setting. A simple BNE instead of CMP #somenum followed by BNE
- you might want to change the sequence altering the interrupt vector so it reads PHP SEI and later PLP instead of SEI...CLI. Sure, if you have total control of the machine, things are unlikely to change and you're perfectly find as is. Still.
- you might also want to save whatever the address of the interrupt vector currently is and exit your interrupt handler with JMP (saveaddr) instead of hard-coding a value that something else might have already changed. You're probably okay as is, but it is a bit neater.
- you have some delay loops running in your interrupt handler. Bad form. Set a few flags and get out of the handler as soon as possible. Pay attention to the flags in your main (non-interrupt) game loop and do your delays there.

Nice, thanks for sharing! I've never programmed the Commodore systems so it's interesting to see how it's done.

Some additional comments on general 6502 programming style, based only on looking at game.asm. These are maybe nit-picky optimisations, and everyone has their own opinion, but personally I think these are good things to bear in mind and do habitually, as it costs little in mental effort but produces more optimal - and to a 6502 expert, more immediately understandable - code. I also don't always remember to do these things up front, but when I don't, I often find myself regretting it later on.

It's worth reading Garth Wilson's advice here: https://wilsonminesco.com/6502primer/PgmTips.html


Generally you don't need the "cmp" on line 100 - after the "and", the zero flag will be set if the result was 0. So you could remove the "cmp" and use "bne" instead of "beq" on line 101.

Depending what else it's used for, you could also consider "lsr collision_flg" on line 98 - then the bit you're interested in goes into the carry, but it modifies the variable as well which could be a good or a bad thing.

Or if you could arrange for the interesting bit to be bit 6 or bit 7 then simply "bit collision_flg" would be enough to set a flag depending upon the state of the bit, very cheaply without any "and", "cmp", or modification of the variable. It looks like it came from a hardware register though which I don't know the meaning of, so perhaps it's not possible to change which bit gets set here.


What are these addresses used for? It'd be nice to give them names. You can see from the code what is happening, but names would make it clearer why it's being done. "scr_data_ptr" and "screen_ram_ptr" for example for the first two.


Similar to the case above for testing bit 1 - if it's possible to change the order of these tests in joystick_handler, then you could just "ror joy_cache" to get the bottom bit into the carry and then handle upward movement first; and then "ror joy_cache" again to get the next bit and handle downward movement, etc. But it looks like you only want to process one direction, and this change would alter the gameplay, so maybe not something you'd actually want to do here.


This can be written as "inc screen_addr_lo", "bne +", "inc screen_addr_hi" - fewer instructions and fewer cycles at runtime as well!

Similarly:

Here you did use the branch to detect the carry case, but could have used "inc SPRITE_0_X_POSITION" and then "bne +" instead of "bcc +", and saved a few more instructions and cycles.


This is tough on a CPU without hardware multiplication. Note though that 40 = 5*8, and only has two bits set. So you can hardcode a multiply-by-5 routine with 16-bit carry (asl+rol, asl+rol, adc+adc#0) and then just shift three more bits, to mulitply by 40.

If you only usually move the object relatively (e.g. one square or pixel left/right/up/down at a time), then it's even better to maintain a separate variable tracking the address of the object on the screen at all times, and add or subtract 40 to that at the same time as altering its coordinates. The movement is slightly more expensive, but the rendering is much cheaper as you don't need to multiply. In some cases you don't even need the unmultiplied coordinate any more, you can just use the screen address to track the object's position! But it's harder to get your head around. A BBC Micro game I disassembled a year or so ago did this a lot - even tracking the player's lives by the screen memory address of the greatest life "pip" on the screen, and the game time remaining by the screen address of the top of the "time remaining" bar that's displayed for the player.

Thanks for the great tips! Ill be going over these in detail and make the recommended changes. Exactly the review I was looking for, thanks guys!

Hi,

You could save a few Bytes in game.asm (reset: lines 215 ff.)

Since you load the first value (x=0), why not

There are some of those (unnecessary) X uses.

I would just use the Accu:

_________________
ROR A? Where we're coding, we don't need A.