Track RandomSpawner origin of replaced things

[wallabra]

This only applies to RandomSpawners at the moment.

This allows mods to see the class of the original map thing that eventually led to a thing being spawned, to better interface with randomizer mods and the like.

A good usage example can be seen here: if a Zombieman is replaced by a RandomSpawner capable of producing 5 other variants by a replacer mod using RandomSpawner, this change in AI Director would allow it to see the original RandomSpawner and guarantee access to all 5 variants, even in a level with only 1 initial Zombieman (so other types could spawn on, e.g., >100% start spawn factor, or in continuous spawns).

[wallabra]

This PR would produce this gzdoom.pk3 (renamed to ZIP because GitHub), which has been crafted to be monkeypatched into existing GZDoom v4.12.2m installations (for testing, etc):
gzdoom.zip

[RicardoLuis0]

and is there a need to add a new field to actor just for this? IIRC you can iterate over map-placed random spawners on WorldLoaded, they only repace during the first tick on PostBeginPlay (BeginPlay isn't called on map-placed things)

[wallabra]

That sounds like boilerplate that would have to be repeated for every mod interested in tracking RandomSpawner cause-effect chains. I agree there probably is a cleaner way, though...

[wallabra]

I'll go think about that and come back with a better idea. Thank you! :)

[RicardoLuis0]

I'll go think about that and come back with a better idea. Thank you! :)

might be a good idea to use an event instead, ex. ActorReplaced(class<Actor> original, Actor replacement)

[wallabra]

I'll go think about that and come back with a better idea. Thank you! :)

might be a good idea to use an event instead, ex. ActorReplaced(class<Actor> original, Actor replacement)

Maybe, but mods still have to track what class each actor replaced, which also gets complicated when an actor has a multi-step replacement chain (e.g. due to RandomSpawner), vastly increasing code complexity on the modder's end.

That is, unless ActorReplaced somehow keeps track of which type an actor was on map load (the first type replaced by it, rather than the type it is replacing immediately), which in the engine end would probably done by a tracking variable like originSpawner... and we're back to square one!

In the end, this really involves adding related state to track what type each actor comes from, which must transcend intermediary steps like RandomSpawner which would otherwise erase the cause-effect chain history. Adding a new variable to Actor for such a niche purpose seems overkill, there's probably a better way, maybe an associated object, or a separate singleton object with an interior dictionary-esque data structure.

Ew singletons, I know, but keep in the barf, it doesn't sound like that bad of an idea here. Singletons are surprisingly useful in ZScript. Besides, it would keep a clear separation between the well-established Actor code and this somewhat niche state tracking, which also helps with partial implementations and other compatibility-related ideas. Heck, it could be monkeypatched to older versions using an EventHandler.

[RicardoLuis0]

That is, unless ActorReplaced somehow keeps track of which type an actor was on map load (the first type replaced by it, rather than the type it is replacing immediately), which in the engine end would probably done by a tracking variable like originSpawner... and we're back to square one!

yes, but that tracking can be done in randomspawner itself instead of in actor, making it much lighter and simpler

[wallabra]

That is, unless ActorReplaced somehow keeps track of which type an actor was on map load (the first type replaced by it, rather than the type it is replacing immediately), which in the engine end would probably done by a tracking variable like originSpawner... and we're back to square one!

yes, but that tracking can be done in randomspawner itself instead of in actor, making it much lighter and simpler

I assume RandomSpawner instances are destroyed immediately after map load, and thus would be inaccessible from the perspective of, say, EventHnadler::WorldThingSpawner. While I agree that would be a cleaner solution, these instances would have to linger. Ideally it would be clear that they are in a "consumed" state.

[RicardoLuis0]

I assume RandomSpawner instances are destroyed immediately after map load, and thus would be inaccessible from the perspective of, say, EventHnadler::WorldThingSpawner. While I agree that would be a cleaner solution, these instances would have to linger. Ideally it would be clear that they are in a "consumed" state.

The order goes: WorldLoaded -> PostBeginPlay -> WorldThingSpawned, and map-placed RandomSpawners do the replacing in PostBeginPlay so you can check them before they do any replacing if you do it from WorldLoaded (dynamically created RandomSpawners do their replacing in BeginPlay, so there isn't really a way to get in between there unfortunately, but that can still be fixed like i said with an ActorReplaced event that works directly with the random spawners, instead of adding fields to actors -- i'm not against a way to detect replacement of actors via RandomSpawners, i just do not think doing it via tracking fields in every actor is a good way to go about it)

[wallabra]

That's fair, I'm also thinking about changing the RandomSpawner code to update a singleton object.

[wallabra]

(Tests pending)

[RicardoLuis0]

i still don't think this is a good solution -- any tracking can and should be done only by mods that need the relevant data, all that's needed by the engine is an event to signal when replacements happen

[wallabra]

The engine is losing information about map start if it doesn't. I don't know if such behaviour should be left up to mods. This could be made an optional feature.

[RicardoLuis0]

the engine doesn't need that information, so if a mod needs it, it can just save the information itself

[wallabra]

Fair enough. I'll probably just make an event for when a mobj is replaced, and I'll move the tracking code elsewhere.

I still want it to be in some sort of optional utils library, to reduce the amount of duplicate code across mods. An engine-side ZScript module dependency resolver would be cool.

[Boondorl]

I'm not sure I understand the use case for this. Is it solely to detect if something was spawned by a RandomSpawner? The example makes it seem like getting the RandomSpawner's list of monsters is desired, but there are much more trivial ways to do this such as iterating through all classes and storing classes marked as a monster that aren't replaced. This is something the mod itself should be handling in my opinion and not something baked into the engine.

[RicardoLuis0]

I'm not sure I understand the use case for this. [...] The example makes it seem like getting the RandomSpawner's list of monsters is desired.

It's the opposite -- it's for getting the original randomspawner class that created a particular actor

[Boondorl]

It's the opposite -- it's for getting the original randomspawner class that created a particular actor

That was my take away, but it seemed they wanted this so they could get the rest of the monsters the spawner has. My thoughts are that these kinds of mods should simply be looking through the Actor classes for valid monsters, which can be done by getting its default, checking bIsMonster, and seeing if something replaces it. This kind of system can be extrapolated to any kind of Actor type e.g. items. Relying on any kind of RandomSpawner is bad practice because their behavior can be arbitrarily changed which is impossible to account for in a system like this.

[wallabra]

It's the opposite -- it's for getting the original randomspawner class that created a particular actor

That was my take away, but it seemed they wanted this so they could get the rest of the monsters the spawner has. My thoughts are that these kinds of mods should simply be looking through the Actor classes for valid monsters, which can be done by getting its default, checking bIsMonster, and seeing if something replaces it. This kind of system can be extrapolated to any kind of Actor type e.g. items. Relying on any kind of RandomSpawner is bad practice because their behavior can be arbitrarily changed which is impossible to account for in a system like this.

This seems to ignore the fact that a mod might want to see what kind of monsters (or even non-monster classes, like Items) could possibly be spawned within a level in its initial configuration, not every class there is as a whole.

[Boondorl]

Then the CheckReplacement() event can be used. Check to see if the replacement is a spawner and if it is, store any necessary information from there.