Page 1 of 1
Posted: 02 Feb 2010, 12:20
jigebren
This is following a discussion from the topic:
Differences Between Patch 0916 And 1207?
As the bug is now identified, I think it deserves now his own topic (and version 0916 and 1207 both have the bug).
Arto has reported that revolt use to crash when loading 'nhood1_battle' or 'Muse_Bat' (battle tag levels). This fortunately happens only when the compatibility mode is deactivated, but I tried to understand the reason why of this bug.
Notice: compatibility mode is working good, but there could be some reasons we don't want to use it (for exemple, being able to use high resolution).
I now have clearly located the bug, that's a good start. But for a strange reason, the bug doesn't occur under my debugger, and that really doesn't make things easy. I have to find information in the windows error report (which badly seems to be designed for microsoft only, not for the user, we can't even copy the informations).
So, I believe
this bug is linked to the fact that no AI nodes are defined for these levels.
It seems that it lacks an initialization of some variables, and revolt try to use values that have no meaning, thus creating an 'Access violation'.
Just something that I don't understand is why 'bot_bat' or 'markar' don't crash.
And I was able to avois the bug by skipping some parts of the code related to AI node. Of course, it's not a solution, as it deactivates the AI in other race modes.
Can somebody (arto, or anyone else) help me to understand some C++ stuff in the source code?
Posted: 02 Feb 2010, 23:25
arto
jigebren @ Feb 2 2010, 07:50 AM wrote: Can somebody (arto, or anyone else) help me to understand some C++ stuff in the source code?
Yes, just let me know what part.
Posted: 02 Feb 2010, 23:47
jigebren
Ok, thanks arto.
I was trying to understand this part, in 'ainode.cpp'
Code: Select all
/********************************************************************************
* AINODE *AIN_FindNodePlayerIsIn(PLAYER *pPlayer)
*
* Finds which node the player is in. This function assumes that the player is
* inside a valid Zone & checks the nodes inside it.
********************************************************************************/
AINODE *AIN_FindNodePlayerIsIn(PLAYER *pPlayer)
{
AIZONE *pZone;
AINODE *pNode;
int i;
if (pPlayer->CarAI.ZoneID >= 0)
{
if (pZone = &AiZones[pPlayer->CarAI.ZoneID])
{
pNode = pZone->FirstNode;
for (i = 0; i < pZone->Count; i++, pNode = pNode->ZoneNext)
{
if (AIN_IsPointInNodeBounds(&pPlayer->car.Body->Centre.Pos, pNode, -1))
return pNode;
}
}
}
#if 0
pNode = AiNode;
for (i = 0; i < AiNodeNum; i++, pNode++)
{
if (AIN_IsPointInNodeBounds(&pPlayer->car.Body->Centre.Pos, pNode))
return pNode;
}
#endif
return NULL;
}
But now, I think it is quite clear for me (I have documented myself a bit about c++ before).
I was forgetting that, in the line:
Code: Select all
if (pZone = &AiZones[pPlayer->CarAI.ZoneID])
the '=' is an assignement, not an equality test.
And I was wondering about the goal of 'pNode = pNode->ZoneNext' in the
for loop.
Can you just clarify what the '*' before '*AIN_FindNodePlayerIsIn' stands for. Has it something to do with pointers?
Posted: 03 Feb 2010, 00:05
KDL
if (pZone = &AiZones[pPlayer->CarAI.ZoneID])
means:
if "pZone = &AiZones[pPlayer->CarAI.ZoneID]" succeeded then read what in singleton
pZone: present/current zone
AiZones: array for the ai zones (read from the binary)
hopes this would help you
Posted: 03 Feb 2010, 00:54
urnemanden
Looking at it, it doesn't seem so hard to read the cpp files, but I guess that all the english I see is actually ID's refering to other values elsewhere?
Posted: 03 Feb 2010, 10:08
arto
I was forgetting that, in the line:
Code: Select all
if (pZone = &AiZones[pPlayer->CarAI.ZoneID])
the '=' is an assignement, not an equality test.
Yes. Basically it assigns to pZone a pointer to an item in AiZones array. And if the result is null, it won't go inside the if.
And I was wondering about the goal of 'pNode = pNode->ZoneNext' in the for loop.
That for loop iterates over all the zones. The AiZones seem to be a linked list of nodes. So pNode = pNode->ZoneNext just reassigns pNode to point to the next node item. What throws me off is that it is named "ZoneNext" and not "NodeNext", but maybe that's a mistake.
Can you just clarify what the '*' before '*AIN_FindNodePlayerIsIn' stands for. Has it something to do with pointers?
Yes, it means the function returns a pointer to AINODE.
Posted: 03 Feb 2010, 11:40
jigebren
Good news.
I think I said it this every time (but this time, it's even more true than before
), I really thought I will never correct this bug! Well, is seems that I was wrong. I found the cause an a way to correct it. But it tooks me a lot of time...
I can now launch 'nhood1_battle' or 'Muse_Bat' without without compatibility mode set, and without bug.
I haven't tested it a lot yet, so I hope it won't have bad side effect, which I can't promise for now.
Not sure, but I believe it can be an error from the developper, not an incompatibility with more modern OS like XP, so it's could just be only by chance that revolt doesn't crash before.
---
urnemanden wrote:Looking at it, it doesn't seem so hard to read the cpp files, but I guess that all the english I see is actually ID's refering to other values elsewhere?
Do you want to see the assembly version of this function? It looks a bit more funny...
@arto
Well, for 'pNode = pNode->ZoneNext', it's not a mistake, I think it could be traduced by: the current pNode will become the pNode in the Next Zone.
Posted: 03 Feb 2010, 14:55
urnemanden
Hey, that's great! Need any help testing things out, just ask me!
Jig wrote:Do you want to see the assembly version of this function? It looks a bit more funny...
Sure, I hope it isn't harder to understand than the code above. =P
Posted: 03 Feb 2010, 20:20
jigebren
urnemanden @ Feb 3 2010, 10:25 AM wrote:Jig wrote:Do you want to see the assembly version of this function? It looks a bit more funny...
Sure, I hope it isn't harder to understand than the code above. =P
Just for fun, here is the ASM version, sorry for the lenght:
Code: Select all
0040B9F0 /$ 8B4424 04 MOV EAX,DWORD PTR SS:[ESP+4]
0040B9F4 |. 83EC 08 SUB ESP,8
0040B9F7 |. 8B88 40400000 MOV ECX,DWORD PTR DS:[EAX+4040]
0040B9FD |. 53 PUSH EBX
0040B9FE |. 55 PUSH EBP
0040B9FF |. 56 PUSH ESI
0040BA00 |. 85C9 TEST ECX,ECX
0040BA02 |. 57 PUSH EDI
0040BA03 |. 0F8C A7000000 JL revolt_d.0040BAB0
0040BA09 |. 8B0D 04BB4C00 MOV ECX,DWORD PTR DS:[4CBB04]
0040BA0F |. A1 FCBA4C00 MOV EAX,DWORD PTR DS:[4CBAFC]
0040BA14 |. BA 848D4B00 MOV EDX,revolt_d.004B8D84
0040BA19 |. 895424 14 MOV DWORD PTR SS:[ESP+14],EDX
0040BA1D |> 8B7424 1C /MOV ESI,DWORD PTR SS:[ESP+1C]
0040BA21 |. 8B2A |MOV EBP,DWORD PTR DS:[EDX]
0040BA23 |. 03AE 40400000 |ADD EBP,DWORD PTR DS:[ESI+4040]
0040BA29 |. 79 02 |JNS SHORT revolt_d.0040BA2D
0040BA2B |. 03E9 |ADD EBP,ECX
0040BA2D |> 3BE9 |CMP EBP,ECX
0040BA2F |. 7C 02 |JL SHORT revolt_d.0040BA33
0040BA31 |. 2BE9 |SUB EBP,ECX
0040BA33 |> 8B34E8 |MOV ESI,DWORD PTR DS:[EAX+EBP*8]
0040BA36 |. 8B7CE8 04 |MOV EDI,DWORD PTR DS:[EAX+EBP*8+4]
0040BA3A |. 85F6 |TEST ESI,ESI
0040BA3C |. C74424 10 00000000 |MOV DWORD PTR SS:[ESP+10],0
0040BA44 |. 7E 57 |JLE SHORT revolt_d.0040BA9D
0040BA46 |. 83C7 68 |ADD EDI,68
0040BA49 |> 8B17 |/MOV EDX,DWORD PTR DS:[EDI]
0040BA4B |. 8B77 04 ||MOV ESI,DWORD PTR DS:[EDI+4]
0040BA4E |. 33DB ||XOR EBX,EBX
0040BA50 |. 85D2 ||TEST EDX,EDX
0040BA52 |. 7E 32 ||JLE SHORT revolt_d.0040BA86
0040BA54 |> 8B4C24 1C ||/MOV ECX,DWORD PTR SS:[ESP+1C]
0040BA58 |. 6A FF |||PUSH -1
0040BA5A |. 56 |||PUSH ESI
0040BA5B |. 8B91 9C030000 |||MOV EDX,DWORD PTR DS:[ECX+39C]
0040BA61 |. 83C2 14 |||ADD EDX,14
0040BA64 |. 52 |||PUSH EDX
0040BA65 |. E8 66000000 |||CALL revolt_d.0040BAD0
0040BA6A |. 83C4 0C |||ADD ESP,0C
0040BA6D |. 85C0 |||TEST EAX,EAX
0040BA6F |. 75 49 |||JNZ SHORT revolt_d.0040BABA
0040BA71 |. 8B07 |||MOV EAX,DWORD PTR DS:[EDI]
0040BA73 |. 8B76 74 |||MOV ESI,DWORD PTR DS:[ESI+74]
0040BA76 |. 43 |||INC EBX
0040BA77 |. 3BD8 |||CMP EBX,EAX
0040BA79 |.^ 7C D9 ||\JL SHORT revolt_d.0040BA54
0040BA7B |. 8B0D 04BB4C00 ||MOV ECX,DWORD PTR DS:[4CBB04]
0040BA81 |. A1 FCBA4C00 ||MOV EAX,DWORD PTR DS:[4CBAFC]
0040BA86 |> 8B5424 10 ||MOV EDX,DWORD PTR SS:[ESP+10]
0040BA8A |. 8B34E8 ||MOV ESI,DWORD PTR DS:[EAX+EBP*8]
0040BA8D |. 42 ||INC EDX
0040BA8E |. 83C7 70 ||ADD EDI,70
0040BA91 |. 3BD6 ||CMP EDX,ESI
0040BA93 |. 895424 10 ||MOV DWORD PTR SS:[ESP+10],EDX
0040BA97 |.^ 7C B0 |\JL SHORT revolt_d.0040BA49
0040BA99 |. 8B5424 14 |MOV EDX,DWORD PTR SS:[ESP+14]
0040BA9D |> 83C2 04 |ADD EDX,4
0040BAA0 |. 81FA 908D4B00 |CMP EDX,revolt_d.004B8D90
0040BAA6 |. 895424 14 |MOV DWORD PTR SS:[ESP+14],EDX
0040BAAA |.^ 0F8C 6DFFFFFF \JL revolt_d.0040BA1D
0040BAB0 |> 5F POP EDI
0040BAB1 |. 5E POP ESI
0040BAB2 |. 5D POP EBP
0040BAB3 |. 33C0 XOR EAX,EAX
0040BAB5 |. 5B POP EBX
0040BAB6 |. 83C4 08 ADD ESP,8
0040BAB9 |. C3 RETN
0040BABA |> 8BC6 MOV EAX,ESI
0040BABC |. 5F POP EDI
0040BABD |. 5E POP ESI
0040BABE |. 5D POP EBP
0040BABF |. 5B POP EBX
0040BAC0 |. 83C4 08 ADD ESP,8
0040BAC3 \. C3 RETN
I think you'll now understand why it is a hard job
, given that I have no specific ASM knowledge.
I will do some more tests, if it's working, I may send to some of you the patched file for further testing.
Posted: 04 Feb 2010, 02:13
arto
jigebren @ Feb 3 2010, 07:10 AM wrote: @arto
Well, for 'pNode = pNode->ZoneNext', it's not a mistake, I think it could be traduced by: the current pNode will become the pNode in the Next Zone.
Maybe. But that's at odds with the comment in the function. And doesn't seem to make sense, since what use would it be to travel the first node of each zone. In contrast to travelling every node in the zone the player is in. And the for loop counter seems to limit the iterations by pZone->Count, which by common sense would be the number of nodes in the zone.
But that's just speculation, obviously I haven't really looked what that NextZone thing is nor do I understand what Zone and Node really are. In fact the commented out code in the #if 0 makes more sense to me, so probably the code is as it is for a good reason.
Posted: 04 Feb 2010, 05:41
jigebren
Well, arto, I PMed you a file. Haven't you received it?
I'm sorry I don't know your level of knownledge, and I don't know if you have taken a look at the sources, but do you (or someone else) think it's normal that revolt uses 100% of the CPU (at least on my system)? Even when V-Sync is set.
I've always been amazed that as long as computers get more and more powerful, revolt still eat all the CPU.
Posted: 04 Feb 2010, 06:45
arto
I tried your patched version and the battle tag levels work without crashing! Great work! I can try using this patched version when I'm racing online to see if there's any other problems (caveat: I don't race offline, unless there's something that needs to be testing. Also I don't have the 512 patch applied, can that can cause problems? Caveat2: I almost never race battle tag online, and it seems nowadays a rare form of race online anyway).
Regarding the CPU usage... I'm not sure if that's normal or not. Games do funny things and are resource hungry. If Re-Volt is constantly polling for something, like key presses, it will consume whatever is available. On my system it consumes 25%, as can be expected on a quad-core CPU. I haven't checked the sources if there's any obvious places where it does that. It may be that the directX5 or was it directx6 version that Re-Volt uses doesn't provide event based API for keypresses or other input Re-Volt needs - this is again just guessing, I haven't checked what could be the exact reason Re-Volt consumes the CPU for in such aggressive manner.
Posted: 04 Feb 2010, 07:12
jigebren
arto @ Feb 4 2010, 02:15 AM wrote:I tried your patched version and the battle tag levels work without crashing! Great work!
Glad to hear that!
I think from the way I corrected the issue that if you see the difference offline, it is likely to work online too, but let's wait to be sure. And I understand that race battle tag are not very usual online.
For the 512 patch, the file I gave you is already patched, so if your texture files are still 256x256, I think the game will just look more pixellated. (It would be a lot worse if you try to use 512x512 textures in a non patched version).
About the CPU usage, the 25% consumption on quadricore is funny, though as you said, not very surprising. Just for fun, during levels loading, this value could increase to 50% or more, as other thread(s) is/are launched.
In fact, there is a lot of
while loops used in revolt to make pauses, without any delay inside the loop, which is a nonsense AFAIK. So I think revolt is looping as far as possible inside these empty loops until the end condition is reached, and consumes all CPU power just for that.
Posted: 04 Feb 2010, 20:57
urnemanden
I tested all battle tags and restarted Neighborhood battle twice, without any bugs at all. The 1207 battle tag fix works great too here!
I would like to do some more tests (i.e. normal races with the bugfixer turned on) but that'll wait until someone perhaps, creates something similar for the 0916 patch.
Posted: 28 Aug 2010, 09:31
Abc
hey, jig, arto, revolt always do full cpu use and full bandwith use
aaand gpu!
Posted: 28 Aug 2010, 13:17
Huki
ah i have never read this topic.. nice.
Did you find out why market and garden battle doesnt crash and why compatibility fixes a problem with AINode init? Would be interesting to know..
arto wrote:What throws me off is that it is named "ZoneNext" and not "NodeNext", but maybe that's a mistake.
ZoneNext/ ZonePrev (members of AINODE struct) represent the Next or Previous Node of the
Current Zone. For the
First AINode of the 13th Zone for example (AINODE::ZoneID = 12), ZonePrev would be NULL. Similarly for the last ainode of 13th zone, ZoneNext would be NULL.
Atleast that is what I can understand
Posted: 28 Aug 2010, 17:26
jigebren
Abc wrote: hey, jig, arto, revolt always do full cpu use and full bandwith use tongue.gif aaand gpu!
Re-volt doesn't use full cpu is you run it in
fullscreen with
VSync turned ON.
Did you find out why market and garden battle doesnt crash and why compatibility fixes a problem with AINode init? Would be interesting to know..
Well, I can't remeber clearly. It was quite a while ago...