CSBwin bug: attack resistances Antifire/Antimagic/Vitality

Discuss Chaos Strikes Back for Windows and Linux, an unofficial port of Chaos Strikes Back to PC by Paul Stevens, as well as CSBuild, an associated dungeon editor.

Moderator: Zyx

Forum rules
Please read the Forum rules and policies before posting.
Post Reply
User avatar
ChristopheF
Encyclopedist
Posts: 1537
Joined: Sun Oct 24, 1999 2:36 pm
Location: France
Contact:

CSBwin bug: attack resistances Antifire/Antimagic/Vitality

Post by ChristopheF »

There have been several discussion threads in the past about Antifire and Antimagic being ineffective because of a bug. Examples:
http://www.dungeon-master.com/forum/vie ... 27&t=27791
http://www.dungeon-master.com/forum/vie ... 46&t=23134

Today I can say I have the final explanation on this question. I am 100% positive that this bug is found in CSBwin only and not in the original game on Atari ST. Sophia identified this bug in the past but it was never fixed in CSBwin (except in a special test version).
The bug lies in the function named TAG016426, as found in the CSBwin 12.3 source code (in Character.cpp).
Paul, when converting the game you were confused by some strange machine code and added this comment in your source code: "They stored a word and fetch a byte!"

Here is a disassembly of the original machine code for this function as found in CSB 2.0 for Atari ST:

Code: Select all

        link	  a6,#-$000A	
        move.w	d7,-(a7)	
        move.w	$000C(a6),d0	
        movea.l	$0008(a6),a0	
        muls	  #$0003,d0	
        lea	    $0047(a0),a0	
        adda.w	d0,a0	
        move.b	(a0),d0	       The byte containing the current value of the statistic (like Antifire or Antimagic) is copied to d0
        move.w	d0,-$000A(a6)	 A word is stored in a local variable, with the high byte not properly initialized. Stange, but in fact this is not an issue, see below.
        move.w	#$00AA,d0	
        clr.w	  d3	           This high byte of d3 is set to 0
        move.b	-$000A(a6),d3	 Only the low byte of the word is used from the local variable, so we don't care what the value of the high byte was
        sub.w	  d3,d0	
        move.w	d0,d7	         d7 now contains 170 - CurrentStatisticValue. There is no division by 256 to be found anywhere.
        cmpi.w	#$0010,d0	
        bge.s	  L16460	
        move.w	$000E(a6),d0	
        lsr.w	  #3,d0	
        bra.s	  L16470	
L16460	move.w	d7,-(a7)	
        move.w	#$0007,-(a7)	
        move.w	$000E(a6),-(a7)	
        jsr	    $0198(a5)
        addq.l	#6,a7	
L16470	move.w	(a7)+,d7	
        unlk	  a6	
        rts		
The strange code was not intended by the FTL developers because in fact, the compiler is responsible for it. I know that because I have studied the behavior of the Megamax C compiler that FTL used to develop the game. In fact, I have reverse engineered that function back to C source code. Here is the function as FTL wrote it in C language (except for the function and variable names):

Code: Select all

unsigned int F307_fzzz_CHAMPION_GetStatisticAdjustedAttack(P642_ps_Champion, P643_ui_StatisticIndex, P644_ui_Attack)
CHAMPION* P642_ps_Champion;
unsigned int P643_ui_StatisticIndex;
unsigned int P644_ui_Attack;
{
        register int L0927_i_Factor;


        if ((L0927_i_Factor = 170 - P642_ps_Champion->Statistics[P643_ui_StatisticIndex][INDEX1_CURRENT]) < 16) {
                return P644_ui_Attack >> 3;
        }
        return F030_aaaW_MAIN_GetScaledProduct(P644_ui_Attack, 7, L0927_i_Factor);
}
If you compile this function with the Megamax compiler, the resulting machine code is exactly the same as the one found in the original game. I did check this.

