A simple sprite demo for teaching

[tepples]

I don't have a Windows+MSYS+Python installation handy to troubleshoot it myself, but this user has the same problem you're having. You might need to put Python on whatever MSYS thinks is your PATH as well.

[bigjt_2]

Got er! It ended up being pretty simple. Python doesn't automatically set up an Path environment variable when it's installed on Windows, so I did that myself in the Advanced System Settings under System.

I also had an issue with directory/executable naming for no$sns. I left the executable named that when it unzipped, and unzipped everything for no$sns into a directory called no$sns months ago when I started playing around with it. The Make script didn't like the $ character, so I just renamed the directory and executable and all was well.

The ROM has compiled and is now up and running fine. Thanks for taking the time to make and share this Tepples. I've been off and on experimenting with SNES for a few months now and a nice beginner example like this will be very fun/helpful to dig into.

[tepples]

Egg is on my face. I had forgotten to include an essential file. This has been corrected.

[tepples]

So it appears my code is unreadable, based on what koitsu wrote in this post.

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)

"very bad init routine"
Does it write the right values to the right registers? Is accessing them using D at $2100 a deprecated practice?

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".

and a ton of other things. Can you honestly read this given the formatting and almost "inline" comments without any actual structure?

What sort of "structure" should have been used?

[Drew Sebastino]

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"

You know the primary programming language here isn't C, right? (Hell, I don't even know anything except ASM. Everything else might as well be scribbles for all I care.)

I thing one problem is that everything is shoehorned into only a couple of files. This is alright for if you made it and know where everything is, but if other people are looking at it, you might want to have things like the init code separate. You might want to look at Walker on the SNES starterkit. Its very organized and understandable, in my opinion.

[tepples]

I agree that a map of the source code would help. Might it look like the following?

Organization of the program

Include files

• snes.h: Register definitions and some useful macros for the S-CPU
• global.h: S-CPU global variable and function declarations
• spc-ca65.inc: Macro library to produce SPC700 instructions
• spc-65c02.inc: Macro library to use 65C02 syntax with the SPC700

Source code files (65816)

• snesheader.s: Super NES internal header and initialization code
• main.s: Main program
• ppuclear.s: Useful low-level subroutines for interacting with the S-PPU
• blarggapu.s: Send a sound driver to the S-SMP

Source code files (SPC700)

• spcheader.s: Header for the .spc file; unused in .sfc
• spcimage.s: Sound driver

(end)

If too much code is in main.s, tell me what you think should be split and why, and I'll consider it.

[Drew Sebastino]

Yes. I'll look at main.s in a bit and tell you what to split.

[Drew Sebastino]

I stood looking at main.s and Walker.asm, wondering how you could make it better when I just realized the oblivious...

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

;==============================================================================
; main
;==============================================================================

Also, you need to change stuff like this:

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:

Into this:

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:

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?

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. Walker kind of does this, but not quite as bad. I just think the main problem is that it is not a very clear where something ends and something starts. If you want to split the file, you could have all BG mode and BG tile stuff in a different file from the part where the controllers are read to make the character move, so it is a little more clear as to what is going on.

I might try to help you a bit on this latter, (I don't really want to use wla in its current buggy state, but I am.) but for now, you might just want to take a look at walker in the SNES starterkit.

[NovaSquirrel]

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?

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.

[koitsu]

I will talk with tepples later about the "quality" of his code.

For Espozo: I would not bother spending any time analysing this code in comparison to your own. What I have been working on since last night is converting your code to use ca65 (including all necessary .exes, an updated .bat, etc.). I've done most of the work at this point, but as you can see from the other thread, I'm obviously running into problems with ca65 which are certainly user error on my part, and I cannot progress further until I can talk to someone who actually has very in-depth familiarity with ca65.

[thefox]

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.

Looks like there's a bug in that piece of code. beq doneWallCollision never jumps since A was loaded with 28, but the bcc notHitRight below it ends up picking up the slack. On top of that, I think it's a good practice to add a comment when you're purposefully using a conditional branch instruction for an unconditional branch, e.g. something like:

Code: Select all

bne doneWallCollision ; jumps always

...or just use BRA since we're on 65816

[tepples]

Thank you for taking the time to look at my code. Sometimes it's like pulling teeth to get a code review. Please bear with me while I try to understand the importance of each issue you uncovered.

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.)

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 take no arguments. Here's what a doc comment from ppuclear.s looks like:

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)

Also, you need to change stuff like this:

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.

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?

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.

And yes, I discovered the same bug that thefox mentioned (a never taken branch that was intended to be always taken), but my laptop's battery suddenly ran lowbefore I could finish this post about having done so. I had changed a lda #$00 sta to stz because hey, 65816, but I forgot to change the following branch. Here's what it has become:

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:

Would this make more sense if I were to indent the "then clause" stuff?

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:

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.

Current order of subroutine:
Top level functions: nmi_handler, main
"Game logic" subroutine: move_player
Graphics subroutines: draw_player_sprite, draw_bg
Graphics data

I just think the main problem is that it is not a very clear where something ends and something starts.

In my ca65 coding convention, everything from .proc to .endproc can be pulled out and put into another file.

Let me know what other improvements I could add to make this more understandable, and then I can backport them to the NES version.

[Drew Sebastino]

Would this make more sense if I were to indent the "then clause" stuff?

Yes.

Current order of subroutine:Top level functions: nmi_handler, main"Game logic" subroutine: move_playerGraphics subroutines: draw_player_sprite, draw_bgGraphics data

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.

Sorry if I seem a bit like a jerk...

[tepples]

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?

[Drew Sebastino]

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?

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.