[TI ASM] Problem with shuffling

[kv83]

I was trying to make the routine we spoke of in the General Algorithme thread. Now, when compiling, I get a strange error. Sometimes it crashes, other times it shuffle the cards, but suddenly two of the same cards appear.

Anyone a idea? Here is the code:

Code: Select all

;==========================================================
; Shuffle Routine
; ---------------
; Shuffles hl. Use 255 as end marker.
;==========================================================
lblShuffle:
	ld hl,CardsPlayer
lblShuffleLoop:	
	;--   Copy current card position to de   --
	push hl
	pop de
	;------------------------------------------
	;--Search random position to shuffle with--
	ld b,5
	call ionrandom
	ld b,$00
	ld c,a
	add hl,bc
	;------------------------------------------	
	;--   Shuffle values of both positions   --
	ld b,(hl)
	ld a,(de)
	ld c,a
	ld a,b
	ld (de),a
	ld (hl),c
	;------------------------------------------
	;--         Go to next position          --	
	push de
	pop hl
	inc hl
	ld a,(hl)
	cp 255
	ret z
	jr lblShuffleLoop

Thanks in advance

[tr1p1ea]

Hhhmmm... strange, you could probably get rid of that last push/pop with an 'ex de,hl' but either way, thats not the problem. Are you sure its this particular routines fault?

I like how nice you code is layed out though! .

[Duck]

Hhhmmm... strange, you could probably get rid of that last push/pop with an 'ex de,hl'

Nope, that will echange the values, whereas you want to keep the value in hl.
But this is faster then push/pop:

Code: Select all

   ld d,h
   ld e,l

And, (this is causing the failure I think), remove the push/pop in the end:

Code: Select all

   push de      ;<
   pop hl      ;< should be removed

because HL is the position in the list you want to keep, and DE is just temporary.

[coelurus]

Let's see if I remember those things...

Are you certain this is where things crash? How do you generate cards?

Other than that, it looks fine. The "push de \ pop hl" lines should be ok, because it seems that you use "hl" to both step through all cards one by one and to calculate a new random location. "de" is a temp container alright and must be put into hl in the end of the loop, if I'm not totally off track :p

[Jim e]

255 gets shuffled with the other cards, have your first byte be the length and loop that many times.
also ex de,hl at the end would be faster and smaller, but then agian your probably just going for a working version first. Personally I don't like Ionrandom it's alittle slow for my taste.

And almost foregot to mention that you are also overwriting things after the end mark. My suggestion is to have de move through each card and have hl contain a random offset within the cards length. then just add hl,de and swap the contents.

[CoBB]

Exchanging (hl) and (de) can be optimised a bit, too:

Code: Select all

   ld b,(hl) 
   ld a,(de)
   ex de,hl 
   ld (hl),b 
   ld (de),a

This also takes care of loading DE into HL at the same time.

[Duck]

Well i also thought of that, but I forgot that DE would be the original value of HL so that's the one you'll want to return to HL anyway.

[kv83]

Hmmm... I am not overwriting anything, and don't shuffle with the end mark, why? Because of this:

Code: Select all

ld b,5

There are not more than 5 cards in the array. (Though you were particular right, cause I needed an additional 'dec a' after the ionrandom). Speed isn't really an issue in a card game, if you ask me btw.

It's still not working, and the reason that I think that this routine is giving the problems is because if I don't use it (unshuffled cards) it just works fine...

The cards are not really generated, just loaded into an array located in saferam1.

Thanks for the optimization btw, but it's still broken

[tr1p1ea]

Only 5 cards? Why not just use djnz? As i think its still possible for it to shuffle the marker because if you are at the 5th card and ionRandom gives you 1 then it will shuffle with the marker. Then when it increases the marker wont be there anymore and it will also keep going until it finds a 255 in memory ... which could cause crashes. Or did you say this wasnt possible?

[kv83]

oh darn!!!!! you are right, how could I be so stupid... it should be 5+the hl value at the beginning, not the one I increase every time! tr1p, you are a genius... thanks to everyone else also btw

Edit: It works now

[Kozak]

tr1p, you are a genius...

|| + && you are a du.....

[tr1p1ea]

Im pretty sure Jim e is the one who picked it. Glad to see its working .

[kv83]

Im pretty sure Jim e is the one who picked it. Glad to see its working .

I know, I know... many thanks to him aswell, but you are the one who triggered it by me

[CalcKing]

If you want to decrease the size even more, you can replace

Code: Select all

   cp 255
   ret z 

with

Code: Select all

   inc a
   ret z

Optimizing code is fun...

[Jim e]

I know, I know... many thanks to him aswell, but you are the one who triggered it by me

Yeah, I guess I need to work on my explaining skills, I didn't even site code, just told you what to try. Well the point is it works now, But I'm curious, what kind of Card game(If a game at all) only has 5 cards.

Just as a side note, I honestly have to say debugging and optimisation is my favorite part of coding, not kidding, anyone else with me on this or am I just too nerdy.