As you see, there is only one local variable L0927_i_Factor, stored in a register (the compiler actually assigns d7 to store L0927_i_Factor).
The 10 bytes of local variable storage allocated by the instruction "link a6,#-$000A" are for an additional "hidden" variable added automatically by the compiler and in which the program stores a temporary value with instruction "move.w d0,-$000A(a6)".
Sometimes, when evaluating an expression, the compiler needs to store a temporary value somewhere. In this purpose, the compiler creates an additional local variable with a fixed size of 10 bytes (don't ask me why this size), even if the temporary value to store is smaller. That is why the code looks confusing. Note that there are many other functions in the game where such 10 bytes temporary local variables are present, all added by the compiler.

Consequently, the current code in CSBwin is wrong in function TAG016426:

Code: Select all

D7W = sw(170 - w_10/256);
and it should be replaced by this one because there is no division by 256 nor any word to byte conversion in the original code:

Code: Select all

D7W = sw(170 - w_10);
I hope this can convince you to fix this bug.
User avatar
Sophia
Concise and Honest
Posts: 4239
Joined: Thu Sep 12, 2002 9:50 pm
Location: Nowhere in particular
Contact:

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by Sophia »

That's really interesting!

However, it makes me wonder about this code (found in Character.cpp):

Code: Select all

    D0W = sw(STRandom(128) + 10);
    D6W = TAG016426(pcA3, 4, D0W);
    LOCAL_10 = D6W;
    if (D7W > LOCAL_10)
    {
      do
      {
        LOCAL_20 = sw(STRandom(8));
        D1W = sw((1 << LOCAL_20) & mask);
        d.PendingOuches[chIdx] |= D1W;
        D6W <<= 1;
        LOCAL_30 = D6W;
        if (D7W <= LOCAL_30) break;
      } while (D6W != 0);
    };
This is just after damage has been determined (the calls to TAG016426 doing spellshields and fireshields are just above here), so the total amount of damage the character is going to suffer is stored in D7W. D6W (and LOCAL_10) get a random threshold value, between 10 and 137, that then gets put into TAG016426 and compared with the damage. In the loop, it assigns an injury, then doubles the threshold and checks again; the logic behind this, I believe, is so that hits that did more HP damage have a greater chance of causing more injuries, because they will be over the threshold value for more repetitions of the loop.

With the broken version of TAG016426, it doesn't change much. The threshold just ends a random number between 13 and 181. If TAG016426 actually worked, it would scale the threshold by vitality, but it would scale it the wrong way. Just like above where damage is being attenuated by anti-magic or anti-fire, the higher the stat, the lower the number that (a working version of) TAG016426 spits out, so, in this case, the higher your vitality, the lower the threshold, and the more likely you are to suffer an injury.

This seems... not intuitive. Is it a bug in CSBwin or in CSB itself, I wonder?
User avatar
Paul Stevens
CSBwin Guru
Posts: 4318
Joined: Sun Apr 08, 2001 6:00 pm
Location: Madison, Wisconsin, USA

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by Paul Stevens »

Good Afternoon, Christophe,

Good work. Hard work. Certainly, I will change
CSBwin to do the 'Right' thing. I have not studied
your analysis nor Sophia's comments. But......after
this is totally settled and we reach a consensus, I
will release a new CSBwin if it is called for.
User avatar
ChristopheF
Encyclopedist
Posts: 1537
Joined: Sun Oct 24, 1999 2:36 pm
Location: France
Contact:

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by ChristopheF »

Sophia,
I have checked my source code (both assembly and C) and the CSBwin source code. The original game works exactly as translated by Paul in CSBwin.
I agree with your analysis: the higher the vitality, the more likely the champion can get wounded. This looks like a bug, but this one is in the original game.

By the way, LOCAL_10, LOCAL_20 and LOCAL_30 variables in CSBwin source code are other examples of what I called "hidden" local variables automatically added by the Megamax C compiler.
User avatar
Paul Stevens
CSBwin Guru
Posts: 4318
Joined: Sun Apr 08, 2001 6:00 pm
Location: Madison, Wisconsin, USA

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by Paul Stevens »

move.w d0,-$000A(a6)
move.b -$000A(a6),d3 Only the low byte of the word is used from the local variable, so we don't care what the value of the high byte was
Good Evening, Mr. ChristopheF,

Here was my reasoning:
The Atari used a 68000 processor (I have personally
written assemblers and debuggers for this processor
and was quite familiar with it.). It is a big-endian
processor. So, when you:

MOVE.W D0,#30

the more significant byte goes to address 30 and
the least significant byte goes to address 31.
Then when you:

MOVE.B #30,D3

You get the upper byte.

So I had to shift the word at address 30
right by 8 bits to do what the assembly code
did. I remember struggling with this at some
length. It seems clear to me now.

So, your comment that "Only the low byte of the word
is used" should say "Only the high byte of the word is used".

That is how I see it.
User avatar
ChristopheF
Encyclopedist
Posts: 1537
Joined: Sun Oct 24, 1999 2:36 pm
Location: France
Contact:

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by ChristopheF »

Sorry to have been so affirmative. You are quite right in your explanation. :oops:

But I did some additional research, and now I still think you should fix CSBwin as proposed, but this time for a different reason :)

Two facts:
1) What I said about the C source code I provided in my first post stands true: this code is the code that FTL used, because when compiled with the compiler that they used back at the time, I get the exact same binary machine code.
Additional information: I have disassembled all Atari ST versions of both Dungeon Master and Chaos Strikes Back, and this function is exactly the same in all of these versions.
2) What you said about the 68000 processor is correct, and clearly shows that the actual byte that is used in the computation is not the expected one. I have run the game in Steem debugger (ST emulator) and confirm the behavior that you described:
At the beginning of the machine code, d0 is initialized (as a word) with the statistic index that was passed as a parameter. This value can be 0 to 6 (there are 7 statistics per champion). Then it is multiplied by 3 resulting in value 0 to 18. Consequently, the high byte of d0 is always 0 in all calls to this function. This means that the code always computes 170 - 0 = 170, thus completely ignoring the antifire, antimagic or vitality values of the champion. Clearly, this cannot be a behavior intended by the designers, that would make no sense!

