• Please review our updated Terms and Rules here

Code review for first 8086 game

fmahnke

New Member
Joined
Feb 26, 2022
Messages
7
Hello, this is my first post on this forum. I'm starting to learn x86 assembly language and just finished my first game in 8086 assembly. It's a simple pong clone for DOS in 40x25 text mode, assembled with the A86 assembler. I would appreciate it if someone would give me a code review so I can improve my understanding of x86. I have a few questions in mind:

- What can I change to make the code more idiomatic, concise, maintainable?
- Are the code and data organized well for performance?
- Am I wasting code memory? What should I do to use fewer instructions?
- Where should I be using more macros?

I'd appreciate any other kind of feedback, too. The code is split into a few files, attached here. To any reviewer, I really appreciate your time in advance.
 

Attachments

  • pong.zip
    6.6 KB · Views: 16

carlos12

Experienced Member
Joined
May 10, 2020
Messages
171
Location
Madrid, Spain
Hi, fmahnke! Welcome to these forums. I'm so glad there's still other people interested on making games for the wonderful, and long forgoten, DOS platform.

First all, I would recommend every DOS developer to read Michael Abrash's Zen of Assemby Language. There is a general consensus this is the bible of 8086/88 assembler programming and optimization. It was generously donated to the public domain some time ago by Abrash himself (I cannot thank him enough).

Zen of Assemby - Epub
Zen of Assembly - mobi (Kindle et al)
Zen of Assembly - html version

I quick reviewed your code. I have no time to do a deep analysis but here come some simple tips, if they can help you:

- I my opinion, code is more clearly read if the different functionalities are separated by carriage returns. Also, separating the operator form the parameters can lead to more clarity. By example:

Code:
  cmp object_position,play_x_min
  jl score_player_1
  cmp object_position,play_x_max
  jle collision_test_top_bottom
score_player_0:
  inc scores
  mov al,0
  call ball_reset_for_serve
  jmp score_wait
score_player_1:

can become:

Code:
    cmp     object_position,play_x_min
    jl      score_player_1
 
    cmp     object_position,play_x_max
    jle     collision_test_top_bottom
 
score_player_0:

     inc     scores
     sub     al,al
     call    ball_reset_for_serve
     jmp     score_wait
 
score_player_1:

- If you want maximum speed, you could change

mov al,0

by

xor al,al

or, even better

sub al,al

It's just faster because it takes less bytes of generated machine code. In a non-time critical part, it has no importance at all. But on a thigh loop like one that paints a sprite, it could give a bit of speed, so necessary.

- All memory references should be bracketed, in order to have more clarity, to clearly differentiate variables from constants and registers.

For example:

mov al,paddle_0_idx

becomes

mov al,[paddle_0_idx]

But I'm not sure A86 accepts this syntax. MASM and TASM, at least no too early version do.

- You could take advantage of the word operations. For example:

Code:
  ; Copy position to old position.
  lodsb
  mov ah,al
  stosb
  lodsb ; ah == pos.x, al == pos.y
  stosb

Could he simplified as:

Code:
    ; Copy position to old position.
    lodsw                                ; Loads AX, AL and AH on one instruction
    xchg        al,ah                    ; Don't know if this is necessary. It depends
                                         ; on what order did you store your data
    stosw                                ; Just two data on one instruction

Even better, depending on how did you organize your data, a single movsw instruction could do the job.

Regarding style, I personally like putting comments aside the code, just like the example above. I also put comments before the instructions. Depending on the code I use one comment style or the other one.

I would also use a 4 spaces tab, instead of 2, for more clarity.

Hope it helps.
 
Last edited:

carlos12

Experienced Member
Joined
May 10, 2020
Messages
171
Location
Madrid, Spain
Another thing that can be done is to rearrange the variable definitions and constant equates. For example:

