Opened 13 years ago

Closed 13 years ago

Last modified 8 years ago

#697 closed enhancement (fixed)

[PATCH] Units promotion

Reported by: fcxSanya Owned by: fcxSanya
Priority: Should Have Milestone: Alpha 5
Component: UI & Simulation Keywords:
Cc: fcxSanya Patch:

Description

Units should be able to promote to next rank (basic -> advanced -> elite).

Attachments (2)

units_promotion.diff (44.3 KB ) - added by fcxSanya 13 years ago.
units_promotion_2011_04_24.diff (61.1 KB ) - added by fcxSanya 13 years ago.

Download all attachments as: .zip

Change History (20)

by fcxSanya, 13 years ago

Attachment: units_promotion.diff added

comment:1 by fcxSanya, 13 years ago

Some notes about patch attached:

  • units can only promote if next template explicitly specified in the Promotion element of xml template (in the future code should also automatically check template suffix _b _a _e and use next template if it is available);
  • when unit is promoted it displays "promoted" animation and invulnerable in this moment;
  • promoted animation exists only for infantry so cavalry just stay still when it is promoted.

comment:2 by fcxSanya, 13 years ago

Keywords: review added

comment:3 by Michael D. Hafer, 13 years ago

"in the future code should also automatically check template suffix _b _a _e and use next template if it is available"

It is possible we might want a unit to promote to another entity that does not use the _b _a _e nomenclature. In fact, I do this with the Roman legion (which goes _marian _imperial _centurio) and Macedonian hypaspist (which goes hypaspist to argyraspis, i.e. "Silver Shield").

in reply to:  3 comment:4 by fcxSanya, 13 years ago

Replying to Mythos_Ruler:

It is possible we might want a unit to promote to another entity that does not use the _b _a _e nomenclature. In fact, I do this with the Roman legion (which goes _marian _imperial _centurio) and Macedonian hypaspist (which goes hypaspist to argyraspis, i.e. "Silver Shield").

This is only way which is currently implemented in the patch - you can specify any template name in Promotion.Entity element, so it already should work for legionaries and hypaspists any other units. In the future there will be just additional logic - if template is not specified explicitly, then it will try to find next template automatically, so there will be not need to specify next entity for _b and _a templates.

(one note: Promotion.Entity should have path starting from templates dir, for example: units/rome_legionnaire_imperial)

comment:5 by Kieran P, 13 years ago

Milestone: BacklogAlpha 4

comment:6 by Kieran P, 13 years ago

Type: taskenhancement

comment:7 by Kieran P, 13 years ago

Summary: Units promotion[PATCH] Units promotion

comment:8 by Philip Taylor, 13 years ago

