#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)
Change History (20)
by , 13 years ago
Attachment: | units_promotion.diff added |
---|
comment:1 by , 13 years ago
comment:2 by , 13 years ago
Keywords: | review added |
---|
follow-up: 4 comment:3 by , 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").
comment:4 by , 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 , 13 years ago
Milestone: | Backlog → Alpha 4 |
---|
comment:6 by , 13 years ago
Type: | task → enhancement |
---|
comment:7 by , 13 years ago
Summary: | Units promotion → [PATCH] Units promotion |
---|
follow-up: 10 comment:8 by , 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 , 13 years ago
Milestone: | Alpha 4 → Alpha 5 |
---|
by , 13 years ago
Attachment: | units_promotion_2011_04_24.diff added |
---|
comment:10 by , 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 , 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 , 13 years ago
Cc: | added |
---|---|
Owner: | set to |
comment:13 by , 13 years ago
Milestone: | Alpha 5 → Alpha 6 |
---|
follow-up: 15 comment:14 by , 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.
comment:15 by , 13 years ago
Replying to Philip:
Templates: Why remove
<Rank>Elite</Rank>
from lots of them? Won't that mean they're wrongly identifed asAdvanced
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 , 13 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:17 by , 13 years ago
Milestone: | Alpha 6 → Alpha 5 |
---|
comment:18 by , 8 years ago
Keywords: | review removed |
---|
Some notes about patch attached: