#2706 closed enhancement (fixed)
[PATCH] Allow individual entities to upgrade to other entities
Reported by: | wraitii | Owned by: | wraitii |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 21 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
(this is not a duplicate of #1919 though the behavior can be similar)
The attached patch implements an "Upgrade" component which allows to upgrade individual entities to other entities. It allows any entity to transform into any other. Usage examples would be: upgrading individual units to their advanced or elite form, upgrading individual towers. It is not meant to upgrade all of a buildings' type (ie a wall strength technology for example).
It checks for entity limit counts and updates them appropriately, and can check for buildRestrictions.
It can take a required technology or use the upgraded template's default. It can take a cost and a "time" to upgrade (those two currently can't be derived form the upgraded template but that could be added). It supports multiple possible upgrades (though they are mutually exclusive). It supports having a custom icon and a custom tooltip information. It also supports using {civ}.
It currently does not pass all data (such as trainingqueue, experience…) to the new entity. It reimburses the player fully if the upgrade is cancelled or the building destroyed, to mimic foundations.
Attached patch also changes the "Long wall->Gate" transformation to use this system and removes obsolete code.
This patch implements a generic helper to replace entities' template which #1919 could use.
Attachments (17)
Change History (65)
by , 10 years ago
Attachment: | UpgradeComponent.patch added |
---|
by , 10 years ago
Attachment: | UpgradeComponent.2.patch added |
---|
comment:1 by , 10 years ago
Description: | modified (diff) |
---|
I have moved the "CanTransform" check and the actual replacing to a Transform.js helper file, that Pack.js component also uses (and made the logic slightly better).
I think I've taken into account all of sanderd17 and leper's style remarks (except for the name of the component, I don't really see Transform as better than Upgrade as the most descriptive name would be ReplaceTemplate or something).
I've added the ability to transfer the garrisoning but not the production queue yet.
comment:2 by , 10 years ago
Above patch actually introduced a ton of bugs. I'll fix them and commit.
comment:4 by , 10 years ago
Milestone: | Alpha 17 → Alpha 18 |
---|
comment:5 by , 10 years ago
A question on a use case: would it be possible to allow a single hastati unit to upgrade to for example an officer (centurion) unit?
Also would it enable packing and unpacking of units?
Also, is it possible to have multiple possible upgrades?
comment:6 by , 9 years ago
niekt: To your questions. 1- yes, it's even designed for exactly that 2- technically yes but it's not really designed for it. 3- If you mean chained, yes, if you mean multiple choices, yes.
Itms= since I haven't committed it back then there must be some kind of issue, but I can't recall what and will check the logs.
comment:8 by , 9 years ago
Allow a mobile unit like a cart convert and unpacked into a CC ( building)? I gameplay that can be nice to Nomads civilization style. Like Huns
comment:9 by , 9 years ago
Milestone: | Alpha 18 → Alpha 19 |
---|
comment:11 by , 9 years ago
Milestone: | Alpha 19 → Alpha 20 |
---|
comment:12 by , 8 years ago
Milestone: | Alpha 20 → Alpha 21 |
---|
comment:13 by , 8 years ago
I merged the conflicts with the current SVN. But I noticed you didn't rip out everything about gates in the GUI. Is there a reason why you left the gate panel?
The current patch seems to work, apart from a missing icon for gates.
comment:14 by , 8 years ago
I tested the patch and it worked well.
It is needed to replace Engine.GuiInterfaceCall("GetNeededResources", totalCost);
by Engine.GuiInterfaceCall("GetNeededResources", { "cost": totalCost });
in selection_panels.js
and to add the icons for gates
<Icon>icons/gate_closed.png</Icon> in the <Upgrade> part of data/mods/public/simulation/templates/structures/rome_siege_wall_long.xml
comment:15 by , 8 years ago
Keywords: | design art ui removed |
---|
- Missing indentation in the L132 of the new
Pack.js
- I'd be happy about a newline for each property of the objects in L676 of
Commands.js
, L613 ofselection_panels.js
and L2 ofunit_commands.js
- L668
selection_panels
missing quotes - L678
selection_panels
newlines? - Inline variable notification in L684 and use newlines
- L694 operators should appear at the end of a line rather than in the beginning of the next
// abort if no template
=> code and comments shouldn't appear on the same line IMO (in this case the comment is not needed anyway)- Variable
totalCost
is a straight copy ofdata.item.cost
and doesn't seem needed (if so, we could also use clone or deepcopy)
by , 8 years ago
Attachment: | UpgradeComponent.diff added |
---|
follow-up: 18 comment:16 by , 8 years ago
I implemented most of the changes
- I don't really agree with splitting L2 of unit_commands.js though. It wouldn't make it more readable IMO, but just make it longer.
- Not sure what you want with L684 (or what file you mean)
- I think there's a bug with the totalCost thing you meantion, because other calls are done differently (and include a player parameter). So I didn't update that yet.
Meanwhile the icon issue is fixed, and unlike fatherbushido's patch, all new files should be included too. But I still have to read this code completely for the first time.
by , 8 years ago
Attachment: | UpgradeComponent.8.patch added |
---|
comment:17 by , 8 years ago
Keywords: | review removed |
---|
Style fixes:
- The adding of the two elements in L521 could be simplified by doing that in a loop
for (let locked of [true, false])
. let
for loop counters- Any reason not to nuke the vars in L1659, L541 and L190 (
sound
)? - One more space in L546
- If
gateIcon
isundefined
, you still add"stretched:session/"
? - The first
TODO
should be easily fixable if(
->if (
- L705 quote object keys and define properties on a single line each. unneeded parenthesis.
unit_commands.js
-> IMO newlines: better many lines with 0 inits than one line that is that wideChangeUpgradedEntityCount
-> Don't define variables inside the if statement (out-of-scope)- L97 space missing
cost["time"]
->cost.time
hasCosts ? cost : undefined
->hasCosts && cost
might work there"RequiredTechnology" in choice
? choice.RequiredTechnology`this.upgrading !== false
->!!this.upgrading
?CheckPlacementRestrictions != undefined
->CheckPlacementRestrictions
?"RequiredTechnology" in this.template[choice] && this.template[choice].RequiredTechnology === undefined
seems to check the same thing twice. Cant we just check!!this.template[choice].RequiredTechnology
?- L181 -> Use let in that scope (if not plumply everyhwere)
- Some more
x in y
which should all be replaced withy.x
as x is a property of y, thus y appear first to the reader GetResourceCosts
-> can those two calls in the loop be merged?- L676, L687 -> nuke notification, missing translate / markForTranslation
- L697 -> unneeded parenthesis
- L50 of
Transform.js
arrow function - L84 quote object keys at least
- L149 Uaaaaaaaaaaaaaargh, put those operators to the lineend, not beginning. Unneeded parenthesis as
&&
has higher operator precedence as||
. .length !== 0
->.length
- L180 -> better put the comment in the line above
length === 0
->!length
and put the comment above the return- All counted loops (
++j
) don't rely on the counter being a number, so it could just usefor j in foo
loops
comment:18 by , 8 years ago
Replying to sanderd17:
- I don't really agree with splitting L2 of unit_commands.js though. It wouldn't make it more readable IMO, but just make it longer.
I agree that the values 0
are boring, but the objects keys are interesting and might be wrong, so they shouldn't be out of view.
- Not sure what you want with L684 (or what file you mean)
var cmpGUIInterface = Engine.QueryInterface(SYSTEM_ENTITY, IID_GuiInterface); cmpGUIInterface.PushNotification({ "players": [data.cmpPlayer.GetPlayerID()], "message": someTranslateCall("Cannot upgrade as distance requirements are not verified or terrain is obstructed.") });
comment:19 by , 8 years ago
re sanderd: probably no reason to leave the gate panel other than I forgot I could remove it in the patch.
Might update it this WE (assuming comp still works) with elexis notes, but I'd really appreciate if you said which file you're talking about :P
Would like some comment on the spirit of the change itself, imo it's nicer this way overall.
by , 8 years ago
Attachment: | UpgradeComponent.10.patch added |
---|
comment:20 by , 8 years ago
Fixed a few mistakes that had crept through. Fixed style according to elexis' comment above (at least what I could find). Patch is tested against SVN and works correctly. Please add the image to check out how it looks.
comment:21 by , 8 years ago
Keywords: | review added |
---|
comment:22 by , 8 years ago
- Use JSdoc comments instead of
//
for functions tooltips.js
160 characters per line, should be below 80 ideally, so might want to sneak a newline in there. 140 characters on that comment inSelectionPanels.Upgrade
.- Use
let
instead ofvar
for newly introduced variables - Unneeded variables
selection
incancelUpgradeEntity
anditem
ingetItems
(L1016) which should just be inlined. Any reason at all why not to simply pushingupgrade
(or a clone of that) instead of copying it toitem
that way? isUpgrading
andprogress
are defined long before they are used, move it down Perhaps referencingdata.item.requiredTechnology
(instead of copying it torequiredTechnology
) would make it more readable with regards to the origin of the data (certainly would introduce more characters. Not completely convinced that copying a variable with a long name to a short one justifies the copy. IMO local variables should only exist if they are changed).- 1042-1044 one tab too much indentation, same for the call below
- L1060 unneeded parenthesis
- Duplicate statement
data.button.enabled = false;
can just be moved outside of the if-else part - L1106 Code could use
progressOverlay.hidden = isUpgrading
instead ofif ... = true else ... = false
- Still dont like the fact to put
g_unitPanelButtons
into a single line with240
characters. Could at least add 2 newlines to make that 80 character lines. Upgrade.prototype.ChangeUpgradedEntityCount
var defined in wrong scope. If you'd just uselet
you would have noticed before. Function could use newlines.- L97 -> missing space (
category,amount
) - L131 -> remove a newline so it's
ret.push({
hasCosts ? cost : undefined
-> if you just uselet hasCost
instead of setting itofalse
, it will beundefined
which eliminates the ternary- Not sure if we prefer
x.y !== undefined
overy in x
(L170), ask someone else (likely noone cares) GetRequiredTechnology
could use some newlinesGetRequiredTechnology
first uses "RequiredTechnology" in this.template[choice]then
this.template[choice].RequiredTechnology`. Should use the same check. Maybe you can even simplify by using an early return; likeif (this.template[choice].RequiredTechnology) return this.template[choice].RequiredTechnology
which might just beundefined
as returned in that other case of the function.Upgrade.prototype.Upgrade
-> merge the two if's- L232 140 characters meh
if (this.IsUpgrading() === false)
->!upgrading
?- L265 could use newline after ? and : (though mimo doesn't like those iirc)
- L707 newline
// Error (e.g. invalid template names)
unneeded comment as the error below (should) state the same// rescale
->// Rescale
(surrounding comments you added also start with caps)// Destroy current entity
comment stating the same as the code// This is kind of ugly, sorry about that.
-> it's not that ugly, remove the comment ;)
Big one:
Transform.js
those functions shouldn't be vars but just functions, f.e.var ChangeEntityTemplate = function(oldEnt, newTemplate)
should befunction ChangeEntityTemplate(oldEnt, newTemplate)
by , 8 years ago
Attachment: | UpgradeComponentRC-fixed5.patch added |
---|
comment:23 by , 8 years ago
Changed all of the above except for: -Use JSdoc comments instead of for functions -Use let instead of var for newly introduced variables -L232 140 characters meh -L265 could use newline after ? and : (though mimo doesn't like those iirc)
first one because the file didn't do it so far mostly and lazy, second one could probably automatically do it but I need to make sure I haven't used the function scoping feature elsewhere, last two just disagreed.
by , 8 years ago
Attachment: | UpgradeComponent-final.patch added |
---|
comment:25 by , 8 years ago
Keywords: | review removed |
---|
comment:26 by , 8 years ago
Here are some TODOs :
- progress of the upgrade process in the selection panels icon
- time of the upgrade process should be in seconds as for techs for example (reported by mimo)
- when the unit is deleted while upgrading, the cost is not given back (no cmpPlayer in CancelUpdate())
- tooltip with the costs of the upgrade in the selection panels is missing
- we need to change the gates stuff in the structure tree
- perhaps the upgrade button should be displayd near the techs instead of the units ? (see screenshot)
by , 8 years ago
Attachment: | screenshot.jpg added |
---|
comment:29 by , 8 years ago
Points 1,2,3,4 taken care in the commit. it was actually always supposed to be like that but was buggy. Also fixed the requiredtech stuff noticed by wow Disagree on point 6
Will let you guys see how you actually want to use the feature.
comment:30 by , 8 years ago
the returncost.diff i joined is wrong (there should be 2 cases) but happily wraitii did good
comment:41 by , 6 years ago
Description: | modified (diff) |
---|
Let's mention Palantius for its initial work on that
comment:42 by , 5 years ago
From #0ad-dev today:
(05:49:00 PM) borg-: elexis: for example, if you research the upgrade tower (sentry tower) to defense tower, and then search other some technology like murder holes, at the moment that defense tower is finished, it cancels the other technology. (05:49:45 PM) borg-: Manual upgrade cancel normal techs
comment:43 by , 4 years ago
In r19600 by s0600204:
Show entity upgrades in the Structure Tree
Reviewed By: bb
Trac Tickets: #4079
Differential Revision: https://code.wildfiregames.com/D92
In r23185 by Stan:
Allow artists to add upgrade animations.
Reviewed by: @Angen
Differential Revision: https://code.wildfiregames.com/D2371
comment:44 by , 4 years ago
Researching a technology while upgrading also seems wrong, because the upgraded entity may not be able to research the technology still in progress.
As noted in Phab:rP23185, the upgrade animation is cancelled when a researched tech is started during upgrade. If it was impossible to research a tech and upgrade simultaneously, that problem would probably be solved, and the problem borg- reported above.
comment:45 by , 4 years ago
refs Phab:D2652 The issue would be avoided if the Upgrade was performed in the ProductionQueue I guess, that would also allow queueing the Upgrade along with techs and trained entities, dunno.
comment:46 by , 4 years ago
Yes, but the entity one upgrades into does not necessarily have the same ProductionQueue. So either that has to be verified when upgrading (removing items from the queue if applicable) or the current state of Phab:D2652, i.e. not allowing both to be performed at the same time. (I'd vote for the last one actually, to prevent surprises of items being cancelled.
comment:47 by , 4 years ago
Not allowing to queue techs or entities after having queued an Upgrade would solve that equally with the same underlying logic (not allowing queued items after an Upgrade was stated) while allowing to queue the Upgraded action and reusing the same process (production queue) to perform an action. I guess its somewhat arbitrary how its designed and it doesnt invalidate the proposed bug avoidance patch.
Updated.