next competition (67)

[will_asher]

Nick suggested having DaJAngband as the next competition, and I think that's a cool idea. But I'm having trouble deciding which race/class combo to use.

Some choices:

Bilbo the Hobbit Thief - Thief is not as good a fighter as the rogue and has less HPS, but has a class speed bonus (+2 to start, additional +1 every 20 levels) and learns magic a little faster. (alchemy realm) I also like to hear from people if the telekinesis spell is actually useful. (Thieves and tourists are the only ones who get that spell which allows you to pick up items from a distance.)

Roscoe the Kobold Thief - In a character dump I said I like the hobbit thief combo, Pete suggested the kobold race might be better, so I'll let you chose.
Hobbits have better DEX, CON, and INT, hold life, 'nice' flag (nonevil animals and light fairies are less aggressive)
Kobolds have better STR, alertness, hit die, and partial poison resistance. I haven't really tested how effective the partial poison resist is against significant breathers.

Murkum the Dark Elf Assassin - Assassin class is pretty similar to the rogue: slightly weaker in most ways, but stronger with magic, especially offensive magic. (black magic realm) Get to use some other interesting spells like drain charges and beam of destruction.

Human Tourist - For those who want a tough challenge. Tourists have some of the worst fighting skills of any class, but I think they'll be fun if you don't mind taking risks. Thier magic is mostly divination/detection or random-effect spells.

Half-Troll Paladin - Had to have this as an option because it's one of my favorites. Because a paladin is one of my favorites, I might have made them a little too powerful (except that they get orb of draining later than in V).

Note / Disclaimer for experienced players: 1) The changes which only show in deeper levels of DaJAngband have only barely been tested (I have used wizard mode to make sure spells and effects work the way I meant them to, but when actually playing I almost never get past L40). 2) I have intended to make DaJAngband slightly easier than vanilla Angband, so if some things seem a little easier than you're used to, that's probably why. (Although since a lot of stuff isn't well tested, some things might be harder than I mean them to be).

other much less important notes:
- There's a stupid-looking mistake in the spell name of an early thief spellbook. (Edit: fixed in the update release).
- I combined object & treasure detection in most places, but overlooked them in the alchemy spell realm. Will be fixed in 1.0.98. Also, I'm planning to make the early disinfectant stronger and remove 'strong disinfectant' because they're nearly useless as they are now.
- Just so you know, I will have connected stairs turned on because I really don't like playing with it turned off (that's why I don't often play in competitions), and I'd like to play in the competition of my variant.
- I will use a renamed safefile which I have used before so there will be some monster memory.

[Pete Mack]

I did an OSX build and got zillions of dangerous warnings, some of which are certain bugs.

First and foremost:
There's a disastrous misuse of the global variable, spellswitch.
I have no idea what this thing is or how it works. It appears to be a massive goto across multiple source files. GET RID OF IT, or you won't ever be able to maintain your code.

Some of the warnings from dajaband are obtrusive and potentially dangerous:
multiple embedded /* in comment, in defines.h
ditto externs.h, line 537

Almost certain bug, otherwise an incredibly dangerous construction:
1947 birth.c: embedded /* in comment

dangerous warnings, probable bug:
birth.c:545: warning: statement with no effect
birth.c:546: warning: statement with no effect

another dangerous warning. Just uncomment it, even though it's a no-op, or use #if 0 construct:
birth.c:1997: embedded /* in comment

Borderline illegal constructs:
cmd0.c:316: warning: implicit declaration of function ‘do_cmd_castluck’
cmd0.c:318: warning: implicit declaration of function ‘do_cmd_castchem’
cmd0.c:320: warning: implicit declaration of function ‘do_cmd_castblack’

BUG!!!!!
cmd1.c:885: warning: control reaches end of non-void function

Dangerous Mystery warning. (I have no idea whether this is a bug or not):
files.c:1166:44: warning: "/*" within comment

Another dangerous mystery:
load.c:1456:26: warning: "/*" within comment
Note: use

#if 0  /* TODO: flags4 */
code;
#endif

for this kind of disabled code. It is much easier to find after you decide what to do with it. The tag TODO: flags4 allows you to grep for all instances of this future feature. (BTW: Microsoft has (or had) a patent on this, no lie!)

More borderline illegal warnings:
melee1.c:1599: warning: implicit declaration of function ‘dispel_life’
melee1.c:1802: warning: implicit declaration of function ‘dispel_silver’

BUG!!!
melee2.c:461: warning: suggest parentheses around assignment used as truth value

Use #if 0 /* TODO */
melee2.c:897:2: warning: "/*" within comment

Borderline illegal:
melee2.c:2007: warning: implicit declaration of function ‘scare_monsters’

Use:
#if 0 /* TODO Black magic */
object1.c:1019:5: warning: "/*" within comment


Other annoying warnings include:
* unused variable, when the variable is not a function argument.

ALMOST CERTAIN BUG: Multiple redeclarations without use of k_idx:
object2.c:739: warning: unused variable ‘k_idx’
object2.c:744: warning: unused variable ‘k_idx’

BUG AT LINE 1807: (You set both BUG and ANIMAL for all cases.)
spells2.c:1728: warning: unused variable ‘l_ptr’

I HAVE NO IDEA WHAT THIS IS:
spells2.c:4074: warning: suggest parentheses around && within ||

APPARENT BUG: misspelling of descent:
use-obj.c:1218: warning: implicit declaration of function ‘deep_decent’
(Why this warning is so dangerous...., even though it's not technincally a bug.)

BUG: improper (re)declaration of use_charge:
use-obj.c:1417: warning: unused variable ‘use_charge’

x-spell.c:
Warnings are too numerous and confusing to know whether they are bugs or not. NEEDS MAJOR REWRITE.


MULTIPLE BUGS:
xtra1.c:2402: warning: suggest parentheses around && within ||
I do not think it means what you think it means.

[Pete Mack]

PS:
Rules of thumb:
1. Get rid of compiler warnings, except for a very limited number of acceptably "bogus" warnings. (When in doubt, get rid of the warning.)
1a. Get rid of all warnings in .h files. (These really screw up the build output, since they repeat for every .c file.)
2. Don't use global state variables, unless it's absolutely necessary. spellswitch is a particularly egregious construction.

[will_asher]

Well, I went in and fixed all that I know how to fix (which was almost all of what you pointed out), and I looked through x-spell.c and cleaned up all the stuff I could find easily (which was several things). I don't know how to find the other stuff. Thanks a lot for that help. (although it'd probably be more fitting in a different thread)

I'm using Dev C++ and it's not giving me all those warnings when I compile. Is there a way I can find this kind of thing using my compiler if it doesn't give me these warnings when I compile or should I use a different compiler?
I had no idea there was anything wrong with having /* comment markers within comment markers. I thought anything within comment markers /* */ had no effect.

I used the spellswitch variable when I needed a variable I could access across different files to do things like tweak the effects of a called function. I ended up using it for several things kindof as a flag, but I kept track of what it does. I thought "why make a bunch of variables when I can use one variable for a bunch of different things?" I can probably get rid of it, but it would be a lot of trouble, and I don't really see why it's that urgent when I kept track of everything it does. (mainly, I have it in a text file, but I also put most of it in a big comment at the end of variable.c, and have just updated that comment so that it explains every use of spellswitch.) You were exactly right, it is a massive goto across multiple source files, and it's quite useful that way. For some of the things I used spellswitch for, it would be hard for me to figure out how it do it without using something like that. Is it really worth the trouble to get rid of it? What would happen if I don't? I mean, the game seems to work fine.

For the competition, I could easily send pav a new zip file with the code I just cleaned up, but when you actually play the game there'd be almost no difference (except for a couple slightly messed up tourist spells). Only a couple of those things were actually bugs, and those few were minor ones (the rest were just sloppy stuff in the code which I don't think would change how the program runs).

