Traps, take five (or thereabouts)

[AnonymousHero]

Okay, so, the traps code is not only entangled with Rogues' monster traps, but also with searching, hidden doors, and chest traps.

I am considering removing all of them.
- Searching and hidden doors are pretty useless, especially without traps, and don't add much flavor
- Chest traps are very, very heavily dependent on floor trap code AFAICT

I will try to preserve them for now though.

(But yes, it would be very nice indeed to yank out all that code!)

Edit: I'll keep trapped chests, hidden doors, and searching for now. Monster traps will be gone though.

Edit: or not, because t_info includes chest traps too. BLARGH.

Yes -- welcome to the world of the traps code . It's all very tangled and messy.

I think maybe we could go in steps here... perhaps removal of the various non-Rogue trap effects would be a decent first step? (I.e. removing the "swap items" and "drop everything" effects, and such.)

[Lord Estraven]

FWIW I have it compiling now - just a few link errors. I should be able to commit something pretty shortly.

Searching and hidden doors will still be there (albeit stupid and useless) for now. Chest traps will be gone.

Edit: alright, it builds and links. Time to see if it runs.

Edit: ooh nice, there's a buffer overflow. Thank you libasan! Gotta fix that first...

Edit: oh dear. r_info_flags6 needs a placeholder. It's size-dependent. YAY.

Edit: dependent on index too by the looks of it. WTH?

Edit: in any case, there we go, revision #1 compiles and runs without crashing. YAY. Will commit and push to the new branch in a minute.

[Lord Estraven]

Alright. Compiles, links, and plays. Pushed to Gitlab and merge request filed.

[Lord Estraven]

Whoops, I just realized I forgot to get rid of Disarming skill. And Sneakiness. And Create Traps.

I'll leave that to my future updates I guess...

[Lord Estraven]

On further inspection, something is very wrong here - some kind of index error I think. Items' intrinsic to-hit bonuses seem to be getting turned into AC bonuses. Locating this will probably be hard, but I'll take a look.

Edit: yeah, everything's off by one I think? To-hit is getting turned into to-dam, and to-dam is getting turned into AC. Sigh.

[Lord Estraven]

Okay - I'm belatedly getting back to this. GDB should let me figure out where things are going wrong, if I can just guess which function is involved. Hopefully I'll have an updated push request by the end of today.

Edit: looks like "good" weapons are getting a_am_aux_2 (for armor) instead of a_m_aux_1 (for weapons). Not sure why yet.

[Lord Estraven]

... I don't think I introduced this bug, actually. In fact, I suspect it is very old indeed, and was just being masked by some other bug.

Code: Select all

       case TV_HAFTED:
        case TV_POLEARM:
        case TV_MSTAFF:
        case TV_SWORD:
        case TV_AXE:
        case TV_BOOMERANG:
        case TV_BOW:
        case TV_SHOT:
        case TV_ARROW:
        case TV_BOLT:
        case TV_DAEMON_BOOK:
                {
                        /* UGLY, but needed, depending of sval teh demon stuff are eitehr weapon or armor */
                        if (o_ptr->sval == SV_DEMONBLADE)
                        {
                                if (power) a_m_aux_1(o_ptr, lev, power);
                        }
                        else
                        {
                                if (power) a_m_aux_2(o_ptr, lev, power);
                        }
                        break;
                }

Those are reversed! The first should be a_m_aux_2, the second a_m_aux_1.

(And this, of course, is why functions should have descriptive names. )

Edit: yup, reversing those works. I'll create another branch and another pull request.

Edit 2: the above is in object2.cc[/i] FYI.

[Lord Estraven]

@AnonymousHero

I actually can't file a pull request at the moment, because the GitLab page for doing so is 404 (!). In any case this is any easy fix, I figure you can make the change when you next have time.

[AnonymousHero]

