Source Code and Ideas question

[another_commander]

That's why I had proposed initially when the question was asked in another thread that a dictionary be used instead. As Nyarlatothep has pointed out, different size arrays of rating names and rating kills will result in a GNUstep exception being thrown or just a standard crash. So instead of using descriptions.plist array rating names with the core code's array of rating kills, it would be best if everything got read from a dictionary in descriptions.plist, with a format like:

Code: Select all

{
    "Harmless" = 8;
    "Mostly Harmless" = 24; // or whatever it is
    ...
    "Elite" = 6400;
};

This way you have a) everything in one place, nice and neat and b) much less chance of messing up rating names with corresponding kill ratings, with subsequent crashes.

[cim]

You've got 2 independent arrays there, and by the look of it, there's no guarantee in the code that rating_names has got as many entries as rating_kills. If you try to get a string or a number from past the end of the array, any array, programs do tend to crash a lot.

Depends on your language. Obj-C's array and dictionary objects just return nil if you ask for something they don't have, though, and then there's the oo_arrayForKey, oo_unsignedIntAtIndex and oo_stringAtIndex helpers (from OOCollectionExtractors.m) which are type-safe even on missing indexes. I think the worst it'll do is return nil or @"" instead of the string you want (possibly unless ratingKills is an empty array)

Pleb: I'm assuming you did the obvious things like shift-restarting after changing descriptions.plist? The only other thing I can see is that if you have the obvious arrangement of elements in the arrays ("Harmless", "Mostly Harmless", etc) vs (0,8,etc.) the oo_stringAtIndex:i to get the rating out should be an oo_stringAtIndex:i-1

The dictionary approach would still be better (though you'd have to be somewhat more complex with the checking code, since dictionary keys will not be returned in order)

[JensAyton]

Depends on your language. Obj-C's array and dictionary objects just return nil if you ask for something they don't have, though

You’d think, but NSArray throws an exception, which is de facto a crash.

[hangar18]

Thanks guys for your input...it seems though that I still have v1.79 installed despite calling in 1.77.1, through git checkout 1.77.1. When I later update the code after making changes to a few .m files, I followed the stage 4 instructions by another_commander

git pull
git submodule update
make debug=no

and this seems to have brought back 1.79. Does this code need 1.77.1 in it somewhere?

[another_commander]

1.77.1 is a tagged revision, i.e. the exact revision of the code that was used for the release of version 1.77.1. You are not meant to do a git pull and submodule update if you are working on it. The instructions in the Oolite-PC subforum are for updating the master branch (trunk) code and do not necessarily apply to development happening on side branches. If you chose to play with side branches, I recommend reading the github online documentation first, because git is not exactly the simplest of version control systems.

At this stage, I would try a git checkout 1.77.1 again, re-apply changes you have made and NOT attempt another git pull or submodule update. Submodules are separate sub-projects in the repository and have their own revision system. By updating them, you basically bring them to their latest revision, which is what trunk uses.

Edit: When I do a git checkout 1.77.1 I get the below message. Maybe it's worth following the instruction contained therein, but I have not tried it myself and cannot guarantee how it will work for you.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

git checkout -b new_branch_name

HEAD is now at dfc8e1f... Fix bug in disallowedDockingCollides (SVN:5698)

In any case, the v1.77.1 source code tarball is also available for download from github in both .zip and .tar.gz formats. If all else fails, you can always download that, unzip it to a folder of your choice, then use the development environment to cd into that folder and run make debug=no. That's completely independent of git, though.

[Pleb]

That's why I had proposed initially when the question was asked in another thread that a dictionary be used instead. As Nyarlatothep has pointed out, different size arrays of rating names and rating kills will result in a GNUstep exception being thrown or just a standard crash. So instead of using descriptions.plist array rating names with the core code's array of rating kills, it would be best if everything got read from a dictionary in descriptions.plist, with a format like:

Code: Select all

{
    "Harmless" = 8;
    "Mostly Harmless" = 24; // or whatever it is
    ...
    "Elite" = 6400;
};

This way you have a) everything in one place, nice and neat and b) much less chance of messing up rating names with corresponding kill ratings, with subsequent crashes.

I actually tried this first, although I had it the other way around (6400 = "Elite"). It read this fine, but only if your kills were exactly 6400, if it was over it set to nil. But I think this was because of how I told it to read from the array...

