Testing a makeitem.cc patch.

[reaver]

I'm currently working on a clean up of makeitem.cc, which is very messy and hard to modify right now, and I have a couple questions to ensure the quality of my code.

First, I want to run crawl -test to ensure I don't break anything, but I can't decypher this line from testing.txt

  Code:

To use unit tests, you must compile Crawl either with the macro DEBUG_DIAGNOSTICS defined (the debug make target) or with both the macros DEBUG and DEBUG_TESTS defined (the wizard make target, plus -DDEBUG_TESTS).

I understand what -DDEBUG_TESTS is (I need to add it to the make command) but I don't get what "wizard make target" means.

Second, I don't know how closely I need to match the old formulas. Currently, I've worked based on the 1% rule- if the new formula changes the result in less than 1% of cases, It's fine. Please tell me if this is too large a margin (so I can adjust the work I've already done.) or too small a margin (so I can make the code even simpler at the expense of slightly different behavior).

[savageorange]

The wizard make target is what gets built when you run 'make wizard'. Apparently it adds -DDEBUG to the CFLAGS, according to the quote above.

If I were you I would just use the 'debug' make target.

Additional parameters to make seem to be ?required?, anyway. At least USE_UNICODE=y seems to be a good idea.
the 'stone-soup-git' PKGBUILD for Arch Linux runs 'make USE_UNICODE=y TILES=y install'* to build and install DCSS.
(and then again without the TILES=y to build the console version)

* actually it also specifies values for DESTDIR, SAVEDIR, and DATADIR, but I'm pretty sure they aren't relevant to what you're doing.

[mumra]

The wizard make target is a slightly out of date thing for the instructions to refer to really, since it's the default anyway now.

Second, I don't know how closely I need to match the old formulas. Currently, I've worked based on the 1% rule- if the new formula changes the result in less than 1% of cases, It's fine. Please tell me if this is too large a margin (so I can adjust the work I've already done.) or too small a margin (so I can make the code even simpler at the expense of slightly different behavior).

That seems fine to me, from what I've seen of makeitem a lot of the probabilities would be extremely awkward to replicate exactly. Also as lots of new items have been added it's doubtful the probabilities there are perfectly balanced anyway. If the file is better organised it should be easier to subsequently review and tweak the probabilities anyway.

I'm not sure how you're implementing it, but what would be quite nice is a big table of data like we have for mon-pick so the relative weights of different items can be seen at a glance. It's even worth considering using the mon-pick code (that I refactored into random-pick.h for exactly this kind of purpose) so we can weight items e.g. by depth/branch. But at least getting things into a form where we can see what the probabilities are is a good start

Something else worth checking - have you run any item generation stats? Would be good to get a sample of many item generations (there's probably already a script that does this) both before and after your changes.

[reaver]

I got crawl to compile with -DDEBUG_TESTS, but now I have a different problem. When I type "crawl -test" I get a command not found error. I got it to compile with this:

  Code:

make APPLE_GCC=y NO_PKGCONFIG=y CONTRIB_SDL=y TILES=y DDEBUG_TESTS=y

I'm not sure how you're implementing it, but what would be quite nice is a big table of data like we have for mon-pick so the relative weights of different items can be seen at a glance. It's even worth considering using the mon-pick code (that I refactored into random-pick.h for exactly this kind of purpose) so we can weight items e.g. by depth/branch. But at least getting things into a form where we can see what the probabilities are is a good start

Using random-pick.h sounds like something that should be done eventually, but I've already got some changes and I think this patch has enough stuff in it. I'm going to start off like this:

  Code:

static brand_type _roll_mace_brand()
random_choose_weighted(100, SPWPN_DRAINING,
                           300, SPWPN_PROTECTION,
                           150, SPWPN_HOLY_WRATH,
                           165, SPWPN_VORPAL,
                           12,  SPWPN_DISTORTION,
                           11,  SPWPN_ANTIMAGIC
                           11,  SPWPN_PAIN,
                           0);
......
switch (item.sub_type)
            {
            case WPN_EVENINGSTAR:
                rc= random_choose_weighted(71, SPWPN_DRAINING,
                                           58, SPWPN_NORMAL,
                                           649, _roll_mace_brand(),
                                            etc.

Note that's just a quick mock-up of what I'm doing.

Something else worth checking - have you run any item generation stats? Would be good to get a sample of many item generations (there's probably already a script that does this) both before and after your changes.

No, I haven't, but I would love to. Do you know where I should look for such a script? I tried looking in docs/develop and grepping for "item generation test" but I came up empty-handed.

[galehar]

I got crawl to compile with -DDEBUG_TESTS, but now I have a different problem. When I type "crawl -test" I get a command not found error. I got it to compile with this:

  Code:

make APPLE_GCC=y NO_PKGCONFIG=y CONTRIB_SDL=y TILES=y DDEBUG_TESTS=y

Coding on a mac, that's new
Have you tried to do make debug instead of using DDEBUG_TESTS? Like that:

  Code:

make debug APPLE_GCC=y NO_PKGCONFIG=y CONTRIB_SDL=y TILES=y

Command not found heh? Does the crawl executable has been generated? Maybe you need to do ./crawl

Something else worth checking - have you run any item generation stats? Would be good to get a sample of many item generations (there's probably already a script that does this) both before and after your changes.

No, I haven't, but I would love to. Do you know where I should look for such a script? I tried looking in docs/develop and grepping for "item generation test" but I came up empty-handed.

I don't think there is one, but you can try adapting scripts/place-population.lua. It generates maps and count monsters, should be too hard to make it count items instead.

[reaver]

Coding on a mac, that's new

That's interesting. Is it because Macs are terrible for programming or just uncommon? I've heard the "learning to break dance in a body cast" comparison before but I never took it too seriously.

Maybe you need to do ./crawl

This was completely correct, it works now. Except there's a crash if I try to use even ./crawl on a tiles debug build (with or without my patch). It works fine on console. (My patch passed all the tests.)

I don't think there is one, but you can try adapting scripts/place-population.lua.

I've never tried to program in lua before, but I'm sure I'll manage to get through. Any suggestions on resources to learn? (It doesn't help that script is completely uncommented.)

[galehar]

Coding on a mac, that's new

That's interesting. Is it because Macs are terrible for programming or just uncommon?

I don't think we ever had a mac guy in the devteam. But I can't judge if they are good for coding, I don't know enough. I know that apple has a tendency to provide obsolete gcc versions in the xcode toolchain, though.

I don't think there is one, but you can try adapting scripts/place-population.lua.

I've never tried to program in lua before, but I'm sure I'll manage to get through. Any suggestions on resources to learn? (It doesn't help that script is completely uncommented.)

Sorry, can't really help you there.