Code: Select all

       case TV_HAFTED:
        case TV_POLEARM:
        case TV_MSTAFF:
        case TV_SWORD:
        case TV_AXE:
        case TV_BOOMERANG:
        case TV_BOW:
        case TV_SHOT:
        case TV_ARROW:
        case TV_BOLT:
        case TV_DAEMON_BOOK:
                {
                        /* UGLY, but needed, depending of sval teh demon stuff are eitehr weapon or armor */
                        if (o_ptr->sval == SV_DEMONBLADE)
                        {
                                if (power) a_m_aux_1(o_ptr, lev, power);
                        }
                        else
                        {
                                if (power) a_m_aux_2(o_ptr, lev, power);
                        }
                        break;
                }

Those are reversed! The first should be a_m_aux_2, the second a_m_aux_1.

It actually even worse than that: The if-check completely ignores the TVAL, and so an object with any of the above TVALs and which happens to have SVAL == SV_DEMONBLADE gets treated with the "workaround". The quality of the T2 code is very, very dubious overall.

If you can show a quick way to reproduce, I'd be happy to fix this.

[Lord Estraven]

Blargh! Sorry for the oversight there.

To reproduce, just checkout and compile my 'v2.4-trap-removal' branch:
https://gitlab.com/miramor/tome2/tree/v2.4-trap-removal

For some reason, "good" weapons in that branch reliably get AC bonus (rather than to-hit/to-dam) unless the above is fixed.

[AnonymousHero]

Hang on... that's a bug you've introduced yourself. (EDIT: ... which kind of makes sense. I was having a hard time imagining how such an obvious bug should creep through.)

The original code in v2.4 reads:

Code: Select all

	...
	case TV_SHOT:
	case TV_ARROW:
	case TV_BOLT:
	case TV_TRAPKIT:
		{
			if (power) a_m_aux_1(o_ptr, lev, power);
			break;
		}

	case TV_DAEMON_BOOK:
		{
			/* UGLY, but needed, depending of sval teh demon stuff are eitehr weapon or armor */
			if (o_ptr->sval == SV_DEMONBLADE)
			{
				if (power) a_m_aux_1(o_ptr, lev, power);
			}
			else
			{
				if (power) a_m_aux_2(o_ptr, lev, power);
			}
			break;
		}

IOW, you've removed too much code. Fall-through results in the bug.

Btw, not sure why you should be having problems with Qt Creator's (3.5 and 3.6) Find Usages... it works almost flawlessly for me. It does seem to sometimes have a little trouble with macros (#define'd values, mostly), but you can Ctrl-click through to the macro definition and use Find Usages on that instead to get it working.

[Lord Estraven]

D'oh! That's what I get for deleting stuff on automatic. Thanks @AnonymousHero.

[AnonymousHero]

I've left a couple of comments on the gitlab PR page, but don't bother uploading a new version of the PR just yet.

I think I've just about decided to move the repository to GitHub sometime soon (next day or so, at a guess). Unfortunately, it appears that GitHub is basically the only viable option right now. (Too many unexplained weird issues and generally incredible slowness of GitLab means that it's annoying to use.)

I'll see if it's feasible to at auto-mirror it to Gitlab or something, but development will take place on GH once the move it complete. I'll announce in a new thread + the maintenance thread once it's done.

EDIT: Moved, but there'll be no mirroring. (See the other post for details.)

Feel free to submit a new PR against the new repository. You can should be able to do copy all your changes across by creating a repository on GH, adding that as a remote in your current checkout ("git remote add github git@github.com:/....") and pushing your branch to that remote.

[Lord Estraven]

So, an update: I managed to ditch large portions of trap-related code in ToME, on one of my branches. One thing I neglected though, was that Theme still had all kinds of trap stuff. This proved difficult to get rid of.

That was several weeks ago... I'm still thinking about how this stuff might be handled decently. I don't think it can be done using skills, at least not in a balanced or elegant way.

Best way might be something stupidly simple. Like, for instance, making levitation/flying confer total immunity to traps. That is inconsistent vs. monster traps, but whatever; I'll test it out in a branch I guess.

[Lord Estraven]

Okay, trap removal take #3. Yanked all the trap code; got rid of all the item-related crashes I could find; and rearranged the skill tree to make sense again.

The (currently) final result is in the rm-traps branch:

https://github.com/miramor/tome2/tree/rm-traps

Hope that is sufficient for now.