Opened 8 years ago

Last modified 4 months ago

#4000 new defect

[WIP] Don't use BuildingAI for ships or siege tower

Reported by: elexis Owned by:
Priority: Must Have Milestone: Backlog
Component: Simulation Keywords: beta
Cc: b.jansen5@… Patch:

Description (last modified by DanW58)

Since ships were introduced they used BuildingAI to shoot arrows when being garrisoned and use UnitAI to have a stance, move around and get into attack range.

This resulted in inconsistencies like 1 arrow being shot from UnitAI and N arrows being shot from BuildingAI.

The code to determine the number of garrisoned arrows should be refactored so that both buildings and ships can call the same methods to shoot arrows without ships using BuildingAI logic to f.e. check for enemies in range.

Change History (22)

comment:1 by fatherbushido, 8 years ago

Summary: Don't use BuildingAI for shipsDon't use BuildingAI for ships or siege tower

ref #4189

Last edited 8 years ago by fatherbushido (previous) (diff)

comment:2 by bb, 8 years ago

Cc: b.jansen5@… added

comment:3 by elexis, 8 years ago

In 18454:

Tooltip overhaul.

Also contains a patch by fatherbushido for attackRateDetails and getAttackTooltip to
not show something broken for buildingAI units that can capture, fixes #4061 (see #4000).

Make tooltip functions uniform.

Pass template everywhere

instead of template.armour in getArmorTooltip
rate in getRepairRateTooltip and getBuildRateTooltip and
entState in attackRateDetails.

Add an early return for every tooltip function.

Use empty string instead of "Armor: (None)" for trees etc..

Don't prefix tooltip return values with "\n", but let the user of that function add them.
Thus make tooltip concatenation much nicer (f.e. draw.js).

Use a loop instead of duplicating per damage type in damageTypesToText.
Add font functions to avoid duplicating tag code.
Merge sprintf's and inline variables.
Add few TODOs.

Fix some strings:

Use "%(specificName)s %(fontStart)s(%(genericName)s)%(fontEnd)s") instead of "(" + foo + ")" ...
Use existing "%(percentage)s%%" instead of foo + "%" in armorLevelToPercentageString.

Remove

duplication by calling/introducing shared functions (getEntityTooltip, getHealthTooltip, getGatherTooltip, getVisibleEntityClassesFormatted),
unused function damageTypeDetails which was also a duplicate of damageTypesToText,
unused function damageValues,
some warns that are equivalent to errors they attempt to cover up (getAttackTypeLabel, getCostComponentDisplayIcon, getEntityNames, getEntityNamesFormatted),
some unused variables,
"???" and translate("???").

Don't fix translate("Foo:") strings to avoid a lot of translation work.

comment:4 by bb, 8 years ago

Keywords: rfc added
Milestone: BacklogAlpha 21
Summary: Don't use BuildingAI for ships or siege tower[WIP] Don't use BuildingAI for ships or siege tower

Some WIP here: https://github.com/bb-bb/0ad/tree/t4000_buildingAI. This is just a draft for this ticket so don't kill me for style issues.

The approach in this WIP is to merge the attack logic from buildingAI with the ranged logic from attack and call those functions from either buildingAI or UnitAI. The buildingAI schema has got empty by these changes (the values moved to attack), So to keep the component living I putted some dummy in the schema, maybe there are cleaner ways to do this.

A side effect is that ships and siege towers no longer attack multiple enemy's but only 1, don't know if that is wanted (and this is a WIP after all...). Also they cannot move and shoot at the same time anymore. Allowing this again can be done with defining a new unitAI state like: WALKINGANDATTACKING. (That one can then also be used for e.g. chariots.)

This patch solves #4189 and makes ship ramming possible. Somewhat related #2010

Last edited 8 years ago by bb (previous) (diff)

comment:5 by elexis, 8 years ago

Milestone: Alpha 21Alpha 22

Feature freeze in 2 days.

comment:6 by fatherbushido, 8 years ago

In 18766:

Reverts r16907 as capture doesn't work for units with BuildingAI. Refs #3356, #4189, #4000.

comment:7 by fatherbushido, 8 years ago

Last edited 8 years ago by fatherbushido (previous) (diff)

comment:8 by Imarok, 7 years ago

Keywords: beta added

comment:9 by fatherbushido, 7 years ago

Keywords: rfc removed

comment:10 by Stan, 7 years ago

Milestone: Alpha 22Backlog

Backlogging due to lack of progress

comment:11 by fatherbushido, 7 years ago

Milestone: BacklogAlpha 22

stanislas69: there has been some progress about that. There is now a plan about fixing that. I submitted it to bb and I hope we will have that for a22 (at least the first cleaning part). So I put it to alpha22 (not for the sake of bothering you ;-)).

comment:12 by bb, 7 years ago

