Fix bug in Vorpalize scroll

[jwt27]

The Vorpalize scroll enchants a weapon to do more damage to a specific type of monster. This monster is randomly picked in pick_mons() from the string lvl_mons.

Problem is, this string contains spaces, and sometimes (surprisingly often) it lands on one. Identification then causes an out-of-bounds access on the monster name array. In the original version, this just results in a garbled name:

On modern systems however, we get a segmentation fault.

This patch resolves the issue by skipping over spaces. Evaluation order could change the mechanics slightly, (*cp == ' ' || rnd(10)) vs (rnd(10) || *cp == ' '). I think the former increases the probability of selecting a higher level monster.

In case this behaviour was actually intentional (i.e., it should be possible for vorpalize to partially fail), then pick_mons() should return 0 instead when it lands on a space. The weapon still receives a +1,+1 boost, and it is then possible to use a second vorp scroll on the same weapon. Personally I doubt the scroll was intended to work this way.

[jwt27]

Another option is to index from wand_mons if *cp == ' ', so all monsters are potential vorpalize targets. Maybe that's the best solution.

[MestreLion]

Seems a strange oversight from Rogue devs. I'm usually reluctant of changing anything in the original mechanics or gameplay, even bugs, but a segfault should be addressed.

I like the idea of using wand_mons in case of *cp == ' ', but it's a bit brittle as it relies on the convenient fact that wand_mons has monsters on every space of lvl_mons. Also, as cp is a pointer and not an index, the implementation of peeking the same index on another string requires substantial change to that function.

Another solution is to use the existing fallback for Medusas to include the *cp == ' ' case. As it is, it currently only triggers if all rnd(10) checks fail, but it's possible that the original intention was for that to be a general fallback for all fail cases.

So I believe if (cp < lvl_mons || *cp == ' ') return 'M'; is the closest to the original intention. Yes, it would dramatically increase the odds of Vorpalizing Medusas, but it's fair to assume this was/could be intentional.

Btw... nicely spotted bug! In my playthroughts I don't think I got to vorpalize weapons often enough to trigger that.

[jwt27]

Seems a strange oversight from Rogue devs. I'm usually reluctant of changing anything in the original mechanics or gameplay, even bugs, but a segfault should be addressed.

I like the idea of using wand_mons in case of *cp == ' ', but it's a bit brittle as it relies on the convenient fact that wand_mons has monsters on every space of lvl_mons.

I agree that you wouldn't use such a workaround on new code, but in this case, we don't expect those strings to change ever again. If you want gameplay to stay 100% faithful to the original release, bugs and all, you could add a bogus entry to monsters with a similarly garbled name (from what I remember, it changes every level). I think that's going a bit too far :)

Also, as cp is a pointer and not an index, the implementation of peeking the same index on another string requires substantial change to that function.

I'll push what I had in mind. If you don't want to change too much, the fix could be as simple as: if (*cp == ' ') return wand_mons[cp - lvl_mons];

Another solution is to use the existing fallback for Medusas to include the *cp == ' ' case. As it is, it currently only triggers if all rnd(10) checks fail, but it's possible that the original intention was for that to be a general fallback for all fail cases.

So I believe if (cp < lvl_mons || *cp == ' ') return 'M'; is the closest to the original intention. Yes, it would dramatically increase the odds of Vorpalizing Medusas, but it's fair to assume this was/could be intentional.

That's a possible solution, but it seems unlikely to me that was the original intent. Ideally we'd find some other port where this bug has been noticed and fixed. I think the vorpal scroll is unique to the PC version though.

If you look at the BSD Rogue 3.6 source (https://britzl.github.io/roguearchive), it doesn't have this scroll, but lvl_mons also does not have any spaces. My suspicion is that the spaces were added at a later point, after the vorpalize scroll was implemented, and it just didn't receive enough playtesting to trigger the bug.

[jwt27]

I ran "Rogue 1.0 by Jon Lane" through a disassembler.

This version already has the Vorpal scroll, and lvl_mons is defined as "KEBSHIORZLCAQNYTWFPUGMXVJD", no spaces!

This proves that the scroll was originally intended to target all monsters, so I believe we should replicate this behavior.

pick_mons() looks as follows: (identical to what we have now)

seg000:7379 ; ¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦ S U B R O U T I N E ¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦
seg000:7379
seg000:7379 ; Attributes: bp-based frame fpd=4
seg000:7379
seg000:7379 pick_mons       proc near               ; CODE XREF: sub_A3AF:loc_A90Bp
seg000:7379
seg000:7379 cp              = word ptr -2
seg000:7379
seg000:7379                 push    bp
seg000:737A                 sub     sp, 4
seg000:737D                 mov     bp, sp
seg000:737F                 push    p_lvl_mons
seg000:7383                 call    strlen
seg000:7386                 mov     sp, bp
seg000:7388                 mov     bx, p_lvl_mons
seg000:738C                 add     bx, ax
seg000:738E                 mov     [bp+4+cp], bx
seg000:7391
seg000:7391 loc_7391:                               ; CODE XREF: pick_mons+30j
seg000:7391                 mov     ax, [bp+4+cp]
seg000:7394                 dec     ax
seg000:7395                 cmp     ax, p_lvl_mons
seg000:7399                 mov     [bp+4+cp], ax
seg000:739C                 jb      short loc_73AB
seg000:739E                 mov     ax, 10
seg000:73A1                 push    ax
seg000:73A2                 call    rnd
seg000:73A5                 mov     sp, bp
seg000:73A7                 test    ax, ax
seg000:73A9                 jnz     short loc_7391
seg000:73AB
seg000:73AB loc_73AB:                               ; CODE XREF: pick_mons+23j
seg000:73AB                 mov     ax, [bp+4+cp]
seg000:73AE                 cmp     ax, p_lvl_mons
seg000:73B2                 jnb     short loc_73BC
seg000:73B4                 mov     ax, 'M'
seg000:73B7                 add     sp, 4
seg000:73BA                 pop     bp
seg000:73BB                 retn
seg000:73BC ; ---------------------------------------------------------------------------
seg000:73BC
seg000:73BC loc_73BC:                               ; CODE XREF: pick_mons+39j
seg000:73BC                 mov     si, [bp+4+cp]
seg000:73BF                 mov     al, [si]
seg000:73C1                 xor     ah, ah
seg000:73C3                 add     sp, 4
seg000:73C6                 pop     bp
seg000:73C7                 retn
seg000:73C7 pick_mons       endp ; sp =  4

[MestreLion]

I agree that you wouldn't use such a workaround on new code, but in this case, we don't expect those strings to change ever again.

True. But still...

If you want gameplay to stay 100% faithful to the original release, bugs and all, you could add a bogus entry to monsters with a similarly garbled name (from what I remember, it changes every level). I think that's going a bit too far :)

Yup. Worse than adding code to fix a bug would be to add code to simulate a bug :)

If you don't want to change too much, the fix could be as simple as: if (*cp == ' ') return wand_mons[cp - lvl_mons];

Wow, that was clever! I'm ashamed I didn't see how simple that approach would be.

Ideally we'd find some other port where this bug has been noticed and fixed. I think the vorpal scroll is unique to the PC version though. If you look at the BSD Rogue 3.6 source (https://britzl.github.io/roguearchive), it doesn't have this scroll, but lvl_mons also does not have any spaces. My suspicion is that the spaces were added at a later point, after the vorpalize scroll was implemented, and it just didn't receive enough playtesting to trigger the bug.

I truly appreciate all the investigation you're doing... that's awesome! It's the very spirit I envisioned for this humble project, not only the challenge of porting a 16-bit DOS full of ASMs to modern platforms but specially the archaeological care to preserve and understand a historical artifact.

[MestreLion]

I ran "Rogue 1.0 by Jon Lane" through a disassembler. This version already has the Vorpal scroll, and lvl_mons is defined as "KEBSHIORZLCAQNYTWFPUGMXVJD", no spaces! This proves that the scroll was originally intended to target all monsters, so I believe we should replicate this behavior.

That's a fantastic finding! We now have a strong reason to go with your original idea of simply skipping the spaces. Great!

[sunfall]

