Virtualize all canonical monster-specific attacks

[Graf Zahl]

We cannot gratuitously change how those functions work without breaking stuff. It doesn't really matter if you are inconvenienced by it, it is magnitudes more important to keep old stuff working.

[gramps]

I understand the importance of keeping old stuff working. I'm not suggesting breaking it. Although, I suppose deprecating missiletype also broke old stuff, didn't it? But I understand if the oldest stuff takes precedence.

All I'm suggesting is that it's worth looking into ways of doing this that don't sanctify using large chunks of copypasted code as the go-to way to change a missiletype. Not because it's inconvenient, but because it promotes poor practices, the exact kind of thing the wiki warns strongly against on nearly every page: "Wait! Don't copy this giant thing into your mod!"

But if there's no other way to do it, there's no other way.

[Xaser]

Going back to the OP (key word "virtualize"), what about DEHACKED support is preventing marking these functions as `virtual`, exactly?

[Enjay]

Although, I suppose deprecating missiletype also broke old stuff, didn't it?

Probably not. MissileType still works.

Deprecating, at least in GZDoom, means "there is now a better way to do this, so don't use the old way, it is no longer supported". The old way is left as functional as possible (basically as functional as it was just before deprecation) but will no longer be supported in terms of additional features or refinements. It will usually even be maintained to the point that, where possible, its functionality will be preserved unless doing so actively inhibits a newer, better feature. Most deprecated features still work just as well as they ever did - but there are no guarantees that they will continue to do so. This does not mean, of course, that they should be used for new mods, merely that old mods that did use them are unlikely to break.

[Graf Zahl]

Going back to the OP (key word "virtualize"), what about DEHACKED support is preventing marking these functions as `virtual`, exactly?

These functions can be used everywhere and are guaranteed to always do the same thing. If that can no longer be guaranteed we have a mess, not a feature.
Even something as banal as having a monster call both A_MissileAttack and one of the hard coded functions to fire a stock missile is something I have seen.
If you now change how e.g. A_TroopAttack works based on the MissileType property, not even having DoomImpBall as a default would be guaranteed to always do the right thing.

If you change how this function works you create error cases. And that's the end of the story.

[gramps]

@Enjay: Hmm, maybe I'm mis-remembering, but I thought in the old days, you could extend a DoomImp actor, give it a MissileType of PlasmaBall, and voila, it shoots plasma balls now. Is that not how it worked before? It certainly doesn't work like that anymore, if I'm not mistaken, because I can't simply define MissileType as a default property of an Actor at all, it's no longer recognized. And even if I could, its missile type is clearly hard-coded now.

As for Xaser's suggestion, it sounds like what's being suggested here is that the dehacked part of the code would always call the base A_TroopAttack, and never the overridden one. But again I have no idea if that's possible, or at least not a giant pain.

[Enjay]

@Enjay: Hmm, maybe I'm mis-remembering, but I thought in the old days, you could extend a DoomImp actor, give it a MissileType of PlasmaBall, and voila, it shoots plasma balls now. Is that not how it worked before? It certainly doesn't work like that anymore, if I'm not mistaken, because I can't simply define MissileType as a default property of an Actor at all, it's no longer recognized. And even if I could, its missile type is clearly hard-coded now.

My recollection is that it was always intended to be used in conjunction with pointers such as A_MissileAttack and A_ComboAttack which were not given any parameters directly but used the MissileType property instead (and that certainly still works). I don't *think* it could be used to override pointers such as A_TroopAttack with their hard coded missile types, but I'm prepared to be corrected on that.

Of course, all that was in DECORATE. I have no idea if MissileType can be used as a default property in ZScript but, given that would essentially mean extending its functionality, it wouldn't surprise me if it didn't work.

[gramps]

Well, that would explain a lot, I suppose it would have had the same issues back then as it does now. My reading of the OP suggested that it did used to work like that, and then I thought I remembered it working like that as well, but maybe not.