Pleb: I'm assuming you did the obvious things like shift-restarting after changing descriptions.plist? The only other thing I can see is that if you have the obvious arrangement of elements in the arrays ("Harmless", "Mostly Harmless", etc) vs (0,8,etc.) the oo_stringAtIndex:i to get the rating out should be an oo_stringAtIndex:i-1

The dictionary approach would still be better (though you'd have to be somewhat more complex with the checking code, since dictionary keys will not be returned in order)

I did try all of this, and I also spotted the i-1 after I had posted up what I had so far, but it was already nearly 3am by that point and I was far too tired! I will take another look at this tonight when I get home from work though, as although it loads fine the first time, if you go off the status screen and back onto it again it crashes to the desktop. The same if you go to the load screen, it displays it the first time but then afterwards when it goes to the status screen it crashes...

[hangar18]

1.77.1 is a tagged revision, i.e. the exact revision of the code that was used for the release of version 1.77.1. You are not meant to do a git pull and submodule update if you are working on it. The instructions in the Oolite-PC subforum are for updating the master branch (trunk) code and do not necessarily apply to development happening on side branches. If you chose to play with side branches, I recommend reading the github online documentation first, because git is not exactly the simplest of version control systems.

At this stage, I would try a git checkout 1.77.1 again, re-apply changes you have made and NOT attempt another git pull or submodule update. Submodules are separate sub-projects in the repository and have their own revision system. By updating them, you basically bring them to their latest revision, which is what trunk uses.

Edit: When I do a git checkout 1.77.1 I get the below message. Maybe it's worth following the instruction contained therein, but I have not tried it myself and cannot guarantee how it will work for you.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

git checkout -b new_branch_name

HEAD is now at dfc8e1f... Fix bug in disallowedDockingCollides (SVN:5698)

In any case, the v1.77.1 source code tarball is also available for download from github in both .zip and .tar.gz formats. If all else fails, you can always download that, unzip it to a folder of your choice, then use the development environment to cd into that folder and run make debug=no. That's completely independent of git, though.

Thanks, I'm learning slowly but surely. Thanks for persevering with me!

[cim]

Depends on your language. Obj-C's array and dictionary objects just return nil if you ask for something they don't have, though

You’d think, but NSArray throws an exception, which is de facto a crash.

So it does. That's what I get for posting before breakfast...

Still, the oo_typeAtIndex methods are safe to point off the end of an array.

it would be best if everything got read from a dictionary in descriptions.plist

Thinking about it, descriptions.plist is not a sensible place to be putting kill count thresholds. I don't think we currently have a plist file which is, though, but if we did it could contain a dictionary of kill counts and description.plist keys, to be expanded later.

[Nyarlatothep]

Fair enough! Still, some out-of-scope shenanigan does seem to be happening, regardless.

Failing everything else, an old, old debugging trick all the way from BASIC was to add a PRINT statement every other line, like this:

Code: Select all

10 do something
20 PRINT "done something"
30 something else
40 PRINT "done something else"
50 and another thing
60 PRINT "and another thing"

& if the last thing you saw before the crash was "done something else", the problem was inside line 50.

You could hijack the code that prints to the log file as a modern version of ye olde PRINT debugging, and sure enough it will let you know the exact point where the code falls over.

Hope this helps.

[cim]

You could hijack the code that prints to the log file as a modern version of ye olde PRINT debugging, and sure enough it will let you know the exact point where the code falls over.

Not really a hijack, since that's what OOLog gets used for quite a bit already...

A more efficient approach is to use a proper debugger: if you're on Linux or Windows, gdb; if you're on Mac, Xcode has one built in. Recompile Oolite with debug=yes or you won't get much useful information out of it.

Run the program in the debugger, then when it crashes, get the backtrace. Here's a gdb example

Code: Select all

$ gdb oolite.app/oolite.dbg
...gdb loads the program and prepares to run it
(gdb) run
...gdb runs the program
...it crashes
(gdb) bt
#0  0x00007ffff50876c0 in objc_msg_lookup ()
   from /usr/lib/x86_64-linux-gnu/libobjc.so.4
#1  0x00000000005d16b6 in TreeSplay (root=0x7fffd4207060, key=0x3e869d0)
    at src/Core/OOCache.m:704
#2  0x00000000005d0fa9 in CacheRemove (cache=0x7fffd4207060, key=0x3e869d0)
    at src/Core/OOCache.m:481
#3  0x00000000005d110e in CacheRemoveOldest (cache=0x7fffd4207060, 
    logKey=0x7fffdd415990) at src/Core/OOCache.m:512
