[PATCH] Player-owned animals are deletable (kill button). This is incorrect behavior.

[michael]

Player-owned animals (sheep, goats, anything) are deletable (kill button). This is incorrect behavior. The player's units should have to kill them first.

[historic_bruno]

Probably needs the usual multi-angled approach:

• filter the selection in input.js:performCommand() so we don't even try to delete animals (covers the hotkey case)
• for a selection of only animals, the UI shouldn't show a "Delete/Kill" action
• and the simulation can do its own check when processing "delete-entities", so nobody can cheat ;)

[leper]

Please add '[PATCH] ' to the summary and review to the keywords when submitting a patch as outlined in SubmittingPatches.

Now some comments on the code:

• You're already changing the indentation of the code in utility_functions.js so you should indent commands.push too.
  
• In GuiInterface.js you used "Animal" twice. The second one should probably be "Domestic".
  
• Commands.js: You should check if cmpIdentity is non-null too, and only check for the classes if it is. Otherwise just delete the unit.

Apart from that the code is looking good.

[historic_bruno]

I don't like the "deletable" property in the patch because it obscures the real test, which is more like "isdomesticanimal". Maybe it would be cleaner to add the ResourceSupply.KillBeforeGather state? That should cover any case of player-owned resources that must be killed before gathering, though animals are the only ones that come to mind.

Are there any non-resource animals/entities the player will be able to own but not allowed to delete?

[Erik Johansson]

Replying to historic_bruno:

Are there any non-resource animals/entities the player will be able to own but not allowed to delete?

Not that I can think of, but it's probably a good idea to have as flexible a system as possible. In case someone in a mod wants to do some kind of special unit building that cannot be deleted. I don't know what, but as long as it doesn't add any(/too much) unnecessary complexity it's better to have that flexibility. (I guess it could be used in a specific game mode where the stat points are important and you don't want users to delete buildings the enemy is about to destroy/likely to destroy soon just to avoid the enemy gaining points for destroying it. I don't know how feasible that is, but it's an example of a scenario where it could be useful I guess =) )

[yakca]

Replying to historic_bruno:

I don't like the "deletable" property in the patch because it obscures the real test, which is more like "isdomesticanimal". Maybe it would be cleaner to add the ResourceSupply.KillBeforeGather state? That should cover any case of player-owned resources that must be killed before gathering, though animals are the only ones that come to mind.

Are there any non-resource animals/entities the player will be able to own but not allowed to delete?

Yeah, I agree with your sentiment on the deletable property. I'll see what can be done to make the property's name a bit more justifiable.

[Deiz]

Your Commands.js hunk is incorrect. You should be checking cmpHealth first, and then checking for cmpResourceSupply inside, and killing normally otherwise, e.g.:

if (cmpHealth)
{
   var cmpResourceSupply = Engine.QueryInterface(ent, IID_ResourceSupply);
   if (!cmpResourceSupply || !cmpResourceSupply.GetKillBeforeGather())
      cmpHealth.Kill();
}

Otherwise it looks fine to me.

[historic_bruno]

I suggest moving killBeforeGather into ret.resourceSupply, since it's the same component and should be logically grouped.

[Deiz]

In 12445:

Prevent domestic animals from being manually deleted (killed). Patch by yakca. Fixes #1525.