Func_itemremove: fix or get rid?

[SteveL]

This came up in the newbie questions thread in July. There's a entity called "func_itemremove" available in DR which advertises that it removes an item from the player's inventory, but it doesn't work. So we should either fix it, or get rid of it so that it doesn't show up in DR and cause confusion. I'll pick up the tracker issue, but I'm not sure which to do. Do people want to use it?

 

Here's the previous discussion:

From what I can tell the func_itemremove is busted and never works.

It looks like you're right. I can't find any code for it.

I was about to offer to pick that one up -- a quick enough fix -- except that it's not at all clear what the intended behaviour was.

• Remove the item from the game?
  
• Drop it at the player's feet?
  
• In the case of stackables, remove 1 item or all of them?
  

And the editor tip mentions objectives, which have their own management system. They're not inventory items.

There was no tracker issue for func_itemremove so I've raised one: http://bugs.thedarkm...iew.php?id=3784

You could have a spawnarg "droptofloor" or something similar and if its set to 1 the item drops to the floor, if its set to 0 (default) it deletes it entirely out of the game. To solve the issue of stackables also use a spawnarg "quantitytoremove" or something similar and once again the player can set how many get removed when the entity is triggered.. I would say by default set it to 1 and yeah.

 

And thank you to everyone who helped with the script ideas you guys are always so helpful!

 

As Goldwell points out, we could support all the options though spawnargs, so we don't need to make a decision on which behaviour to implement. But it's worth doing only if people will find it useful. Does anyone want it?

[nbohr1more]

I recall some earlier discussion about a func_remove meant to cleanup the script interpreter stack, maybe the use case is wrong here?

[nbohr1more]

Nope, I'm wrong. This should work as advertised as it was part of Doom 3. I wonder if item management code in TDM is just too divergent and this broke

somewhere. Seems like a handy thing to have though.

[SteveL]

The original intent was as discussed. Here are the editor prompts from the entity def:

"editor_usage" "Removes an item from the players inventory"
"editor_var remove" "name of the inventory item to remove.. i.e. objective, or key_red"

 

Someone created that def but never got round to implementing it.

 

What's up with the interpreter stack?

 

Edit: crossed with your post #2

Edited September 2, 2014 by SteveL

[nbohr1more]

As I recall (vaguely) when you spawn script actions the script interpreter keeps them resident in it's stack until you tell it to "remove" them.

Tels can probably clarify this further since he was the one who discovered this behavior.

[SteveL]

Seems like a handy thing to have though.

I think you're right, given that at least 2 people tried it recently, to start the discussion quoted above. The correct answer was probably "just fix it".

[Tels]

I don't remember how or if it was implemented. There are a few possibilities:

 

* this was a special entity implemented in C++, and the code code lost somehow

* it was just a script and the code was lost

* it was never implemented

 

But finding out which it actually was is a bit moot.

 

As there is a script event

 

scriptEvent float			   replaceInvItem(entity oldItem, entity newItem);

 

this could probably be implemented in a probably 10 line script object, and a matching 10 line entityDef. Including the "droptofloor".

[SteveL]

I don't remember how or if it was implemented. There are a few possibilities:

 

* this was a special entity implemented in C++, and the code code lost somehow

* it was just a script and the code was lost

* it was never implemented

 

I did a bit of code archeology at lunchtime. nbohr1more was right: this was a doom3 thing. It was implemented through an idItemRemover which called a different inventory function on the player: one that doesn't exist in TDM. It must have been scrapped when TDM's new inventory methods were designed. The original didn't apparently need to make choices about where to put the item or how many to remove.

 

As there is a script event

 

scriptEvent float replaceInvItem(entity oldItem, entity newItem);

 

this could probably be implemented in a probably 10 line script object, and a matching 10 line entityDef. Including the "droptofloor".

You're right, it could be done purely through a script object OR in the game code. I've been mulling over that one but without coming up with any strong reasons to prefer one over the other. I'm edging in favour of using a c++ class just because I presume people will want this to work when activated/triggered, and the other targets are implemented as idClasses.

 

I'd probably reach for that script event either way, because I put in dozens of hours of testing with it it earlier in the summer and even wrote test harnesses for it. I now know exactly how it reacts under all circumstances!