Because of these two facts, the only possible conclusion is that this is a bug in the Megamax C compiler itself. The 10 bytes temporary local variable is fully managed by the compiler, so this byte/word mismatch is now clearly a bug on the compiler part. The C code, as written by FTL, is correct, but the resulting machine code is just wrong.

By using "/256" in the CSBwin source code, you reproduce the same buggy behavior: the statistic value is stored in one byte so with a value between 0 and 255. Divided by 256, you always get 0. You could as well replace D7W = sw(170 - w_10/256); by D7W = sw(170);


I had a look at the Amiga version of Dungeon Master 2.0 (that I have also disassembled). It was compiled with a completely different compiler (Aztec C, from Manx company).
Here is the assembly code for the same function:

Code: Select all

            LINK.W    A5,#0
            MOVE.L    D4,-(A7)    Backup D4 to the stack (D4 is used for a local variable in this function)
            MOVE.W    12(A5),D0   12(A5) contains the statistic index, as a function parameter
            MULU      #$0003,D0   3 bytes for each statistic
            MOVEA.L   D0,A0
            ADDA.L    8(A5),A0    8(A5) contains the address of the champion data
            MOVEQ     #0,D0
            MOVE.B    71(A0),D0   This gets the current statistic byte value and "converts" it to a word (high byte set to 0 on previous line)
            MOVE.W    #$00aa,D4
            SUB.W     D0,D4       D4 = 170 - CurrentStatistic
            CMP.W     #$0010,D4
            BGE.S     LAB_0D77    Branch if D4 >= 16
            MOVE.W    14(A5),D0   Attack value
            LSR.W     #3,D0
LAB_0D76:   MOVE.L    (A7)+,D4    Restore D4 from the stack
            UNLK      A5
            RTS
LAB_0D77:   MOVE.W    D4,-(A7)
            MOVE.W    #$0007,-(A7)
            MOVE.W    14(A5),-(A7)
            JSR       -31104(A4)  Call ScaledMultiply function
            ADDQ.W    #6,A7
            BRA.S     LAB_0D76
As you can see, there is no mumbo jumbo with hidden local variables as in the Atari ST version. There is no ambiguity, and this time the correct statistic value is used to compute the difference with 170. Antifire and Antimagic do work fine in Amiga versions of the game.

As a conclusion, I think there is indeed a bug in the original Atari ST version, but it is not a bug by FTL, it is a compiler bug. This bug is not present in other versions of the game because they use other compilers.

You may choose to leave CSBwin as it is in order to be as faithful as possible to the original Atari ST game, or to fix this bug.
As you have already fixed in CSBwin many bugs of the original game, I think this one should be fixed too.
User avatar
ChristopheF
Encyclopedist
Posts: 1537
Joined: Sun Oct 24, 1999 2:36 pm
Location: France
Contact:

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by ChristopheF »

I have checked if this compiler bug has any other impact elsewhere in the Atari ST versions of the game, but this is the only instance. All other hidden local variables (I have identified 48 such variables) are used to store words (2 bytes) or longs (4 bytes) and there are no inconsistency like the one found by Paul (store a word in the variable then read a byte).
User avatar
Paul Stevens
CSBwin Guru
Posts: 4318
Joined: Sun Apr 08, 2001 6:00 pm
Location: Madison, Wisconsin, USA

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by Paul Stevens »

Thank you; thank you very much for your hard
work on this detail. Your machine language
from the Amiga version is definitive. We know
what the authors intended. Moreover, it makes
'sense'.

Look for a new CSBWin as soon as I can produce one.
Your entire report will be included in the code
as a comment.

Again, your research is appreciated. Compiler errors
can be tough nuts to crack.
User avatar
Paul Stevens
CSBwin Guru
Posts: 4318
Joined: Sun Apr 08, 2001 6:00 pm
Location: Madison, Wisconsin, USA

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by Paul Stevens »

CSBWn version 12.6 incorporates this 'fix'.

Totally untested. I did fight my way through CSB's
worm room and worked my way out of the junction.
So it appears to be not totally broken.
User avatar
ChristopheF
Encyclopedist
Posts: 1537
Joined: Sun Oct 24, 1999 2:36 pm
Location: France
Contact:

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by ChristopheF »

Thank YOU for the time you took to listen to me and to fix csbwin.
User avatar
Gambit37
Should eat more pies
Posts: 13714
Joined: Wed May 31, 2000 1:57 pm
Location: Location, Location
Contact:

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by Gambit37 »

I believe that Christophe and Paul may be the two most dedicated software engineers of retro code. Where else would you read a discourse like this, and with such a respectful resolution.

I tip my metaphorical hat to both these fine gents.
User avatar
Paul Stevens
CSBwin Guru
Posts: 4318
Joined: Sun Apr 08, 2001 6:00 pm
Location: Madison, Wisconsin, USA

Re: CSBwin bug: attack resistances Antifire/Antimagic/Vitali

Post by Paul Stevens »

Gambit37 wrote:Where else would you read a discourse like this, and with such a respectful resolution.
Any time you find more than one honest person striving
for a common goal. Where does that leave our US Senate?
Post Reply

Return to “Chaos Strikes Back for Windows & Linux (CSBWin) / CSBuild”