Note that just skipping spaces to go to the next letter will favor that letter, as it could be picked either directly or via the space-move. You'll want your solutions to target each non-space with equal probability. (I haven't looked that closely at the current proposed solution, I'm just an interested bystander.)

[MestreLion]

Oh, I see... simply skipping the spaces would also skip some monsters, so a solution that includes them all is bounded to combine lvl_mons with wand_mons. Hum, tough call. I hate to be the one to introduce a workaround.

If we go with that, why not use your two line if (*cp == ' ') return wand_mons[cp - lvl_mons]; right before the final return? It's clever, elegant, and much less obtrusive then the current PR.

Also, regardless of the approach, I believe the workaround must be more thoroughly documented, be it in code, commit message or preferably both. In code it could still be succinct, something like:

    if (cp < lvl_mons)
        return 'M';

    /*@
     * This check was not in original, causing a bug.
     * Spaces in lvl_mons were introduced after Vorpalize Scrolls,
     * so it's believed the bug was an oversight
     */
    if (*cp == ' ')
        return wand_mons[cp - lvl_mons];

    return *cp;

[MestreLion]

The commit message could be more verbose, following git's guidelines: 1st line as short subject header (as it is right now), then a blank line, and then more lines/paragraphs as needed to explain the problem, the solution taken, and the reasoning for that. English is not my native language, so I'd rather not suggest any wording, but it should at least mention the bug was in the original, it triggered when a space was chosen, spaces were arguably added to lvl_mons later in development, and to keep all monsters being "vorpalizable" (ewwww) we had to resort to wand_mons.

[MestreLion]

Note that just skipping spaces to go to the next letter will favor that letter, as it could be picked either directly or via the space-move. You'll want your solutions to target each non-space with equal probability. (I haven't looked that closely at the current proposed solution, I'm just an interested bystander.)

A much welcome bystander my dear friend! Glad you're interested in this historical recreation!

PS: Current proposal is to effectively consider a string with no spaces and all monsters, KEBSHIORZLCAQNYTWFPUGMXVJD, as it was when the vorpalize scroll was introduced.

[jwt27]

Oh, I see... simply skipping the spaces would also skip some monsters, so a solution that includes them all is bounded to combine lvl_mons with wand_mons. Hum, tough call. I hate to be the one to introduce a workaround.

Another alternative maybe, we could re-introduce the monsters string without spaces:

static char *vorp_mons = "KEBHISORZLCAQNYTWFPUGMXVJD";
static char *lvl_mons =  "K BHISOR LCA NYTWFP GMXVJD";
static char *wand_mons = "KEBHISORZ CAQ YTW PUGM VJ ";

but...

If we go with that, why not use your two line if (*cp == ' ') return wand_mons[cp - lvl_mons]; right before the final return? It's clever, elegant, and much less obtrusive then the current PR.

... I do prefer this option.

Also, regardless of the approach, I believe the workaround must be more thoroughly documented, be it in code, commit message or preferably both. The commit message could be more verbose, following git's guidelines: 1st line as short subject header (as it is right now), then a blank line, and then more lines/paragraphs as needed to explain the problem, the solution taken, and the reasoning for that. English is not my native language, so I'd rather not suggest any wording, but it should at least mention the bug was in the original, it triggered when a space was chosen, spaces were arguably added to lvl_mons later in development, and to keep all monsters being "vorpalizable" (ewwww) we had to resort to wand_mons.

Good call, I'll try and make sure it's thoroughly documented. This is always the most difficult part of programming :)

[MestreLion]

Another alternative maybe, we could re-introduce the monsters string without spaces:

static char *vorp_mons = "KEBHISORZLCAQNYTWFPUGMXVJD";
static char *lvl_mons =  "K BHISOR LCA NYTWFP GMXVJD";
static char *wand_mons = "KEBHISORZ CAQ YTW PUGM VJ ";

but...

That's actually a very good idea. More obtrusive but less brittle. And it keeps pick_mons() logic intact (apart from renaming lvl_mons to vorp_mons)

If we go with that, why not use your two line if (*cp == ' ') return wand_mons[cp - lvl_mons]; right before the final return? It's clever, elegant, and much less obtrusive then the current PR.

