Atari 2600 pacman clone for the Commodore 128

[xlar54]

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

[teamtempest]

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.

[gfoot]

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

Code: Select all

 98	lda collision_flg
 99	and #$01
100	cmp #$01
101    beq collide

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.

Code: Select all

212 -	lda ($64),y
213 	sta ($66),y
214 	lda ($62),y
215 	sta ($60),y
216 	iny
217 	bne -

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.

Code: Select all

449 	lda joy_cache
450 	and #$08
451 	bne +

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.

Code: Select all

513 	; we are looking one space to the right
514 	; so add 1 to the location
515     lda screen_addr_lo 
516     clc 
517     adc #$01
518     sta screen_addr_lo
519     lda screen_addr_hi
520     adc #$00
521     sta screen_addr_hi

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

Similarly:

Code: Select all

541 	clc 
542 	lda SPRITE_0_X_POSITION
543 	adc #$01
544 	bcc +
545 	ldx #$01
546 	stx SPRITE_XMSB
547 +	sta SPRITE_0_X_POSITION

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.

Code: Select all

892 	ldy #40   						; Number of columns per row
893 	jsr mult_8b_by_8b

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.

[xlar54]

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!

[Broti]

Hi,

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

Code: Select all

	ldx #$00
	lda pacman_anim_pointers,x
	sta SPRITE_0_POINTER

	ldx #$00
	lda ghost_anim_pointers,x
	sta SPRITE_1_POINTER
	sta SPRITE_2_POINTER
	sta SPRITE_3_POINTER
	sta SPRITE_4_POINTER

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

Code: Select all

	lda pacman_anim_pointers
	sta SPRITE_0_POINTER

	lda ghost_anim_pointers
	sta SPRITE_1_POINTER
	sta SPRITE_2_POINTER
	sta SPRITE_3_POINTER
	sta SPRITE_4_POINTER

There are some of those (unnecessary) X uses.

Code: Select all

; draw screen
	lda #<scr_data
	ldx #>scr_data
	sta $64
	stx $65

	lda #<SCREEN_RAM
	ldx #>SCREEN_RAM
	sta $66
	stx $67

I would just use the Accu:

Code: Select all

lda #<scr_data
sta $64
lda #>scr_data
sta $65

lda #<SCREEN_RAM
sta $66
lda #>SCREEN_RAM
sta $67