...and so on

So this shows that (#0) the program crashed in objc_msg_lookup - which generally means that you tried to call an object method on something which wasn't a valid object. Entry #1 shows that happened because of line 704 of OOCache.m, in the TreeSplay function. Entry #2 shows what called that, and so on.

This can really help put the logging calls into the right places and make them print the right variable contents to track down where the mistake was.

[Pleb]

Okay I fixed it, not sure how, but anyway... Open up OOConstToString.m and go to line 470, and replace the section there with the following:

Code: Select all

NSString *OODisplayRatingStringFromKillCount(unsigned kills)
{
	NSArray			*ratingNames = nil;
	NSArray			*ratingKills = nil;
	unsigned		i;

	ratingNames = [[UNIVERSE descriptions] oo_arrayForKey:@"rating_names"];
	ratingKills = [[UNIVERSE descriptions] oo_arrayForKey:@"rating_kills"];
	for (i = 0; i < [ratingKills count]; ++i)
	{
		if (kills < [ratingKills oo_unsignedIntAtIndex:i])  return [ratingNames oo_stringAtIndex:i-1];
	}

	return [ratingNames oo_stringAtIndex:[ratingKills count] - 1];
}

Then go to line 493 and replace the section there with the following:

Code: Select all

NSString *OODisplayStringFromLegalStatus(int legalStatus)
{
	NSArray			*statusNames = nil;
	NSArray			*statusBounty = nil;
	unsigned		i;

	statusNames = [[UNIVERSE descriptions] oo_arrayForKey:@"legal_status_names"];
	statusBounty = [[UNIVERSE descriptions] oo_arrayForKey:@"legal_status_bounty"];
	for (i = 0; i < [statusBounty count]; ++i)
	{
		if (legalStatus < [statusBounty oo_intAtIndex:i])  return [statusNames oo_stringAtIndex:i-1];
	}

	return [statusNames oo_stringAtIndex:[statusBounty count] - 1];
}

Then open up descriptions.plist and add the following:

Code: Select all

"rating_kills" = 
	(
		"0",
		"8",
		"16",
		"32",
		"64",
		"126",
		"512",
		"2560",
		"6400"
	);
	
	"rating_names" = 
	(
		"Harmless",
		"Mostly Harmless",
		"Poor",
		"Average",
		"Above Average",
		"Competent",
		"Dangerous",
		"☆ Deadly ☆",
		"★★★ E L I T E ★★★"
	);
	
	"legal_status_bounty" =
	(
		"0",
		"1",
		"51"
	);
	
	"legal_status_names" =
	(
		"Clean",
		"Offender",
		"Fugitive"
	);

Then compile and you're done! You can now change the kills and bounties require for statuses and ratings, and even define what they say. For example:



I created a script to generate the kills, bounties (and add a load of cash and equipment) for testing purposes just for this, but there you go...

[Zireael]

*dun dun*

Thanks for this, Pleb. Can we get this into git source soon? *puppy eyes*

[another_commander]

*dun dun*

Thanks for this, Pleb. Can we get this into git source soon? *puppy eyes*

First of all, good job to Pleb. I would not have a problem putting this in until we get a more water-tight implementation. However, it's not ready to go in yet. There is no error checking at all and it is in fact very easy to mess things up if the two value and names arrays are of different sizes. This is what I was able to do just by mucking for two seconds with descriptions.plist:

OXPs should not be able to do this to the game. It needs some more work. And, in all fairness, you can do something similar even now to the game if, for example, you comment out a few of the ratings in descriptions, but the point is that if we are going to go ahead and change it, let's do it right the first time.

[Pleb]

Agreed, this was a very crude way of doing this and I would much prefer to implement this using a dictionary laid out like mentioned before 6400 = "Elite". Also it might be better if it read this from a different plus file. This can be accomplished easily as I've done it before with something else I was messing about with in the source.

[another_commander]

I would still be OK with the above implementation (for now), as I believe it's better than what we have now functionality-wise, provided that basic errors are caught and maybe emergency fall-back hardcoded arrays exist in case something goes wrong with the external ones.