... I do prefer this option.

I'm fine with either way, it's your call. In any case it would be nice to add a note on the strings declaration, be it either:

//@ vorp_mons not in original, keep it consistent with lvl_mons and wand_mons, see pick_mons()
static char *vorp_mons = "KEBHISORZLCAQNYTWFPUGMXVJD";
static char *lvl_mons =  "K BHISOR LCA NYTWFP GMXVJD";
static char *wand_mons = "KEBHISORZ CAQ YTW PUGM VJ ";

or

//@ wand_mons must be "complementary" and consistent with lvl_mons regarding spaces and ordering,
//@ See pick_mons() which relies on that.
static char *lvl_mons =  "K BHISOR LCA NYTWFP GMXVJD";
static char *wand_mons = "KEBHISORZ CAQ YTW PUGM VJ ";

Anyway, you got the idea

Good call, I'll try and make sure it's thoroughly documented. This is always the most difficult part of programming :)

Agree :)

Well, it's nothing you haven't done already when writing this PR. You could actually hand-pick some sentences from your comments here and that's more than enough material for the commit message. I loved the way you disassembled Rogue 1.0 and tracked down BSD's Rogue 3.6 source

[sunfall]

Another alternative maybe, we could re-introduce the monsters string without spaces:

static char *vorp_mons = "KEBHISORZLCAQNYTWFPUGMXVJD";
static char *lvl_mons =  "K BHISOR LCA NYTWFP GMXVJD";
static char *wand_mons = "KEBHISORZ CAQ YTW PUGM VJ ";

Again: just a bystander, but this is an excellent solution, particularly if they're next to each other and well documented, because it immediately tells by looking what's going on. What are targets for vorpalization? All of these. For wands? Well, not these. That's nice and coherent and comprehensible, even into the future where clever solutions may not be so well-understood by someone coming behind us.

(Source: having to actively maintain a 10+ year old critical codebase written in C++ where no original authors remained.)

[MestreLion]

Another alternative maybe, we could re-introduce the monsters string without spaces:

static char *vorp_mons = "KEBHISORZLCAQNYTWFPUGMXVJD";
static char *lvl_mons =  "K BHISOR LCA NYTWFP GMXVJD";
static char *wand_mons = "KEBHISORZ CAQ YTW PUGM VJ ";

Again: just a bystander, but this is an excellent solution, particularly if they're next to each other and well documented, because it immediately tells by looking what's going on. What are targets for vorpalization? All of these. For wands? Well, not these. That's nice and coherent and comprehensible, even into the future where clever solutions may not be so well-understood by someone coming behind us.

(Source: having to actively maintain a 10+ year old critical codebase written in C++ where no original authors remained.)

That's a very good remark @sunfall, thanks ! Always appreciated your wisdom since my early days tinkering with Singularity!

Btw... wand_mons stands for wandering monsters, i.e. which (new) monsters are not created once at level creation (that's lvl_mons), but rather spawn afterwards (and periodically) while you wander through that level.

Very poor word choice from Rogue Devs, specially considering a game that has wands, which highlights exactly the point you made :)

[sunfall]

I couldn't have picked a better example of the exact problem I was describing if I tried, apparently.

[MestreLion]

@jwt27 , the sage has spoken, so vorp_mons it is!

Go ahead, and don't forget to include some succinct comments in code, such as:

/*@
 * vorp_mons not in original, introduced here as the previous version of lvl_mons
 * with all monsters and no spaces, fixing a bug in pick_mons()
 */
static char *vorp_mons = "KEBHISORZLCAQNYTWFPUGMXVJD";
...

/*
 * pick_mons:
 *  Choose a sort of monster for the enemy of a vorpally enchanted weapon
 * 
 * @ originally used lvl_mons instead of vorp_mons, causing a bug when it changed to contain spaces
 */
char
pick_mons(void)
{
...

[jwt27]

Thanks for the merge! Nice to finally have this old bug resolved.

[MestreLion]

Thanks for the merge! Nice to finally have this old bug resolved.

36 years, old bug indeed!

But hey, I'm the one that thanks you! Truly appreciate the report and fix!