7
\$\begingroup\$

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.

enter image description here

;=========================================================================================================
;                       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
;=========================================================================================================
\$\endgroup\$
  • 1
    \$\begingroup\$ There seems to be a fair amount of duplicate code, have you tried reducing it? \$\endgroup\$ – D. Jurcau Mar 5 '17 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\$ – Nick A Mar 5 '17 at 17:36
5
\$\begingroup\$

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.

\$\endgroup\$
  • 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 Mar 11 '17 at 14:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.