About OP / Xaser's suggestion, even if dehacked could safely call the original function in the base class while allowing overrides on subclasses, you'd still need to copypaste the entire attack function in order to change that string literal; all it would save is copying part of the state block to call your new function. So I'm not sure of the value in that really, which is why I went off on the MissileType tangent in the first place.

[Matt]

My reading of the OP suggested that it did used to work like that

Come to think of it I was probably wrong about that. It was an afterthought example that occurred to me at the last moment of typing.

[Xaser]

Yeah, I'm really not following. Everyone's still focusing on the "use missiletype" non-sequitur -- that's not at all what was actually being suggested in the title/OP.

If, say, A_TroopAttack is virtual, and you do the following two things:

• Extend HellKnight and make him fire a salvo of imp fireballs with A_TroopAttack
• Extend DoomImp and override A_TroopAttack to make it fire a rocket

...then the Hellknight fires Imp balls while the Imp shoots rockets, just as expected. A_TroopAttack behaves exactly as it always has, except in the case where it's explicitly told to do something different in a derived class.

[Graf Zahl]

Action functions cannot be virtual which kills this right away.

[gramps]

How is that really useful anyway? "Extend DoomImp and override A_TroopAttack to make it fire a rocket" still involves copying the entire A_TroopAttack function into your extended DoomImp.

Is this just about not wanting to copy part of the state block to call a renamed A_TroopAttack2? That doesn't seem particularly important if you're already copying that entire function anyway.

Granted, there's not much going on in A_TroopAttack, but by the time you're copying the mancubus attack or something, copying a few lines of a state block is basically nothing in comparison. Manc's missile state is 8 LOC, but the functions you'd need to copy are about 70 LOC. Archvile's another nasty one, 7 LOC in missile state, about 50 LOC of copied functions.

Point being, avoiding copying a small part of the state block seems like going after the wrong thing here. Something interesting to look at is the pain elemental. The function called from his state block is very short, because he's organized in a more granular way; the A_PainAttack function from the state block actually delegates all the work to a much longer A_PainShootSkull function. But you don't need to copy that, because it's not called directly from the state block. If some of the other monsters were organized like that, that would cut down a whole lot on the need for cargo-coding copypasta. Maybe something like that would be a more reasonable idea for a feature suggestion?

[Graf Zahl]

You are missing something here: All A_FatShot functions and A_PainAttack have a parameter for the missile type, and so does A_VileTarget for the flame. A_VileAttack is even fully configurable. The simple ones like A_TroopAttack are not worth it because they are far too simple and can be done with generic functions instead. If it wasn't for Dehacked backwards compatibility they would have been deleted long, long ago.

[gramps]

Ahh, I was totally missing that!

So the entire issue here is wanting to avoid copying ~8 LOC in a state block, then? That doesn't seem like too much of a burden at all, honestly.

I mean, I guess the imp could get that parameter too, even though it's simple, just so we don't have to look up the the details of its attack to be sure we set it up properly. But that'd be an issue for a separate ticket.

[Xaser]

Action functions cannot be virtual which kills this right away.

True, though interestingly, the legacy monster attack functions (or at least the few I poked around at, like A_TroopAttack) don't seem to be marked "action" anymore -- I guess it's not needed since none of them use the invoker pointer.

How is that really useful anyway? "Extend DoomImp and override A_TroopAttack to make it fire a rocket" still involves copying the entire A_TroopAttack function into your extended DoomImp.

The vast majority of use cases, I imagine, are covered by calling one of the generalized attack functions, like so:

Code: Select all

class RocketImp : DoomImp replaces DoomImp
{
    override void A_TroopAttack()
    {
        A_SpawnProjectile("Rocket");
    }
}

The old attack functions do have all their logic inline (probably a "why fix what's not broken?" kinda thing), but pretty much all of them have a generalized version somewhere -- e.g. if you want the Imp's "melee if the player is too close" behavior, there's A_CustomComboAttack, and so forth.