(Sorry I've been so slow at responding :-( )

This looks like the best approach to handling promotion, so I'm happy with the general design. Just got several minor points from reading through the patch.


<animation file="biped/not used/inf_salute_c.psa" name="Promotion" speed="288"/>

Animations should probably be moved out of "not used" before they're used ;-)

EntitySelection.prototype.checkRenamedEntities: Instead of calling toList and selectedEntities.indexOf(id) != -1, just check this.selected[id] (since that's simpler and faster).

What happens if an entity gets promoted twice in a single turn? (Maybe it's just defeated Zeus and won a million XP). It looks like the new entity from the second promotion won't get added to the GUI selection.

selection_details.js: experience should be declared with var before using it. (I thought SpiderMonkey checked for this automatically, but maybe only in simulation scripts, not GUI scripts...)

MT_EntityRenamed should probably be consistent with MT_ConstructionFinished, i.e. use entity/newentity instead of oldEntityId/newEntityId.

Identity.js: Entities mustn't change this.template.Rank, since templates are shared between multiple entities. SentRank doesn't seem to be called at all so it could just be removed.

Promotion.js: Why add Curr in the schema? It doesn't seem to be set anywhere.

The GetCurr/GetReq function names are very short and uninformative - it'd probably be good to make them more readable, like GetCurrentXp/GetRequiredXp.

I'm not sure about the idea of automatically doing b->a->e promotions - I generally prefer everything to be explicit, and for filenames to not affect gameplay at all. With implicit promotions, it becomes harder to analyse the entities in external tools or to read the raw XML manually, because the behaviour is hidden in the code instead. It shouldn't be a problem to add the correct <Promotion> tags to every entity and I think it would be less confusing in the long term.

Is CanPromote necessary? It looks like elite unit templates have <Promotion disable=""/> so they won't have this component at all and so CanPromote will always be true. (The <Promotion disable=""/> is explicit in the XML, so I prefer it to having implicit special behaviour based on the string "Elite", as before :-) )

Promotion.prototype.Promote: Should check cmpCurrentUnitPosition.IsInWorld() before calling GetPosition() on it (because units might have no position). Should use GetPosition2D instead of GetPosition, since the height doesn't matter. (That'll return a vector with x/y properties instead of x/z). Should copy the GetHeightOffset value too (we don't use it yet but we might).

What happens when garrisoned units get promoted? (Maybe we can just assert that that's impossible, because units can never gain XP while garrisoned?)

What if a unit is in a formation when it's promoted? (Probably the Formation component should listen for MT_EntityRenamed and update this.members.)

What if another unit is attacking a unit that gets promoted? (Probably UnitAI should listen for MT_EntityRenamed, and if it's in a COMBAT state it should start attacking the new entity instead.)

UnitAI.js: Put the SetInvulnerability inside the leave function instead of Timer. That'll guarantee the unit definitely becomes non-invulnerable when it stops cheering, even if it's unexpectedly pulled out of the CHEERING state by some other order or event. (We don't want players to exploit bugs and get an invulnerable army.)

template_unit.xml: The convention is to do <Looter/> instead of <Looter /> (and similar for Promotion in other XMLs).

comment:9 by Kieran P, 13 years ago

Milestone: Alpha 4Alpha 5

by fcxSanya, 13 years ago

in reply to:  8 comment:10 by fcxSanya, 13 years ago

Replying to Philip:

What happens when garrisoned units get promoted? (Maybe we can just assert that that's impossible, because units can never gain XP while garrisoned?)

Yes, with current design garrisoned unit don't gain XP and can't be promoted, so with this part probably should not be problems.

Everything else should be fixed in new version of patch.

comment:11 by fcxSanya, 13 years ago

Forgot to wrote: I moved "binaries/data/mods/public/art/animation/biped/not used/inf_salute_?.psa" files to "binaries/data/mods/public/art/animation/biped" folder, but this can't be reflected in diff, so to test this patch you need perform something like:

svn mv inf_salute_?.psa ..

from "binaries/data/mods/public/art/animation/biped/not used" folder

comment:12 by Kieran P, 13 years ago

Cc: fcxSanya added
Owner: set to fcxSanya

comment:13 by Kieran P, 13 years ago

Milestone: Alpha 5Alpha 6

comment:14 by Philip Taylor, 13 years ago

Hmm, if a unit fires an arrow then gets garrisoned then the arrow kills a target a second later, won't the attacker gain XP while it's garrisoned? (If so, maybe it just needs GarrisonHolder to catch OnGlobalEntityRenamed, unless I'm missing something.)

Formation.js: The line

if (this.members[msg.entity])

looks wrong - it should probably be if (this.members.indexOf(msg.entity) != -1)

Formation.js: Instead of using splice/push, probably better to do this.members[this.members.indexOf(msg.entity)] = msg.newentity, so the ordering will stay the same and units won't be tempted to move to different slots in the formation.

Templates: Why remove <Rank>Elite</Rank> from lots of them? Won't that mean they're wrongly identifed as Advanced instead?

GuiInterface.js: GetRenamedEntities doesn't need to do .slice() to clone the array, since values get cloned when being return to the GUI scripts anyway.

Promotion.js: Looks like GetCurrentXp can just do return this.currentXp; since it's always defined and an integer and doesn't need any fancy conversions.

Quite a few lines seem to have trailing whitespace, which is a bit ugly (or maybe I'm just being too fussy :-) ).

Conditionals like

if (this.invulnerable) return { "killed": false };

(plus several more similar) should preferably be written like

if (this.invulnerable)
    return { "killed": false };

to match the usual style.

template_unit.xml: The </Vision> line is indented wrongly.

If this is ready in the next day or so, I think I'd be happy if you commit it before alpha 5, since it should be safe enough; or wait until after the release if that'd be too much of a rush.

in reply to:  14 comment:15 by fcxSanya, 13 years ago

Replying to Philip:

Templates: Why remove <Rank>Elite</Rank> from lots of them? Won't that mean they're wrongly identifed as Advanced instead?

Sorry, I decided that this is not used in other places and deleted when add <Promotion disable=""/> element, but this is probably used at least by gui scripts, I will revert it back.

If this is ready in the next day or so, I think I'd be happy if you commit it before alpha 5, since it should be safe enough; or wait until after the release if that'd be too much of a rush.

I will try to finish and commit it today.

comment:16 by fcxSanya, 13 years ago

Resolution: fixed
Status: newclosed

(In [9391]) Units promotion. Closes #697.

comment:17 by Erik Johansson, 13 years ago

Milestone: Alpha 6Alpha 5

comment:18 by sanderd17, 8 years ago

Keywords: review removed
Note: See TracTickets for help on using tickets.