[Pete Mack]

I'm using Dev C++ and it's not giving me all those warnings when I compile. Is there a way I can find this kind of thing using my compiler if it doesn't give me these warnings when I compile or should I use a different compiler?
I had no idea there was anything wrong with having /* comment markers within comment markers. I thought anything within comment markers /* */ had no effect.

There's almost never anything wrong in any one use of embedded /*. It just turns out to be hard to read commented-out code, and if there's a comment embedded in your commented code, the commenting-out will fail. It's a really easy way to introduce bugs, and it's easy to avoid, and easier to read, with unreachable (#if 0) conditional code.

I doubt there's a way to get those warnings with Dev-C++, although the usual way is to add -Wall (or whatever the compiler directive for being persnickety is.)

You can get the warnings if you use MINGW, which is the same compiler as is used on OS X (gcc).

And about that switchspell: it's just very fragile code, and those numbers are impossible to figure out for any casual reader. (The use in curse armor is particularly confusing. I stopped reading in confusion when I saw it.)

The proper way to do this is to add a parameter (usually a flag, #defined value, or enumerated type) to the called function, so only the callee needs to know the actual context in which it is called.

Consider:

    switchspell = 3;
    foo(x);

...
void foo (int y)
{
   if (switchspell == 3 || switchspell == 2) {...}  /* No context--WTF is 3 or 2?? */
   else ...
}

vs

    foo(x, true);

...
void foo (int y, bool strong_curse)
{
   if (strong_curse) ...
   else ...
}

[Pete Mack]

will_asher wrote:

Only a couple of those things were actually bugs, and those few were minor ones (the rest were just sloppy stuff in the code which I don't think would change how the program runs).

"A couple" bugs takes a hell of a long time to find 'in the field' or in testing. Getting rid of as many as possible up front is just a lot easier, and a low-pain way to get them is ... be strict with warnings.

(Even 30 harmless warnings for one bug is a really big payoff in number of bugs. So is 100.)

And the Inigo Montoya code is almost certainly buggy. You write something like:

if ( cond1 || cond2 && cond3 )

and you often mean

if ( (cond1 || cond2) && cond3 )

rather than:

if ( cond1 || (cond2 && cond3) )

which is what the unparenthesized code means.

[MrCba]

Tourist are fun. This is my favorite Nethack's class

[will_asher]

DaJAngband tourists are pretty different from Nethack tourists (although I did give them a handy camera flash spell which stuns monsters for a few turns).

I'll send the savefile to Nick tomorrow, looks like it'll be either an assassin or a tourist.

EDIT: Another thing I thought I should mention about the tourist: In DaJAngband, almost all classes have strong pseudo, but the wizard and tourist don't. Of course, they get the identify spell earlier than any other classes. (The tourist gets it even earlier than the wizard).
I playing it right now, and the tourist class doesn't seem that hard in the very early levels (partly due to the camera flash spell. I think I'll make it do less damage in the next release, it's supposed to be mainly for stunning), but I'm sure its weaknesses get more noticable later on..

[MrCba]

Originally Posted by will_asher

DaJAngband tourists are pretty different from Nethack tourists (although I did give them a handy camera flash spell which stuns monsters for a few turns).

I'll send the savefile to Nick tomorrow, looks like it'll be either an assassin or a tourist.

Ho no, did you mean that I wouldn't have my cat's pet ?

[will_asher]

Ha! No, pets are definetly beyond my coding ability.