Code:
data segment
  frame_tick_count db ?    ; Timer ticks until next frame.
  delay dw ?               ; Decremented on each timer tick.
  frame_ready db ?         ; Set when it's time to process a frame.
  scores db 2 dup ?        ; Array of player scores.
  object_position db 2*object_count dup ?     ; Array of position vec2
  object_position_old db 2*object_count dup ? ; Array of old position vec2
  object_velocity db 2*object_count dup ?     ; Array of velocity vec2
  object_size db 2*object_count dup ?         ; Array of size vec2
  object_char_code db object_count dup ?      ; Array of object char codes
  object_moved db object_count dup ?          ; Array of object moved flags
  keys_pressed db 5 dup ? ; w, s, i, k, q     ; Array of input key states

rearranges as:

Code:
data segment

  frame_tick_count      db ?                       ; Timer ticks until next frame
  delay                 dw ?                       ; Decremented on each timer
                                                   ; tick.
  frame_ready           db ?                       ; Set when it's time to
                                                   ; process a frame.
  scores                db 2 dup ?                 ; Array of player scores.
  object_position       db 2*object_count dup ?    ; Array of position vec2
  object_position_old   db 2*object_count dup ?    ; Array of old position vec2
  object_velocity       db 2*object_count dup ?    ; Array of velocity vec2
  object_size           db 2*object_count dup ?    ; Array of size vec2
  object_char_code      db object_count dup ?      ; Array of object char codes
  object_moved          db object_count dup ?      ; Array of object moved flags
  keys_pressed          db 5 dup ?                 ; w, s, i, k, q     
                                                   ; Array of input key states

which is, I believe, easier to read and, if necessary, update.
 

fmahnke

New Member
Joined
Feb 26, 2022
Messages
7
carlos12, thank you very much for your review. Those are all helpful points and I'll take them into consideration. The stosw/lodsw recommendations are helpful. I just didn't think about using them, but they would have made writing those routines less awkward.

Do you have any opinion on the arrangement of this kind of data? I chose this:

object_position db 2*object_count dup ? ; Array of position vec2 object_position_old db 2*object_count dup ? ; Array of old position vec2 object_velocity db 2*object_count dup ? ; Array of velocity vec2 object_size db 2*object_count dup ? ; Array of size vec2

Over this:

object db 8*object_count dup ? ; Array of struct of object position vec2, old position vec2, velocity vec2, size vec2

In other words, the position vectors for all objects are stored linearly before their velocities, and so on, instead of each object's different vectors stored before the next. My impression is the way I did it is more data-oriented and may perform better with large numbers of these objects, but I'm not sure what's best.

I'll definitely read the Abrash manuals in the next days as I work on my next asm program.

Finally, do you recommend any tools? I'm using A86 as assembler, DOS debug.com as debugger, and dosbox for testing. I'm interested in what tools other people use.
 

carlos12

Experienced Member
Joined
May 10, 2020
Messages
171
Location
Madrid, Spain
object_position db 2*object_count dup ? ; Array of position vec2 object_position_old db 2*object_count dup ? ; Array of old position vec2 object_velocity db 2*object_count dup ? ; Array of velocity vec2 object_size db 2*object_count dup ? ; Array of size vec2

Over this:

object db 8*object_count dup ? ; Array of struct of object position vec2, old position vec2, velocity vec2, size vec2
I would have to examine all the code and see the context, so I don't want to emit a strong opinion. Anyway, at first sight, I would prefer the same option you did as it looks more manageable and understandable. I don't think there's any performance advantage on the second one.

Finally, do you recommend any tools? I'm using A86 as assembler, DOS debug.com as debugger, and dosbox for testing. I'm interested in what tools other people use.
While A86 is powerful enough to make such a master piece game as Planet X3, in general I would use a more powerful and standard assembler. A86, for example, only can produce .com files, so if your game needs more than 64kb combining code + static data, it just cannot generate an .exe file that would actually do it. The game I'm developing just couldn't fit on a .com file.

Many people here use some version of Microsoft Masm. Many others use Borland Turbo Assembler. That's my favorite. It's as powerful as MASM, it's 100% compatible with it, and it can use, as the late versions of Masm, simplified segment syntax, just as A86. In addition to this, Tasm also simplifies the stack parameters and local variables so one can use hers/his valuable time doing other things than counting BPs. Many other retro coders use Nasm (Netwide Assembler), Yasm or Wasm, which can be used on modern platforms.

