Note for jamespetts, please consider pulling from my jp-devel branch

Topic: Note for jamespetts, please consider pulling from my jp-devel branch (Read 3613 times) previous topic - next topic

Note for jamespetts, please consider pulling from my jp-devel branch

April 26, 2010, 03:14:00 am

For some reason

I just spent several hours manually merging in one of Dwachs's commits from standard which merges very uncleanly because it combines work on the old physics engine (irrelevant to experimental) with code cleanup and rearrangement (which *is* relevant).

You probably don't want to duplicate this work so I suggest merging my branch. If something is wrong it's still probably a better basis to start from!

Re: Note for jamespetts, please consider pulling from my jp-devel branch

Reply #1 – April 26, 2010, 09:35:48 am

Neroden,

thank you very much! I had noticed the changes and was not looking forward to merging them. One thing that I've noticed, however, is that the adjustment for bits per month has been removed from certain tooltips, seemingly without being replaced. Is this intentional, or is this an error?

Thank you again for doing this!

Edit: Having merged the code, I now find that I am getting a stack overflow in line 327 of obj_reader.cc every time that I load Pak128.Britian-Ex (I have not yet tried other paksets). I have no idea what causes this, as the debugger refuses to give me any backtrace.

Edit 2: I do not get the same error with the latest nightly of Standard.

Re: Note for jamespetts, please consider pulling from my jp-devel branch

Reply #2 – April 26, 2010, 10:46:24 am

Quote from: jamespetts – on April 26, 2010, 09:35:48 am

One thing that I've noticed, however, is that the adjustment for bits per month has been removed from certain tooltips, seemingly without being replaced. Is this intentional, or is this an error?

I think they are dealt with in calls to tooltip_with_price_maintenance(...)

Re: Note for jamespetts, please consider pulling from my jp-devel branch

Reply #3 – April 26, 2010, 11:00:26 am

Dwachs,

ahh, thank you for that clarification!

Neroden,

still having trouble with the stack overflow, but noticing that it only happens with Pak128.Britain-Ex.

Edit: Found that the problem was caused by using files compiled with a recent version of Makeobj - recompiling Makeobj after the latest changes from Standard and recompiling the paks with that version of Makeobj solved the problem.

Edit 2: I am now getting strange corruption (bizarre errors, often without any backtrace, in parts of the code not touched by Experimental) on loading games saved with Simutrans-Experimental 7.x. There do not appear to be similar errors in loading games saved with any other version of Simutrans-Experimental, or any version of Simutrans-Standard.

Re: Note for jamespetts, please consider pulling from my jp-devel branch

Reply #4 – April 26, 2010, 04:30:04 pm

Quote from: jamespetts – on April 26, 2010, 11:00:26 am

Dwachs,

ahh, thank you for that clarification!

Neroden,

still having trouble with the stack overflow, but noticing that it only happens with Pak128.Britain-Ex.

Edit: Found that the problem was caused by using files compiled with a recent version of Makeobj - recompiling Makeobj after the latest changes from Standard and recompiling the paks with that version of Makeobj solved the problem.

Edit 2: I am now getting strange corruption (bizarre errors, often without any backtrace, in parts of the code not touched by Experimental) on loading games saved with Simutrans-Experimental 7.x. There do not appear to be similar errors in loading games saved with any other version of Simutrans-Experimental, or any version of Simutrans-Standard.

*Sigh*. Well, the makeobj and object loading code in standard WAS changed, by prissi. And it was changed in several tricky ways: for one thing, a number of routines appear to correctly handle endianness now, which may not have before (which could cause some very odd results); for another thing, there's a new format for "larger nodes", and I have no idea what the implications of that are.

This is actually most likely from the code which merged cleanly. :-/ So you could create a temporary branch for finding the trouble (a "find-the-bug" branch), and reverse cherrypick;

d844a0f6a836d806ae814caac646364b214f3e35, which I merged as 7b55edadb5407d181bd76e7e831c44eebab0de6e, "making makeobj endian-independent for all operations"

and 7062ca5343e7ee43ca91a6c4eade6e078c9dc27b, which I merged as bd61156bfa359705ce8fd1119808a2c2eb3db964, "CHANGE: allowing for largers nodes in pak and making image nodes smaller"

are the commits most likely to be causing trouble. So you could try

Code: [Select]

git revert 7b55edadb5407d181bd76e7e831c44eebab0de6e

and/or

Code: [Select]

git revert bd61156bfa359705ce8fd1119808a2c2eb3db964

And see if one of those fixes the problem. If so, then that will give you a starting point in figuring out how to work around the change and read 7.x save games correctly.

The other, less likely, possibility is in my code. The fallout from merging c0fd3b7732cbaceb0f632f31946f8c0a3ff83cee included the discovery that akt_speed_soll in convoi_t was a *write-only variable* in experimental, so it's now thrown away on read. This could have curious results for some saved games.

Re: Note for jamespetts, please consider pulling from my jp-devel branch

Reply #5 – April 26, 2010, 06:00:11 pm

Neroden,

the current problem is not with the .pak files - I tested by recompiling all the pak files with the latest version of Makeobj - same problem. The game runs fine when opened with 7.3, but fails shortly after being opened with 8.0-devel.

Was the object loading code for loading and saving games, rather than .pak data such as vehicles changed, too? If so, that might explain it: possibly corrupt saved games - except that they open fine in 7.3...

It usually crashes in ding_t *dingliste_t::get_convoi_vehicle() const, but no backtrace is available. I don't know whether that gives you any pointers...?

Re: Note for jamespetts, please consider pulling from my jp-devel branch

Reply #6 – April 26, 2010, 06:29:55 pm

Quote from: jamespetts – on April 26, 2010, 06:00:11 pm

the current problem is not with the .pak files - I tested by recompiling all the pak files with the latest version of Makeobj - same problem.

That unfortunately doesn't mean that the problem isn't with the pakfile loading code. :-P We already know that pak files from a somewhat older version of makeobj were behaving badly with the current game version. If objects were loading into memory in a funny, wrong way in an older version, then the saved game from the older version could be corrupted in a funny way, so that the old version interprets it right but the new version misinterprets it. There's a serious shortage of sanity checks in the saved-game loading code, which means the bugs show up late rather than early. I've been hit by this sort of oddity before.... theoretically we could figure out the difference in the saved game and handle both cases on loading. If we could figure out what changed.

Or I could be barking completely up the wrong tree.

Quote

Was the object loading code for loading and saving games, rather than .pak data such as vehicles changed, too?

Parts of them are the same code. Though I don't think that part of the code changed, I'm not sure, because I didn't carefully review the two patches which applied cleanly. :-P

Quote

It usually crashes in ding_t *dingliste_t::get_convoi_vehicle() const, but no backtrace is available. I don't know whether that gives you any pointers...?

Nope. Again, my only idea is to try reverting the individual merge commits. If one of them is at fault, I would guess that suddenly the old, non-recompiled pak files will work with the 7.x saved games, and the new ones won't....

Do the saved games from 7.x open fine in 7.3 *with the new recompiled pak files*? Or, as I'm guessing, are the new pak files incompatible with 7.3?

I'm a little disturbed that you're losing the backtrace -- stack smash? It's probably the caller of ding_t *dingliste_t::get_convoi_vehicle() const which is causing the trouble, so without the backtrace it's hard to debug.

Re: Note for jamespetts, please consider pulling from my jp-devel branch

Reply #7 – April 26, 2010, 07:27:52 pm

Neroden,

I've found the problem, I think (by chance) - a few instances of failing to run proper checks before using data in a union in my new code. Fixed now by proper checks being put in place. Thank you for your help!