#1525 closed defect (fixed)
[PATCH] Player-owned animals are deletable (kill button). This is incorrect behavior.
Reported by: | michael | Owned by: | yakca |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 11 |
Component: | UI & Simulation | Keywords: | simple gui |
Cc: | Patch: |
Description
Player-owned animals (sheep, goats, anything) are deletable (kill button). This is incorrect behavior. The player's units should have to kill them first.
Attachments (1)
Change History (13)
comment:2 by , 12 years ago
Keywords: | simple gui added |
---|
comment:3 by , 12 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:4 by , 12 years ago
Summary: | Player-owned animals are deletable (kill button). This is incorrect behavior. → [PATCH] Player-owned animals are deletable (kill button). This is incorrect behavior. |
---|
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 indentcommands.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.
comment:5 by , 12 years ago
Keywords: | review added |
---|---|
Milestone: | Backlog → Alpha 11 |
follow-ups: 7 8 comment:6 by , 12 years ago
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?
comment:7 by , 12 years ago
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 =) )
comment:8 by , 12 years ago
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.
comment:9 by , 12 years ago
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.
comment:10 by , 12 years ago
I suggest moving killBeforeGather
into ret.resourceSupply
, since it's the same component and should be logically grouped.
by , 12 years ago
Attachment: | deletable_animals.diff added |
---|
comment:12 by , 12 years ago
Keywords: | review removed |
---|
Probably needs the usual multi-angled approach:
input.js
:performCommand()
so we don't even try to delete animals (covers the hotkey case)