[Tels]

Hm, I spoke to soon. It can't be done in scriptin, or at least I can't see how. Here is a stub code for the scripting object:

 

 

 

// vim:ts=4:sw=4:cindent
/***********************************************************************
/*
/* Tels: Script object attached to an func_itemremove entity.
/*
/* When triggered, remove the named item from the player inventory.
***********************************************************************/

object tdm_func_itemremove
{
// has no init routine
//	  init();
	removeInvItem();
}

tdm_func_itemremove::removeInvItem()
{
	entity old = sys.getEntity( getKey("remove") );
	if (entity != $null_entity)
	{
			sys.println("Removing " + old.getName() + " from player inventory.");
			$player1.replaceInvItem( old, $null_entity );
	}

}

 

 

 

However, TDM completely misses a way to actually call the script function. There are hooks that call "frob_action_script" or "equip_action_script" on entities on certain events, but there are none for "Activate()". And ::Activate() is called when a trigger triggers an entity it targets.

 

Since func_itemremove would be targetted by a trigger (or that is how id did it usually), the trigger will "activate" the entity, which will ignore it's own scriptobject and do nothing. (unless it is a special idSomething class).

 

AFAICS there are three solutions to this:

 

* create the idFuncRemover class and implement it there in Activate() (I usually do not like to add a new class in C++, we IIRC even combined a few back then. The reason is that special classes that basically have only one function create code cruft, and if you need to change something, you have one more place to change stuff. If the new class is something completely new and complicated, ok, but for a single function call?)

 

Edit: Basically the argument boils down to that a new class for each function is a waste, only available in full releases, and the next function will be needed next week. The calleventfunction entity was actually the solution to this, but somehow back then I overlooked that this requires an extra entity for each function call because it can't call itself. But for the occasional "do this on triggering" it works.

 

* create an "activate_script_event" hook and call it for all entities on Activate() (this is actually a useful feature which should be done regardless as this feature is sorely missing)

 

* require that the trigger is linked to an atdm:callscriptevent entity and have that entity call the event on func_itemremove. This runs into the same limitation I run into with Swift Mazes - for a single function call you need 3 entities (trigger => call event entity => func_itemremove). This seems overkill, too.

 

(Afterthought: It is not possible to attach the above script object to an callevententity, and let it call the method on its own scriptobject, because can do so only on targets, and entities cannot target themselves. That was another feature request I had

 

Or does anybody else know of a way to hook Activate() into scriptobject methods?

Edited September 3, 2014 by Tels

[SteveL]

Reordered your post a bit to respond to two points:

 

However, TDM completely misses a way to actually call the script function. There are hooks that call "frob_action_script" or "equip_action_script" on entities on certain events, but there are none for "Activate()". And ::Activate() is called when a trigger triggers an entity it targets.

 

Since func_itemremove would be targeted by a trigger (or that is how id did it usually), the trigger will "activate" the entity, which will ignore it's own scriptobject and do nothing. (unless it is a special idSomething class).

 

(Afterthought: It is not possible to attach the above script object to an callevententity, and let it call the method on its own scriptobject, because can do so only on targets, and entities cannot target themselves. That was another feature request I had

 

Or does anybody else know of a way to hook Activate() into scriptobject methods?

 

Obsttorte has a solution for this one: I saw it in his fix to enable compound entity lights to be triggered on and off in 2.03. He added a STIM_TRIGGER and matching response in the object's Init() method.

 

But I agree that letting script objects pick up the trigger call would be a Good Thing.

 

* create the idFuncRemover class and implement it there in Activate() (I usually do not like to add a new class in C++, we IIRC even combined a few back then. The reason is that special classes that basically have only one function create code cruft, and if you need to change something, you have one more place to change stuff. If the new class is something completely new and complicated, ok, but for a single function call?)

 

I don't see the problem with a new class. The code for a new class with 1 function will be about the same size as the code for a new script object. And it has the big advantage that the new code will be where people expect to find it -- with all the other targets. There'll be no code duplication either, so no future requirement to change something in more than one place -- or have I missed something?

[Tels]

Obsttorte has a solution for this one: I saw it in his fix to enable compound entity lights to be triggered on and off in 2.03. He added a STIM_TRIGGER and matching response in the object's Init() method.

 

I'd like to see his solution, maybe it is useful for my own adventures into scripting land.

 

But I agree that letting script objects pick up the trigger call would be a Good Thing.

 

Should I have a go at it? I'd like to add spawnarg support to idTrigger_touch, anyway.

 

I don't see the problem with a new class. The code for a new class with 1 function will be about the same size as the code for a new script object. And it has the big advantage that the new code will be where people expect to find it -- with all the other targets. There'll be no code duplication either, so no future requirement to change something in more than one place -- or have I missed something?

 

My answer is probably a bit off-topic here, if you want you can move this to an extra thread:

 

Anyway, spoken like a true developer

 

Of course, code is not the probem if you are a developer. Also, you can re-compile TDM. And you have access to it before the next version gets released. But the case against it is that one would basically write code like:

 

* addFunction() { return 1 + 2; }

 

and if somebody needs x + 2 you write:

 

* addFunction() { return x + 2; }

 

and then somebody needs x -2 and you write addFunction() { return x + y; } and then somebody needs x / 2, and then x * z and so on.

 

So for every imaginable use-case, you add one C++ class with nothing but one function call in it. This time it is itemremove. Next time itemadd. Then itemnamechange. And so on.

 

The atdm:target_callscriptevent is the generic solution to this problem. It allows you to redefine the function that gets called without having to recompile TDM. (Even at runtime!)

 

It's basically a design choice - cement one special case in the code, or just add a generic hook so other use cases can be done, too.

 

Currently the generic solution does have a bit higher overhead (one extra entity), but it would solve the issue without cementing it in the code where it can only be debugged and/or changed by developers instead of by mappers.

 

In general terms, I'd like to see the TDM engine getting more generic - you never know what crazy things people will do with it. The guy who recreated UUW maps with it - pure genius. And these things get difficult if the scope of the engine is too narrow. A good example is for instance the idTech4 engine - I heard it was so flexible, people could mod a popular shooter into a popular stealth game and it worked!

[SteveL]

I'd like to see his solution, maybe it is useful for my own adventures into scripting land.

This is from the Init() method of the script object:

// Obsttorte #3760
ResponseAdd(STIM_TRIGGER);
ResponseSetAction(STIM_TRIGGER,"LightsToggleResponse");
ResponseEnable(STIM_TRIGGER,1);

Which makes the object call LightsToggleResponse() on its own script object when activated.

 

As for the rest of it, I agree this isn't the thread for it. That's not the choice facing us here, which is how to fix a broken feature that's already partly implemented, and to do it in an unsurprising way that won't have future devs asking "where and why did they hide this bit of the code?". No-one is arguing against re-usable and extendable interfaces. Agreed, they're good things too, and something I always aim for too of course when designing new features.

[SteveL]

Should it stay as func_itemremove, or should we replace it with atdm:target_itemremove? No existing maps use it and it didn't work anyway so changing it wouldn't hurt. The atdm:target group looks to be the TDM way of letting mappers trigger changes in game state. The func_ entities are older and more of a mixed bunch. They mostly get triggered too but they mostly do physical stuff, like rotate or generate a force field within their volume. Some do other jobs like activate a camera view, but they have little in common. (@Tels: I guess that's the kind of piecemeal approach that you want to avoid). I reckon the item remover would fit better in the atdm:target set unless there's a reason to stick with the existing name.

[Tels]

Should it stay as func_itemremove, or should we replace it with atdm:target_itemremove? No existing maps use it and it didn't work anyway so changing it wouldn't hurt. The atdm:target group looks to be the TDM way of letting mappers trigger changes in game state. The func_ entities are older and more of a mixed bunch. They mostly get triggered too but they mostly do physical stuff, like rotate or generate a force field within their volume. Some do other jobs like activate a camera view, but they have little in common. (@Tels: I guess that's the kind of piecemeal approach that you want to avoid). I reckon the item remover would fit better in the atdm:target set unless there's a reason to stick with the existing name.

 

Personally, I'd go with the atdm:target_* name, as the new entity will have a completely different implementation, anyway. And the name makes it clear it works as a target of a trigger.

[SteveL]

Thanks for the vote. We just have to work out now what types it should support. Unique entities and stackables have already been mentioned. Other options are arrows and loot. Those 4 types have only 1 thing in common: they can show up in the inventory. Under the hood they're quite different things, and they each have a different script event that would be needed to manipulate the inventory (although they all use the same script event when being added to the inventory. -- I know... there's a good example for you )

 

I guess (hope?) it's not necessary to support loot. That would get a bit complicated -- it's perfectly possible but would need a fistful of spawnargs.

[Tels]

The biggest hurdle I see is that the removeItem entity should be quite generic, but the spawnarg on it only allows you to say "remove item with name X", and that gets tricky if X is supposed to be some generic item like a broadhead.

 

Because the code needs to spawn an actual broadhead entity, just to read out what the inv. target name would be. (I forgot if it is possible to access the spawnargs of not-yet-spawned-entity of class Y).

 

So it would be better if the mapper specifies what inv-name of item he wants to get removed. But that can be cumbersome, for instance:

 

"inv_name"			  "#str_02435"			 // Noisemaker Arrow

 

The mapper would need to set:

 

"remove"			  "#str_02435"			 // Noisemaker Arrow

 

So alias names "noisemaker" etc could be defined and handled. (If in scripting, I can easily type this out).

 

As for loot, I'm not sure we really need this. But if it is added, you also need the amount (which we probably need for arrows, anyway). Possible so:

 

"remove"		"gold"
"amount"		"200"
"remove_1"	"broadhead"
"amount_1"	"2"

 

That above would remove 200 gold (or less, if the player has less) and 2 (or less) broadheads.

 

This:

 

"remove"		"Pharao Painting"

 

would remove the item which inv_name matches "Pharao Painting" (it is not I18N ready, tho, so mappers should not use it that way).

 

There is just the slight problem that the current script event needs an entity, not an inv_name So, it might be nec. to implement this in C++ afterall.

 

Interestingly enough:

 

bool CInventory::ReplaceItem(idEntity* oldItemEnt, idEntity* newItemEnt)
{
	if (oldItemEnt == NULL) return false;

	idStr oldInvName = oldItemEnt->spawnArgs.GetString("inv_name");

 

the entity passed in is just used to read the spawnarg, anyway. So a new "RemoveItem( const char inv_name )" might be nec. Bascially something like:

 

void CInventory::RemoveItem( const idStr oldInvName)
{
	CInventoryItemPtr oldItem = GetItem(oldInvName);

	if (oldItem == NULL)
	{
			gameLocal.Warning("Could not find old inventory item for %s", oldInvName.c_str());
			DM_LOG(LC_INVENTORY, LT_DEBUG)LOGSTRING("Could not find inventory item for %s\n", oldInvName.c_str());
			return;
	}

	RemoveItem(oldItem);
}

 

Does not handle loot, but there is already a routine for that.

 

How such small things can get so complicated

Edited September 4, 2014 by Tels

[SteveL]

I (eventually!) implemented this as atdm:target_itemremove, found under "targets" in DR. The old func_itemremove was completely broken and hadn't been used in any published map, so it'll be gone with 2.04.

 

The new target_itemremove supports removing unique items, stackables, and ammo. Yes (@Tels) the naming could have been tricky, so I settled on:

• Unique items: the "name" of the item as set in DR
• Stackables: the "inv_name". The help text in DR spells it out: copy the "inv_name" from the item you want to remove, whether that's "#str_02435" or something custom.
• Ammo: you have to name the ammo, as it's known in the def files. That would be trickier for the mapper, so the help text lists all the standard options: broadhead, firearrow, gasarrow, mossarrow, noisemaker, ropearrow, vinearrow, waterarrow

You can also set the amount or use 0 to mean "remove all of them".

Lastly, there's a checkbox option "drop_in_world", so the mapper can sopecify whether the item is to be removed from the world, or dropped at the player's feet. If the player drops 5 arrows or stackables, a single item spawns which adds the right amount to the inventory when picked up.

 

All the options are set in the entity def to show up in DR when the entity is first created, without needing to click on 'inherited spawnargs', and they all have help text.