Opened 10 years ago

Closed 8 years ago

Last modified 8 years ago

#2732 closed enhancement (fixed)

[PATCH] Loot resources that killed enemies carried

Reported by: Radagast Owned by: Stan
Priority: Should Have Milestone: Alpha 20
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by Radagast)

Both the loot defined in the simulation template and traded goods if the to be looted entity is a Trader, are already transfered.

This patch adds the following other inventory related transfers:

  • All carried resources are transfered to the looting entity. e.g. if the dead entity did carry some food, the food will be yours.

To achieve this, 2 distinct algorithms are given: 1) Directly add the whole carried resources' amount to the looting unit's player's resources. 2) Transfer all the carried resources' amount that the looting unit can carry.

  • Garrisoned entities are first being ejected, then the Ownership is transfered from the previous owner (enemy or ally thereof) to the looting unit's player, finally the unit is garrisoned inside the looting unit.

Edit as I'm always recognized as spam despite captcha. @Itms: Thank you Itms for the review, I agree.

I added the review keyword too early. I currently try to learn how to best handle those versions and keeping track of the changes. To get some feedback I submitted this patch. And I got what I wanted. Thanks a lot.

Once I have less trigger work and can resolve a strange git branching issue that somehow reloads my Looter.js twice (or perhaps I have to clear the cache), I will submit new versions (I have maintain a branch for each of my planned patches).

Next I will split the two approaches and make it committable (removing all but the most needed comments).

Attachments (2)

extend_looter_to_transfer_carried_resources_and_in_the_looted_entity_garrisoned_entities.diff (7.1 KB ) - added by Radagast 10 years ago.
extend_looter_to_transfer_carried_resources_and_in_the_looted_entity_garrisoned_entities.2.diff (6.8 KB ) - added by Radagast 10 years ago.
Fix syntax error due to double termination bracket. Reuse GarrisonHolder's function AllowedToGarrison to simplify. Fix wrong indentation (not using tabs).

Download all attachments as: .zip

Change History (12)

comment:1 by Stan, 10 years ago

love it :)

by Radagast, 10 years ago

Fix syntax error due to double termination bracket. Reuse GarrisonHolder's function AllowedToGarrison to simplify. Fix wrong indentation (not using tabs).

comment:2 by Itms, 10 years ago

Keywords: review removed
Summary: [Patch] Extend the Looter component to transfer any carried resources and garrisoned entities (aka Inventory transfer).[PATCH] Looting carried ressources and handling of garrisoned entities

I didn't test the patch but regarding the style, a patch should be eventually committable. The code you write is hopefully made to be added to the game, and is not aimed at discussing design.

So you just can't leave all these commented out lines and explanations in the patch. On top of what I pointed out, they make the patch a pain to review. :(

Given that you have two distinct approaches, you should for instance make two patches, each one being "committable", not referencing the other one, etc.

Looking forward to the next version(s), thanks for proposing this code! :)

comment:3 by Radagast, 10 years ago

Description: modified (diff)

comment:4 by Stan, 9 years ago

Keywords: looter removed
Milestone: Alpha 18Alpha 19
Owner: changed from Radagast to Stan

I want to see if I can split it up but won ´t be able to do it before A19.

therefore pushing it,

comment:5 by mimo, 9 years ago

Milestone: Alpha 19Backlog

moving to backlog as there is no activity since months.

comment:6 by Stan, 9 years ago

I think 1) is better, It lacks realism, but players wouldn't understand if their units suddenly start carrying stuff. Moreover it adds some micro-management, and I don't think anybody wants it.

I will split the first algorithm, and try to make something reviewable.

comment:7 by elexis, 9 years ago

Is there any reason to handle garrissoned entities at all? Why handle them at all? If you destroy a tower with resource-carrying units inside of it, then the resources will be transfered when killing those units afterwards, right? In that case the second half of the patch should be removed

The patch should have all changes needed, but as few changes as possible.

Would be good if we could commit #3424 beforehand, so that the new patch can be implemented in a cleaner way too (adding the resources only once). Those resources would also be added to the statistics tracker then too, which you currently don't do (which is probably very wrong).

comment:8 by elexis, 9 years ago

Summary: [PATCH] Looting carried ressources and handling of garrisoned entities[PATCH] Loot resources that killed enemies carried

Since we instantaneously transfer the resources of killed trade carts to the player, it would be consistent if we do the same for those resources that enemies carried.

If we commit the cleanup in #3424, then this patch will become trivial and it only has to add targetEntityState.resourceCarrying to resources.

comment:9 by elexis, 8 years ago

Resolution: fixed
Status: newclosed

In 17334:

Loot resources that killed enemies carried. Fixes #2732

comment:10 by elexis, 8 years ago

Milestone: BacklogAlpha 20
Note: See TracTickets for help on using tickets.