Fixed the attack enemy sheep bug

[akveton]

The command_flag::attack_res flag was not being added, and therefore the AttackAbility::can_invoke method was returning false. Please review my changes.

Fixes #369.

[TheJJ]

1. please add yourself to copying.md
2. please adhere to the code and indentation style
3. You somehow changed the file mode from 644 to 755 :)
4. I think we should try a more generalized approach and not only add sheep as a "special exception". I'm not sure how the command_flag::attack_res flag is set normally (I'd have do dig a bit first), but I suspect that we should place the sheep fix somewhere else, e.g. when creating the data pack if necessary)

[Jon0]

The command_flag::attack_res was made to prevent attacking being prioritised over gathering, allowing the gather action to placed on stack first and set correct graphics. Recent changes (villagers now change unit types fixing graphic issues, and global priority of abilities in ability.h which shows gathering is highest) might mean this flag can be removed, and the logic in AttackAbility::can_invoke could be refactored.

Gathering must be a higher priority, otherwise the target is killed and the villager returns to an idle state. When gather is used, it automatically stacks the attack on top, so when the target is killed, the villager begins gathering.

[akveton]

I have fixed 1.-3. (hopefully 2. is correct), sorry for the mess.

As for 4., I had the same feeling as you do. When I was skimming through the code, I found that the only other place in the code this flag is being set is in GatherAction::update_in_range, which means this flag is normally used with respect to villagers - attacking trees, sheep and game. This was interfering with the logic in AttackAbility::can_invoke. I cannot think of a better fix at this time as I am new to the project however. I will continue searching for a better solution.

Edit:
@Jon0 : removing the flag and refactoring the logic in AttackAbility::can_invoke would seem like a very good idea to me. I will test villager behavior in the absence of this flag and hopefully propose some changes later.

[Jon0]

Looking at GatherAction::update_in_range since set_ability(ability_type::attack) restricts the actions to attacking anyway, the add_flag(command_flag::attack_res) should be safe to remove. In this case delete that attack_res flag completely as it has no other use. Refactoring AttackAbility::can_invoke would be the best fix.

Simply removing (cmd.has_flag(command_flag::attack_res) == target_is_resource) in AttackAbility::can_invoke could work.

[akveton]

Removing (cmd.has_flag(command_flag::attack_res) == target_is_resource) has the unfortunate consequence that military units are able to attack friendly sheep (which they were not able to in the original game, IIRC). I keep converging towards adding an exception for sheeps (because the sheep really is an exception - a resource which can be owned by a player) - maybe this could be synchronized with farms somehow?

[Jon0]

You're right, you should check the attacker has attribute attr_type::gatherer

[TheJJ]

Then, aren't other resources like turkeys an exception as well?

(btw, please use tab indentation, otherwise it looks good now code-style wise :)

[zuntrax]

I'm still a bit confused about the whole unit type thing, is it possible that turkeys also have unit type SHEEP? Also, there seems to be a resource trait DOMINANT_ANIMAL_DISCOVERY that is related to sheep and turkeys.

[akveton]

My IDE was messing the indentation up, should be ok from the next commit on.

Upon further examination, command_flag::attack_res might be needed in some form after all. Everything depends on the final implementation of Gaia (whether it will be hostile to the players or not). Because without this, military units could attack trees, etc if Gaia was to be hostile. On the other hand, military units could not attack wolves if Gaia was to be friendly.

Therefore, further classification of units might be needed instead (is a military attackable resource), a rundown:

• Tree - is enemy - is a resource - is not a military attackable resource (but what about the trebuchet?)
• Boar - is enemy - is a resource - is a military attackable resource
• Sheep - is enemy/friendly - is a resource - is a military attackable resource
• Wolf - is enemy - is not a resource

A proposal of the new part of the logic would then look something like this in pseudocode:

if (is enemy and is not resource) or (is resource and selected unit is gatherer) or (is enemy and is resource and is military_attackable_resource):
    can_attack = True

This is under assumption gaia stays hostile.

Unsure how to handle the trebuchet/onager - tree thing. Perhaps yet another classifier will be needed.

[zuntrax]

I'm pretty sure the onager-tree-interaction was by setting an area to attack. The trees get killed by splash damage. There might be another special case to consider, the most basic one of the onager-style siege weapons was not able to kill trees.
However, the whole gaia thing will probably a lot easier once #239 is finished. Our current gamedata handling is pretty makeshift.

[akveton]

Can't remember the original game, but the trees are directly targetable by onagers and trebuchets in the HD version, at least (without setting an area to attack) - a green rectangle, similar to buildings, appears, suggesting trees are treated as buildings in the HD version.

[Jon0]

The extracted game data contains an attack_mode value for each unit type. Currently openage ignores this, but it might indicate which types are a military attackable resource.

producer.cpp is where the gamedata gets converted into openage unit types. A new attribute for units could be added.

[TheJJ]

see above

[TheJJ]

Timeout detected, but ready for a revive.