"very bad init routine"In [url=http://forums.nesdev.com/viewtopic.php?p=140215#p140215]this post[/url], koitsu wrote:I finally got around to looking at this code. Ho-ly-shit. Really? God dude, I don't even know where to begin. The code itself is fine (sort of -- very bad init routine from the look of it)
For the same reason that C libraries include a pile of code that runs before the jump to main() in "crt0.s".but it's nearly impossible to follow given formatting, things in files that don't make any sense (like why is the reset vector code in snesheader.s)
What sort of "structure" should have been used?and a ton of other things. Can you honestly read this given the formatting and almost "inline" comments without any actual structure?
You know the primary programming language here isn't C, right?tepples wrote:Quote:but it's nearly impossible to follow given formatting, things in files that don't make any sense (like why is the reset vector code in snesheader.s)For the same reason that C libraries include a pile of code that runs before the jump to main() in "crt0.s"
(Hell, I don't even know anything except ASM. Everything else might as well be scribbles for all I care.)Code: Select all
;==============================================================================
; main
;==============================================================================Code: Select all
cmp #28
bcs notHitLeft
lda #28
sta player_xhi
stz player_dxlo
beq doneWallCollision
notHitLeft:
cmp #212
bcc notHitRight
lda #211
sta player_xhi
stz player_dxlo
notHitRight:
doneWallCollision:Code: Select all
cmp #28
bcs notHitLeft
lda #28
sta player_xhi
stz player_dxlo
beq doneWallCollision
notHitLeft:
cmp #212
bcc notHitRight
lda #211
sta player_xhi
stz player_dxlo
notHitRight:
doneWallCollision:With two labels, you can put code between them later and it's still going to act correctly, and you don't need to remember to update the branches. If I had done that then Sliding Blaster wouldn't have ever had that crash bug.Espozo wrote:Why in the world do you even have two different places you can jump to if there's not a single thing in between them?
Got ninjaed by NovaSquirrel. Anyways, I do the same thing quite often, and it's to help with code clarity and maintainability.Espozo wrote:Why in the world do you even have two different places you can jump to if there's not a single thing in between them?
Code: Select all
bne doneWallCollision ; jumps always
Ideally, that's what .proc is for. But if .proc isn't noticeable in the default settings of other users' existing favorite text editors, then I might need to put doc comments on all subroutines, even subroutines whose behavior is evident from the name and which takes no arguments. Here's what a doc comment from ppuclear.s looks like:Espozo wrote:Walker looks a lot more organized because it has huge barricades like this to separate different parts of the code. (The parts you jmp and jsr to.)
Code: Select all
;;
; Clears a nametable (2048 bytes) to a constant value.
; @param X starting address of nametable in VRAM (16-bit)
; @param Y value to write (16-bit)
My coding convention often includes a blank line after a "skip forward" type label, roughly corresponding to the End If of an If ... Then ... End If block. You are correct that I was missing a blank line.Also, you need to change stuff like this:
Because the jumps are for different reasons, one for not having hit the right side wall, and one for being completely done with collision checks for this frame. It helps with inserting additional checks at the right place should they become needed.Why in the world do you even have two different places you can jump to if there's not a single thing in between them?
Code: Select all
; Test for collision with side walls
; (in actual games, this would involve collision with a tile map)
cmp #28
bcs notHitLeft
lda #28
sta player_xhi
stz player_dxlo
bra doneWallCollision
notHitLeft:
cmp #212
bcc notHitRight
lda #211
sta player_xhi
stz player_dxlo
notHitRight:
; Additional checks for collision, if needed, would go here.
doneWallCollision:
Code: Select all
; Test for collision with side walls
; (in actual games, this would involve collision with a tile map)
cmp #28
bcs notHitLeft
lda #28
sta player_xhi
stz player_dxlo
bra doneWallCollision
notHitLeft:
cmp #212
bcc notHitRight
lda #211
sta player_xhi
stz player_dxlo
notHitRight:
; Additional checks for collision, if needed, would go here.
doneWallCollision:
Current order of subroutine:Also, you seem to have the code that moves the character and checks for collision smack dap in between the code that sets the BG mode and stuff (I think?) and some code that does something with the tile data.
In my ca65 coding convention, everything from .proc to .endproc can be pulled out and put into another file.I just think the main problem is that it is not a very clear where something ends and something starts.
Yes.tepples wrote:Would this make more sense if I were to indent the "then clause" stuff?
I know this may sound a bit crazy to you, but another thing that would make the code a lot more "comprehendible" would be if you actually separated these into separate files.tepples wrote:Current order of subroutine:Top level functions: nmi_handler, main"Game logic" subroutine: move_playerGraphics subroutines: draw_player_sprite, draw_bgGraphics data
What? You don't have to put everything in a separate file, but maybe try taking the largest subroutine (Isn't it move_player?) and putting it by itself. It's also usually nice to have graphics have a separate file, because otherwise, you'll end up with a huge main file.tepples wrote:Are you talking one subroutine per file? I've seen that in libraries to allow the linker to remove unused subroutines. If not, then what level of granularity between that and the current state of the demo would be best?