Regression tests

[turican0]

I've tried regression tests(comparing the data with the original) and currently they all fail for all levels. Before the merge it was fine. Finding problems is important because it can reveal a lot of things(overflows, bad values, etc). It's one of the things I need to focus on.

[turican0]

Test in 32bit(correct init and begin of levels): level1 - OK level2 - OK level3 - OK level4 - OK level5 - OK level6 - OK level7 - OK level8 - OK level9 - OK level10 - OK level11 - OK level12 - OK level13 - OK level14 - OK level15 - OK level16 - OK level17 - OK level18 - OK level19 - OK level20 - OK level21 - OK level22 - failed (mey by in previous version too) level23 - OK level24 - OK level25 - OK

[turican0]

Test in 64bit(correct init and begin of levels): level1 - OK level2 - OK level3 - OK level4 - OK level5 - OK level6 - OK level7 - OK level8 - OK level9 - OK level10 - OK level11 - OK level12 - OK level13 - OK level14 - OK level15 - OK level16 - OK level17 - OK level18 - OK level19 - OK level20 - OK level21 - OK level22 - failed (mey by in previous version too) level23 - OK level24 - OK level25 - OK

[GrimSqueaker]

I adapted the script to Linux, but here the regression tests do not look that good:

[sebi@xpsratz remc2]$ ./tests/testRegressions.sh 
Make sure that you have compiled the game with TEST_REGRESSIONS_GAME in sub_main.cpp
Get the data files from https://github.com/turican0/remc2/tree/data
Extract the ZIP archives to $data_location
Adjust the path to the remc2 executable
testing level 1
Regression compare sequence error @ function compare_with_sequence, line 1276: 33
test level1 failed
testing level 2
Regression compare sequence error @ function compare_with_sequence_D41A0, line 896: 14748
test level2 failed
testing level 3
Regression compare sequence error @ function compare_with_sequence, line 1276: 76
test level3 failed
testing level 4
Regression compare sequence error @ function compare_with_sequence, line 1276: 0
test level4 failed
testing level 5
Regression compare sequence error @ function compare_with_sequence, line 1276: 45
test level5 failed
testing level 6
Regression compare sequence error @ function compare_with_sequence, line 1276: 21
test level6 failed
testing level 7
Regression compare sequence error @ function compare_with_sequence, line 1276: 0
test level7 failed
testing level 8
Regression compare sequence error @ function compare_with_sequence, line 1276: 17
test level8 failed
testing level 9
Regression compare sequence error @ function compare_with_sequence, line 1276: 0
test level9 failed
testing level 10
Regression compare sequence error @ function compare_with_sequence, line 1276: 0
test level10 failed

[turican0]