For debugging I use Turbo Debugger, although I must confess that I use more the "poor man's debugger", that is, if I want to watch the variable values, I just printf them (I normally use a mixture of C and assembler).

For text editing, I primary use VS Codium for Windows (it works on all major modern platforms) and Notepad++. Sometimes, but rarely, I run DOS editors from DosBox, such as Turbo C++ or QBasic.

I use DosBox 0.74 for my main work. I also use DosBox-X, which is more recent and has many bug fixes. I also use 86Box, but only for testing from time to time the final executable, as it's very tedious to transfer files to-from it. I also have some real hardware, namely a PS/2 Model 30 8086 and a Turbo XT clone. But I rarely develop there, as they are painfully slow.
 
Last edited:

Krille

Veteran Member
Joined
Aug 14, 2010
Messages
1,029
Location
Sweden
Hello, this is my first post on this forum.
Welcome!

- What can I change to make the code more idiomatic, concise, maintainable?
The code should be easy to read which means that;

1. On a small project like this you should keep everything in a single file.

2. Commenting the code so that, ideally, someone who doesn't even know assembly can still follow along what the code does. I say "ideally" because I rarely do that myself. :)

3. Everything that carlos12 already said, using proper indentation for code and comments, etc.

4. This might sound strange but you should avoid using macros if possible. This applies especially if other people are supposed to read and maintain the code. Not only do macros "redefine" the language (forcing people to constantly look up what they do until it's internalized) but they also add abstraction that make it harder to optimize the code.

5. Name variables so you can infer their size directly by reading the instruction using them.
For example, something like this;
Code:
bFrame_Tick_Count
wDelay

6. Optimize the code. Again, this might sound strange but I often find optimized code to be easier to read, probably because it's more concise.

- Are the code and data organized well for performance?
No. In this case it doesn't really matter, but as an example, you should in general avoid jumps whenever possible - the very first instruction in this program jumps over some data that could have been located at the end after all the code.

- Am I wasting code memory? What should I do to use fewer instructions?
It's hard to give specific advice so...

Find a good instruction set reference and learn everything there is to know about each of the instructions - what they do and how they affect the flags. Using the flags properly is crucial for writing efficient code. As far as 8086/8088 code goes, smaller is always faster (with a few exceptions - namely the multiply and divide instructions) so you should learn the size of the instructions as well.

The very first thing you should learn as a new beginner is the jump instructions, specifically which jcc:s are signed vs unsigned and when to use which.
BTW, my head hurts after reading this;
Code:
  ; If the high bit of the keyboard code is clear ("make"), the key is pressed.
  ; If it is set ("break"), the key is released.
  ; ah = key press state. 0 is released, 1 is pressed.
  mov ah,1
  test al,080h
  jge >L2
  mov ah,0
Using JGE like this is... not common. :)

JGE jumps when the sign flag equals the overflow flag. The TEST instruction (as well as AND, OR and XOR) always clear the overflow flag so this means that JGE will always jump when the sign flag is cleared in the result of the TEST instruction. In other words, I would have used JNS instead.
On a side note, those four instructions can be replaced with this;
Code:
	cbw
	inc		ah

- Where should I be using more macros?
Nowhere, at least in this program. Macros are great for readability when you're using conditional assembly. For example, in XUB we use a lot of conditional assembly to emulate instructions not available on older processors, or to avoid bugs in some processors. I would not use macros for simple instruction sequences like, for example, your dos_return macro.

Finally, seeing this made me dry heave;
Code:
  mov es,0
I assume that's a built-in macro in A86? I wouldn't use macros like that because "mov sreg, imm" is not valid code and it might fool people into thinking it is.
The code for that actually looks like this;
Code:
	push	ax
	mov		ax, 0
	mov		es, ax
	pop		ax
It has to preserve the flags so that's why it does "mov ax, 0" instead of using the more efficient "xor ax, ax" or "sub ax, ax". This is a perfect example of how macros can make code less efficient.
 
Top