I am really wondering what the "plan" is you talk about. We have had some discussion about implementing a new SiegeAI component, which I didn't see the benefit for as it would mostly be duplicating unitAI. Also with this we would create the same sort of situation between siegeAI and UnitAI as we have now between UnitAI and BuildingAI, and this ticket is about to solve the problem not just move the problem to some new component.

comment:13 by fatherbushido, 7 years ago

@bb: I was refering to a plan submitted in private section of the forum. The one we discussed in pm (not really what you described here, but I have certainly bad explained it ;-)). edit: btw you did good posting that comment because you suggested a first idea of name SiegeAI. That one is not really good as it applys to ship and stuff like that but it's a progress ;-)

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:14 by mimo, 7 years ago

Another possible way for this ticket would be that any garrisoned unit which currently gives an arrow would create a (invisible, indestructible and without obstruction) turret unit with a range attack. Ideally, this turret unit would be created using #252, but in the meantime for test purpose, this could work with a default turret template having only a range attack. That's just an idea i had some time ago but, as i did not really develop it more, there may still be some difficulties to deal with (the turrets should have the same los rangemanager as their parent; when the parent receives an attack order, this order should also be sent back to its turrets).

comment:15 by bb, 7 years ago

@fatherbushido: That's just a naming issue change SiegeAI to TurretAI or something similar.

@Mimo: in your approach we would end up with 1 unit shooting 1 arrow. How would we implement the arrow bonus tower tech? and arrow limit on that? ofc we can hack these things in but it's in concept not a very clean approach imo.

Maybe it would be better to use #2577 to solve this ticket, (I will post my draft for that ticket there). In this way we can have would have a unitAI unit as turretHolder and put a BuildingAI (or probably a turretAI?) unit as turret on that.

comment:16 by fatherbushido, 7 years ago

@bb: It was a positive comment.

There is also that mimo's idea to dig. But I guess anyway the step to split multiple arrows logic of buildingAI and of moving units is to be done anyway?

comment:17 by bb, 7 years ago

Indeed the multiple arrow logic from buildingAI needs to be splitted to somewhere else so moving units can use it too.

in reply to:  15 comment:18 by mimo, 7 years ago

Replying to bb:

@fatherbushido: That's just a naming issue change SiegeAI to TurretAI or something similar.

@Mimo: in your approach we would end up with 1 unit shooting 1 arrow. How would we implement the arrow bonus tower tech? and arrow limit on that? ofc we can hack these things in but it's in concept not a very clean approach imo.

That would not be a problem. I don't know which particular tech you meant, but the one like Sentries would still apply as now (the turret stuff would only apply for garrison arrows) while a tech like Crenellations would decrease the repeattime by 40% of such turret units.

The original interest of my proposition was that turrets are dealt with by UnitAI, so don't need a new component. But such a new component could indeed be cleaner, depending how it is implemented.

Maybe it would be better to use #2577 to solve this ticket, (I will post my draft for that ticket there). In this way we can have would have a unitAI unit as turretHolder and put a BuildingAI (or probably a turretAI?) unit as turret on that.

comment:19 by elexis, 7 years ago

Milestone: Alpha 22Backlog

comment:20 by Imarok, 5 years ago

Component: UI & SimulationSimulation

Move tickets to Simulation as UI & Simulation got some sub components.

comment:21 by DanW58, 3 years ago

Description: modified (diff)

This is probably a dumb comment, but I have to say it... Why should ships use unit AI, building AI, or turret AI? Can't they have ship AI? Ships are not a type of unit, nor a type of building, nor a type of turret. They are perhaps something inbetween, similar to a siege unit AI perhaps; but not the same thing, anyhow. And yes, ramming ships should be possible; and therefore a ship should be able to sink from a hull breach, and not only from having its "health" brought down to zero. All the more reason for them being their own thing.

Last edited 3 years ago by DanW58 (previous) (diff)

comment:22 by wowgetoffyourcellphone, 4 months ago

In 27996:

[Gameplay] Complete Naval Overhaul (UnitAI and Warship Classes)

Warships now use the Unit AI instead of the Building AI. This allows players to specifically target fire onto desired targets.
Warship combat now revamped with completely new Warship classes, which soft counter each other, and new behavior for Fire Ships. With an all-new ship tech tree at the Dock.
Commit includes fixing maps to switch from the old to the new ship templates. Also includes revamped Cycladic Archipelago skirmish maps and some other special surprises. And improved/additional ship UI portraits.

New Warship classes:

Scout Ship
Arrow Ship
Ramming Ship
Siege Ship

Forum discussion: https://wildfiregames.com/forum/topic/104332-naval-overhaul-alpha-27/#comment-539713

Patch by: @wowgetoffyourcellphone
Accepted by: @real_tabasco_sauce
Comments and suggestions by: @borg-, @phosit (more comments and discussion at the forum link)

Differential Revision: https://code.wildfiregames.com/D5213

Refs #4000 for a sound issue for actors without an attack animation.

Note: See TracTickets for help on using tickets.