Try testing just one level in the debug environment. Set test_regression_level in sub_main.cpp to that level(now I don't know if it is 0 or 1 for the first one). Copy the data from remc2\memimages\regressions\level1\ to remc2\memimages\regressions\ Run and see what variable the difference is on. If it was a pointer there is a function test_D41A0_id_pointer that puts the pointers in exceptions so they don't compare (but that should be sorted out by now). Let me know how you get on.

[GrimSqueaker]

Thanks for the hints @turican0 ,

So, with a 64 bit build for level 1 (number 0 in-game) I have a difference here (see the call stack in the bottom left):

It is not a pointer in this case.

Does i=14748 mean that I have to look at position 14748=0x399C in shadow_D41A0_BYTESTR_0 which would be type_str_0x2BDE array_0x2BDE[0x8]; in type_shadow_D41A0_BYTESTR_0 or did I misinterpret this?

[GrimSqueaker]

This is the reference (left) as compared to the memory (right)

How can I find the affected variable in type_shadow_D41A0_BYTESTR_0

[turican0]

Hi, I'll look into it this afternoon, anyway thanks for the report, I'll check it this afternoon, see what variable it is, where it comes from, and how it changes for me.

[turican0]

D41A0_0.array_0x2BDE[1].dword_0x3E6_2BE4_12228.dword_0x189_393 is 14745 - some clock variable

This can be detected by address difference(in 64bit only in shadow structure): (char)(&(D41A0_0.array_0x2BDE[1].dword_0x3E6_2BE4_12228.dword_0x189_393))-(char)(&D41A0_0)

See v2x->dword_0xA4_164x->dword_0x189_393 = j___clock(); We should extend the exception in test_D41A0_id_pointer: if ((adress >= 0x3999) && (adress < 0x399c))return 2;//clock2 -> if ((adress >= 0x3999) && (adress < 0x399d))return 2;//clock2

This didn't happen for me, but I guess clocks are generated differently on Linux. If you still run into something, try to see if it's something significant in a similar way, and if not, add it to the exceptions as well. But be careful if you're not sure, I'd be happy to help you identify the problems. Hopefully I'll get to functional testing on Linux as well. Thanks, Tom.

[GrimSqueaker]

Ah, thanks. But I have to do this for the shadow structure on 64 bit, right?

int pos = (char*)(&(shadow_D41A0_BYTESTR_0.array_0x2BDE[1].dword_0x3E6_2BE4_12228.dword_0x189_393))-(char*)(&shadow_D41A0_BYTESTR_0);
std::cout << "diff pos: " << pos << std::endl;

Then it indeed comes out as 14745.

Yes, the clocks behave differently. That is what I also noticed when I was working on the too 2-5 fps video / level selection a couple of days ago. j___clock() returns a long, i.e. a 64 bit value. Maybe on Windows you get the time since the executable is started and on Linux IIRC you get the time since midnight. So on Windows you might just be lucky to have zeros in the high bytes when you do the comparison. Or something like that. Yes, excluding the whole 4 bytes of that value in the shadow structure should be good.

This one is fixed now.

The next problem arises with str_E2A7 which contains a pointer but is compared byte-by-byte with the memimage. This cannot work as the pointer type_event_0x6E8E* dword_12; takes 8 bytes on a 64 bit system. I think we need a shadow structure here, too.

I will continue on this tomorrow.

[turican0]

Yes you are right str_E2A74 also needs a shadow structure. The mystery is that it works on windows without it :) Maybe it was all zeros. Please add to code this procedure: void Convert_to_shadow_E2A74(type_str_E2A74 from, type_shadow_E2A74 to) { to->word_0 = from->word_0; for (int i = 0; i < 4; i++)to->axis_2[i] = from->axis_2[i]; to->dword_12 = ((uint8_t*)from->dword_12 - Zero_pointer); to->dword_16 = from->dword_16; to->dword_20 = from->dword_20; to->dword_24 = from->dword_24; to->byte_28 = from->byte_28; to->byte_29 = from->byte_29; } (I'd rather you do this, because I don't know what you changed in the code and it's pointless to synchronize of code it now)

[GrimSqueaker]

Yeah, I will do this, because I anyhow want to understand all these tests. Can I try to convert these defines for the regression tests into feature flags that can be controlled via command line flags? This would be good, because then we have just one executable that does not need to be recompiled. And if we could also get rid of the SDL and audio stuff, then we could run these regression test in a Github action during PRs.

[turican0]

That was great. If the tests were running on GitHub. Absolutely. My way of testing is primitive. Please be careful that it still works even with DEBUG_AFTERLOAD/DEBUG_ONSTART. I can test through that not only for errors at the beginning of the level, but also after a save is loaded, after a spell is cast, etc. But it will definitely override. We should also expand testing to shadow and multiplayer levels. Thanks, Tom.

[GrimSqueaker]

I have introduced a shadow struct for this. I needed to adapt it a little since this is actually an array.

Now there is a diff at byte 40 in compare_with_sequence_array_E2A74, i.e. the 10th byte of the second array element -> axis_2[4]. Do you have any idea what that is?

[turican0]

Proměnná str_E2A74[1].axis_2[4] is set at startup: type_str_E2A74 str_E2A74[0x69] = ... {0x0004,0x011E,0x0032,0x0140,0x0050} is 0x50 i.e. 80 And then it doesn't change. If it has changed something is wrong. If your IDE has the ability to set a breakpoint when a variable changes, set it to change &str_E2A74[1].axis_2[4] after startup and watch where the change occurs.

Now I see that I made a mistake in for (int i = 0; i < 4; i++) to[j].axis_2[i] = from[j].axis_2[i]; it should be: for (int i = 0; i < 5; i++) to[j].axis_2[i] = from[j].axis_2[i]; :)

[GrimSqueaker]

Yeah, I could have seen that, too. :) After I have fixed this it fails at byte 543. 543 = 18*30 + 3, so again in axis_2[0] of array element 18. It should be 0x400 and is 0 instead.

The value is set to 0 in sub_88580 in line str_E2A74[index].axis_2[0] &= 0xA7u;. Do you have any idea, why this is not happening in your test?

Here is the callstack

[GrimSqueaker]

Wait. I think I did a mistake...

[GrimSqueaker]

Hmmm. Still a problem. I thought 543 was initially excluded from the comparison in test_E2A74_id_pointer but it wasn't. So, any idea if this should be excluded for some reason?

[GrimSqueaker]

I get diffs here in the first check:

Regression compare sequence error @ function compare_with_sequence_array_E2A74, line 1180: 543
Regression compare sequence error @ function compare_with_sequence_array_E2A74, line 1180: 573
Regression compare sequence error @ function compare_with_sequence_array_E2A74, line 1180: 633
Regression compare sequence error @ function compare_with_sequence_array_E2A74, line 1180: 2433
Regression compare sequence error @ function compare_with_sequence_array_E2A74, line 1180: 2463
Regression compare sequence error @ function compare_with_sequence_array_E2A74, line 1180: 2493
Regression compare sequence error @ function compare_with_sequence_array_E2A74, line 1180: 3089

[turican0]

I missed a line at the end of the E2A74 test if (i < size2) allert_error();, so the errors of this structure were ignored. Skip this test(compare_with_sequence_array_E2A74) for now. I'll look at it later when I finish the network code.

[turican0]

Aren't you going to do an ongoing pull request to https://github.com/thobbsinteractive/magic-carpet-2-hd? I'd like to do some network code function moves, but I'm waiting for you to integrate your code with the cleanup. Related to that, I would at least put a note where the line with the unused original function was removed.

some code; //here was in the original code function xyz(), probably some dealocations some code;

If we add something in the future at least we will know that there was a function in that place.

[GrimSqueaker]

Ok, I'll disable that test for the moment.

Yeah, I thought I could get at least some of the tests passing, but I will open a PR and re-insert some of the removed functions as comments.

[turican0]

Don't turn off all the tests when you get going on this, just turn off compare_with_sequence_array_E2A74, because that doesn't work for me either - why I need to look at that in DOSBOX. The other tests should be able to be run.

[GrimSqueaker]

Yep. I did disable just the one.