Following the successful review of my implementation of Bresenham's Line Algorithm, I've been asked to upload the full implementation of my project which creates a small animation of shapes moving around the screen and changing colours for a full review.
Note that this code hasn't been edited to incorporate the comments made in the last review and as such I don't expect the line algorithm to be reviewed again. (Because of this it still lacks comments and so will still be difficult to review).
Here's a screenshot of the working program, and yes, it looks terrible when a screenshot is taken partway through the drawing.
;=========================================================================================================
; MACROS
;=========================================================================================================
;=========================================================================================================
; BRESENHAM LINE ALGORITHM (lx1,ly1)-(lx2,ly2)
;=========================================================================================================
line macro lx1, ly1, lx2, ly2
local ldxsetup1, ldxsetup2, ldysetup1, ldysetup2
local lxisetup1, lxisetup2, lxisetupexit, lyisetup1, lyisetup2, lyisetupexit
local numsetup1, numsetup2, numsetupexit
local lloopstart, lloopif, lloopifexit, lloopend
pushall
mov ax, lx2
sub ax, lx1
cmp ax, 0
jge ldxsetup2
ldxsetup1:
mov bx, -1
mul bx
ldxsetup2:
mov ldx, ax
mov ax, ly2
sub ax, ly1
cmp ax, 0
jge ldysetup2
ldysetup1:
mov bx, -1
mul bx
ldysetup2:
mov ldy, ax
mov ax, lx1
mov lx, ax
mov ax, ly1
mov ly, ax
mov ax, lx2
cmp ax, lx1
jge lxisetup1
jmp lxisetup2
lxisetup1:
mov ax, 1
jmp lxisetupexit
lxisetup2:
mov ax, -1
lxisetupexit:
mov lxi1, ax
mov lxi2, ax
mov ax, ly2
cmp ax, ly1
jge lyisetup1
jmp lyisetup2
lyisetup1:
mov ax, 1
jmp lyisetupexit
lyisetup2:
mov ax, -1
lyisetupexit:
mov lyi1, ax
mov lyi2, ax
mov ax, ldx
mov bx, ldy
cmp ax, bx
jge numsetup1
jmp numsetup2
numsetup1:
mov ax, 0
mov lxi1, ax
mov lyi2, ax
mov ax, ldx
mov lden, ax
mov lnumpix, ax
shr ax, 1
mov lnum, ax
mov ax, ldy
mov lnumadd, ax
jmp numsetupexit
numsetup2:
mov ax, 0
mov lxi2, ax
mov lyi1, ax
mov ax, ldy
mov lden, ax
mov lnumpix, ax
shr ax, 1
mov lnum, ax
mov ax, ldx
mov lnumadd, ax
numsetupexit:
mov ax, lnum
mov dx, 0
lloopstart:
cmp dx, lnumpix
jg lloopend
plot lx, ly
add ax, lnumadd
cmp ax, lden
jge lloopif
jmp lloopifexit
lloopif:
sub ax, lden
mov bx, lx
add bx, lxi1
mov lx, bx
mov bx, ly
add bx, lyi1
mov ly, bx
lloopifexit:
mov bx, lx
add bx, lxi2
mov lx, bx
mov bx, ly
add bx, lyi2
mov ly, bx
inc dx
jmp lloopstart
lloopend:
popall
endm
;=========================================================================================================
;=========================================================================================================
; DRAW TRIANGLE BOTTOM LEFT POINT AT (tx,ty) OF HEIGHT th AND WIDTH tw
;=========================================================================================================
triangle macro tx, ty, tw, th
pushall
mov ax, tx
add ax, tw
mov tx2,ax
mov bx, tw
shr bx, 1
add bx, tx
mov tx3, bx
mov cx, ty
sub cx, th
mov ty3, cx
line tx, ty, tx2, ty
line tx2, ty, tx3, ty3
line tx3, ty3, tx, ty
popall
endm
;=========================================================================================================
;=========================================================================================================
; DRAW SHAPE
;=========================================================================================================
draw macro
pushall
mov ax, currentshape
cmp currentshape, 1
je draw1
jmp draw2
draw1:
mov ax, 0
add ax, currentx
mov val1, ax
mov ax, 50
add ax, currenty
mov val2, ax
mov ax, 50
mov val3, ax
mov ax, 43
mov val4, ax
triangle val1,val2,val3,val4
mov ax, 25
add ax, currentx
mov val1, ax
mov ax, 93
add ax, currenty
mov val2, ax
triangle val1,val2,val3,val4
mov ax, 50
add ax, currentx
mov val1, ax
mov ax, 50
add ax, currenty
mov val2, ax
triangle val1,val2,val3,val4
mov ax, 25
add ax, currentx
mov val1, ax
mov ax, 7
add ax, currenty
mov val2, ax
mov ax, -43
mov val4, ax
triangle val1,val2,val3,val4
mov ax, 0
add ax, currentx
mov val1, ax
mov ax, 50
add ax, currenty
mov val2, ax
mov ax, -43
mov val4, ax
triangle val1,val2,val3,val4
mov ax, 50
add ax, currentx
mov val1, ax
mov ax, 50
add ax, currenty
mov val2, ax
mov ax, -43
mov val4, ax
triangle val1,val2,val3,val4
jmp drawend
draw2:
mov ax, 50
add ax, currentx
mov val1, ax
mov ax, 0
add ax, currenty
mov val2, ax
mov ax, 68
add ax, currentx
mov val3, ax
mov ax, 3
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 79
add ax, currentx
mov val3, ax
mov ax, 9
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 88
add ax, currentx
mov val3, ax
mov ax, 18
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 94
add ax, currentx
mov val3, ax
mov ax, 26
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 98
add ax, currentx
mov val3, ax
mov ax, 38
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 100
add ax, currentx
mov val3, ax
mov ax, 50
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 98
add ax, currentx
mov val3, ax
mov ax, 62
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 94
add ax, currentx
mov val3, ax
mov ax, 74
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 88
add ax, currentx
mov val3, ax
mov ax, 82
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 79
add ax, currentx
mov val3, ax
mov ax, 91
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 68
add ax, currentx
mov val3, ax
mov ax, 97
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 50
add ax, currentx
mov val3, ax
mov ax, 100
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 32
add ax, currentx
mov val3, ax
mov ax, 97
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 21
add ax, currentx
mov val3, ax
mov ax, 91
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 12
add ax, currentx
mov val3, ax
mov ax, 82
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 6
add ax, currentx
mov val3, ax
mov ax, 74
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 2
add ax, currentx
mov val3, ax
mov ax, 62
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 0
add ax, currentx
mov val3, ax
mov ax, 50
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 2
add ax, currentx
mov val3, ax
mov ax, 38
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 6
add ax, currentx
mov val3, ax
mov ax, 26
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 12
add ax, currentx
mov val3, ax
mov ax, 18
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 21
add ax, currentx
mov val3, ax
mov ax, 9
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 32
add ax, currentx
mov val3, ax
mov ax, 3
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
mov ax, val3
mov val1, ax
mov ax, val4
mov val2, ax
mov ax, 50
add ax, currentx
mov val3, ax
mov ax, 0
add ax, currenty
mov val4, ax
line val1,val2,val3,val4
drawend:
popall
endm
;=========================================================================================================
;=========================================================================================================
; PLOT POINT (px,py)
;=========================================================================================================
plot macro px, py
pushall
mov ax, py
mov bx, 320
mul bx
add ax, px
mov di, ax
mov al, colour
mov es:[di],al
popall
endm
;=========================================================================================================
;=========================================================================================================
; CLEAR SCREEN
;=========================================================================================================
cls macro
pushall
mov ah, 07h
mov al, 00h
mov bh, 00h
mov cx, 00h
mov dx, 1827h
int 10h
popall
endm
;=========================================================================================================
;=========================================================================================================
; PUSH ALL DATA TO STACK
;=========================================================================================================
pushall macro
push ax
push bx
push cx
push dx
endm
;=========================================================================================================
;=========================================================================================================
; POP ALL DATA FROM STACK
;=========================================================================================================
popall macro
pop dx
pop cx
pop bx
pop ax
endm
;=========================================================================================================
;=========================================================================================================
;=========================================================================================================
; SETUP
;=========================================================================================================
.model small
.stack 256
;=========================================================================================================
;=========================================================================================================
; VARIABLES
;=========================================================================================================
.data
;draw line variables
ldx dw 0
ldy dw 0
lx dw 0
ly dw 0
lxi1 dw 0
lxi2 dw 0
lyi1 dw 0
lyi2 dw 0
lden dw 0
lnum dw 0
lnumadd dw 0
lnumpix dw 0
;draw triangle variables
tx2 dw 0
tx3 dw 0
ty3 dw 0
;main program variables
startaddr dw 0a000h
colour db 4
currentx dw 0
currenty dw 0
currentshape dw 0
changeshape dw 0
val1 dw 0
val2 dw 0
val3 dw 0
val4 dw 0
dir dw 0
;=========================================================================================================
;=========================================================================================================
; PROGRAM START
;=========================================================================================================
.code
start:
mov ax, @data
mov ds, ax
mov ah, 00h
mov al, 13h
int 10h
mov es, startaddr
;=========================================================================================================
;=========================================================================================================
; MAIN PROGRAM
;=========================================================================================================
mov ax, 10
mov currentx, ax
mov ax, 10
mov currenty, ax
mov ax, 2
mov currentshape, ax
mov ax, 0
mov changeshape, ax
mainloop:
mov ax, dir
cmp ax, 1
je dircheck1
cmp ax, 2
je dircheck2
cmp ax,3
je dircheck3
jmp dircheck4
dircheck1:
mov ax, 1
add currentx, ax
mov ax, currentx
cmp ax, 210
jle dircheckend
mov ax, 2
mov dir, ax
mov al, colour
inc al
mov colour, al
jmp dircheckend
dircheck2:
mov ax, 1
add currenty, ax
mov ax, currenty
cmp ax, 90
jle dircheckend
mov ax, 3
mov dir, ax
mov al, colour
inc al
mov colour, al
mov ax, 1
mov changeshape, ax
jmp dircheckend
dircheck3:
mov ax, -1
add currentx, ax
mov ax, currentx
cmp ax, 10
jge dircheckend
mov ax, 4
mov dir, ax
mov al, colour
inc al
mov colour, al
jmp dircheckend
dircheck4:
mov ax, -1
add currenty, ax
mov ax, currenty
cmp ax, 10
jge dircheckend
mov ax, 1
mov dir, ax
mov al, colour
inc al
mov colour, al
mov ax, 1
mov changeshape, ax
jmp dircheckend
dircheckend:
mov ax, changeshape
cmp ax, 1
jne shapechangeend
mov ax, 0
mov changeshape, ax
mov ax, currentshape
inc ax
mov currentshape, ax
cmp ax, 3
jne shapechangeend
mov ax, 1
mov currentshape, ax
shapechangeend:
cls
draw
mov ah, 01h
int 16h
jnz finish
jmp mainloop
;=========================================================================================================
;=========================================================================================================
; PROGRAM END
;=========================================================================================================
finish:
mov ah, 00h
mov al, 03h
int 10h
mov ah,04ch
mov al,00h
int 21h
end start
;=========================================================================================================
-
1\$\begingroup\$ There seems to be a fair amount of duplicate code, have you tried reducing it? \$\endgroup\$D. Jurcau– D. Jurcau2017年03月05日 17:01:01 +00:00Commented Mar 5, 2017 at 17:01
-
\$\begingroup\$ @D.Jurcau no, this is an old project that I worked on and am interested in touching up following a full review. \$\endgroup\$user122352– user1223522017年03月05日 17:36:59 +00:00Commented Mar 5, 2017 at 17:36
1 Answer 1
Analysis (in random order)
mov ah, 00h mov al, 03h int 10h mov ah,04ch mov al,00h int 21h
We always strive to get the smallest code without loosing readability. I think next snippet does a good job (it's 2 bytes smaller and well commented):
mov ax, 0003h ;AH=00h BIOS.SetVideo, AL=3 Textmode 80x25
int 10h
mov ax, 4C00h ;AH=4Ch DOS.Terminate, AL=0 ExitCode
int 21h
mov ax, 10 mov currentx, ax mov ax, 10 mov currenty, ax mov ax, 2 mov currentshape, ax mov ax, 0 mov changeshape, ax
It's perfectly possible to move the numbers in the variables without putting them in a register first. But in the case where the same number goes into more than 1 variable using especially the AX
register is advantageous as it reduces code size.
mov ax, 10
mov currentx, ax
mov currenty, ax
mov currentshape, 2
mov changeshape, 0
mov ax, 1 add currentx, ax mov ax, currentx
The 3rd instruction here tells me that the value 1 that you moved into the AX
register isn't all that important after the addition, so why not just increment the currentx variable?
inc currentx
mov ax, currentx
mov al, colour inc al mov colour, al
Again a perfect occasion to use less instructions and just increment the variable in one go:
inc colour
pushall mov ax, py mov bx, 320 mul bx add ax, px mov di, ax mov al, colour mov es:[di],al popall
The plot macro code can be a bit smaller and use a register less if you wrote:
pushall
mov ax, 320
mul py <-- No need to use BX here
add ax, px
mov bx, ax
mov al, colour
mov es:[bx], al
popall
Note that I didn't use the DI
register like you did. Because the pushall and popall macros don't actually preserve it, this could easily become a death-trap!
Although using macros to push/pop registers is easy, it also brings about pushing/popping too much, like in this code the CX
register that isn't used at all.
pushall mov ax, tx add ax, tw mov tx2,ax mov bx, tw shr bx, 1 add bx, tx mov tx3, bx mov cx, ty sub cx, th mov ty3, cx line tx, ty, tx2, ty line tx2, ty, tx3, ty3 line tx3, ty3, tx, ty popall
I'll use the triangle code to show many opportunities to shorten the code. mov bx, tw
was moved to the top to avoid reading the tw variable twice.
push ax <-- Shorter than the pushall code
push bx
mov bx, tw
mov ax, tx
add ax, bx <-- Shorter between 2 registers
mov tx2, ax
shr bx, 1
add bx, tx
mov tx3, bx
mov ax, ty <-- Shorter using AX instead of CX
sub ax, th
mov ty3, ax <-- Shorter using AX instead of CX
line tx, ty, tx2, ty
line tx2, ty, tx3, ty3
line tx3, ty3, tx, ty
pop bx
pop ax <-- Shorter than the popall code
Similar code to the next one is often repeated in the draw macro.
mov ax, 0 add ax, currentx mov val1, ax mov ax, 50 add ax, currenty mov val2, ax mov ax, 50 mov val3, ax mov ax, 43 mov val4, ax
Why not simple mov
the variable in the register?
mov ax, currentx
mov val1, ax
Changing the order gives shorter code:
mov ax, currenty
add ax, 50
mov val2, ax
Move the number in one go:
mov val3, 50
mov val4, 43
Conclusion
As you can see it's possible to optimize this code a lot, but I think the best advice I can offer is that all these macros really should have been written as subroutines. In this form the program is simply too long for what it does.
I tried to not repeat comments that I made on the previous (shorter) review. Obviously they still count.
I'm pretty sure that if all these modifications were applied correctly, and thus shortening the program considerably, more advanced tips (especially on program flow) could be given.
-
1\$\begingroup\$ "...all these macros really should have been written as subroutines..." Beware that this cannot apply to the PUSHALL/POPALL macros! Either keep these or just write the required pushes/pops individually like @SepRoland did in the TRIANGLE code. \$\endgroup\$Fifoernik– Fifoernik2017年03月11日 14:50:50 +00:00Commented Mar 11, 2017 at 14:50