Opened 10 years ago

Closed 8 years ago

Last modified 3 years ago

#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 fatherbushido)

(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)

UpgradeComponent.patch (38.5 KB ) - added by wraitii 10 years ago.
UpgradeComponent.2.patch (45.0 KB ) - added by wraitii 10 years ago.
Updated.
UpgradeComponent.3.patch (44.8 KB ) - added by wraitii 10 years ago.
RC
UpgradeComponent.4.patch (45.6 KB ) - added by wraitii 10 years ago.
Actual RC
UpgradeComponent.5.patch (45.9 KB ) - added by wraitii 8 years ago.
Updated for A20
UpgradeComponent.6.patch (45.9 KB ) - added by wraitii 8 years ago.
Updated following elexis' comments.
UpgradeComponent.7.patch (45.8 KB ) - added by sanderd17 8 years ago.
Update to A21 (SVN)
UpgradeComponent.diff (6 bytes ) - added by fatherbushido 8 years ago.
UpgradeComponent.8.patch (49.5 KB ) - added by sanderd17 8 years ago.
UpgradeComponent.9.patch (49.6 KB ) - added by Stan 8 years ago.
Rebase, command.js wouldn't apply.
UpgradeComponent.10.patch (45.3 KB ) - added by sanderd17 8 years ago.
UpgradeComponentRC-fixed4.patch (46.0 KB ) - added by wraitii 8 years ago.
Fixed everything.
upgrade.png (2.9 KB ) - added by wraitii 8 years ago.
corresponding icon, goes in session/icons/
UpgradeComponentRC-fixed5.patch (46.0 KB ) - added by wraitii 8 years ago.
UpgradeComponent-final.patch (47.5 KB ) - added by wraitii 8 years ago.
screenshot.jpg (27.3 KB ) - added by fatherbushido 8 years ago.
returncost.diff (1.3 KB ) - added by fatherbushido 8 years ago.
retrun cost when capturing or deleting

Download all attachments as: .zip

Change History (65)

by wraitii, 10 years ago

Attachment: UpgradeComponent.patch added

by wraitii, 10 years ago

Attachment: UpgradeComponent.2.patch added

Updated.

comment:1 by wraitii, 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.

by wraitii, 10 years ago

Attachment: UpgradeComponent.3.patch added

RC

comment:2 by wraitii, 10 years ago

Above patch actually introduced a ton of bugs. I'll fix them and commit.

by wraitii, 10 years ago

Attachment: UpgradeComponent.4.patch added

Actual RC

comment:3 by Itms, 10 years ago

Hi wraitii, is this ready for A17?

comment:4 by Itms, 10 years ago

Milestone: Alpha 17Alpha 18

comment:5 by Niek, 9 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?

Last edited 9 years ago by Niek (previous) (diff)

comment:6 by wraitii, 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:7 by Niek, 9 years ago

Thank you for answering :)

comment:8 by Lionkanzen, 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 leper, 9 years ago

Milestone: Alpha 18Alpha 19

comment:10 by wraitii, 9 years ago

I'd like this to be discussed in the next meeting.

comment:11 by mimo, 9 years ago

Milestone: Alpha 19Alpha 20

by wraitii, 8 years ago

Attachment: UpgradeComponent.5.patch added

Updated for A20

by wraitii, 8 years ago

Attachment: UpgradeComponent.6.patch added

Updated following elexis' comments.

comment:12 by Itms, 8 years ago

Milestone: Alpha 20Alpha 21

by sanderd17, 8 years ago

Attachment: UpgradeComponent.7.patch added

Update to A21 (SVN)

comment:13 by sanderd17, 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 fatherbushido, 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

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

comment:15 by elexis, 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 of selection_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 of data.item.cost and doesn't seem needed (if so, we could also use clone or deepcopy)

by fatherbushido, 8 years ago

Attachment: UpgradeComponent.diff added

comment:16 by sanderd17, 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 sanderd17, 8 years ago

Attachment: UpgradeComponent.8.patch added

by Stan, 8 years ago

Attachment: UpgradeComponent.9.patch added

Rebase, command.js wouldn't apply.

comment:17 by elexis, 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 is undefined, 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 wide
  • ChangeUpgradedEntityCount -> 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 with y.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 use for j in foo loops

in reply to:  16 comment:18 by elexis, 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 wraitii, 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 sanderd17, 8 years ago

Attachment: UpgradeComponent.10.patch added

by wraitii, 8 years ago

Fixed everything.

by wraitii, 8 years ago

Attachment: upgrade.png added

corresponding icon, goes in session/icons/

comment:20 by wraitii, 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 wraitii, 8 years ago

Keywords: review added

comment:22 by elexis, 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 in SelectionPanels.Upgrade.
  • Use let instead of var for newly introduced variables
  • Unneeded variables selection in cancelUpgradeEntity and item in getItems (L1016) which should just be inlined. Any reason at all why not to simply pushing upgrade (or a clone of that) instead of copying it to item that way?
  • isUpgrading and progress are defined long before they are used, move it down Perhaps referencing data.item.requiredTechnology (instead of copying it to requiredTechnology) 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 of if ... = true else ... = false
  • Still dont like the fact to put g_unitPanelButtons into a single line with 240 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 use let 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 use let hasCost instead of setting ito false, it will be undefined which eliminates the ternary
  • Not sure if we prefer x.y !== undefined over y in x (L170), ask someone else (likely noone cares)
  • GetRequiredTechnology could use some newlines
  • GetRequiredTechnology 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; like if (this.template[choice].RequiredTechnology) return this.template[choice].RequiredTechnology which might just be undefined 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 be function ChangeEntityTemplate(oldEnt, newTemplate)

by wraitii, 8 years ago

comment:23 by wraitii, 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 wraitii, 8 years ago

comment:24 by wraitii, 8 years ago

Owner: set to wraitii
Resolution: fixed
Status: newclosed

In 18467:

Allow entities to upgrade into other entities.

This new components allows giving the upgrade a cost, required technologies, and a required time.
Implement gates using this generic component.
Fixes #2706

comment:25 by elexis, 8 years ago

Keywords: review removed

comment:26 by fatherbushido, 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
Version 0, edited 8 years ago by fatherbushido (next)

by fatherbushido, 8 years ago

Attachment: screenshot.jpg added

by fatherbushido, 8 years ago

Attachment: returncost.diff added

retrun cost when capturing or deleting

comment:27 by Lionkanzen, 8 years ago

Don't forget the Roman army camp upgrade to CC.

comment:28 by wraitii, 8 years ago

In 18470:

Fix to the upgrade commit, including one by fatherbushido. Refs #2706

comment:29 by wraitii, 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 fatherbushido, 8 years ago

the returncost.diff i joined is wrong (there should be 2 cases) but happily wraitii did good

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

comment:31 by elexis, 8 years ago

In 18471:

Fix tests. Patch by fatherbushido, refs #2706.

Some simplifications and array functions.
Fix indentation, thanks pastebin.

comment:32 by elexis, 8 years ago

In 18472:

Upgrade component cleanup, refs #2706.

Use the cheat multiplier for upgrading and un/packing.
Remove if-statements that are always true.
Remove unused undefined "time" item from array in the selection panel.
Pass that object directly instead of copying each property explicitly.
Various indentation fixes in the selection panel code.

comment:33 by mimo, 8 years ago

In 18532:

fix a call to DeleteEntityAndReturn with undefined arguments, + some cleanup, refs #2706

comment:34 by fatherbushido, 8 years ago

In 18586:

Update translation messages after the removal of the gate conversion tooltip in r18467. Reviewed by elexis. Refs #2706.

comment:35 by elexis, 8 years ago

In 18754:

Make wooden tower upgradable to the defense tower. Reviewed by fatherbushido, siole and Hannibal Barca, fixes #4125.

Let towers cost 50 wood and reduce the stone cost by 50.
The upgrade costs so much that it's cheaper to build a stone tower than building and upgrading a wooden tower, but still cheaper than building a wooden and stone tower.
Increase the minimum distance between wooden tower to 60, so that the upgrade won't circumstance the minDistance of the stone tower.
Add missing newline to the entity limit tooltip of the upgrade component (refs #2706) that was never visible before.

comment:36 by elexis, 8 years ago

In 18787:

Display the correct cost when selecting multiple entities that can perform the same upgrade. Patch by Imarok, refs #2706, #3823.

comment:37 by elexis, 7 years ago

In 18791:

Don't refund the upgrade costs when the upgrade completes. Patch by fatherbushido, fixes #4255, refs #2706.

comment:38 by fatherbushido, 7 years ago

In 18796:

Fixes a counter mistake when an upgrade is complete. Reviewed by elexis. Refs #4255, #2706.

comment:39 by Imarok, 7 years ago

In 18927:

Disable upgrade buttons to observers. Reviewed by elexis. Fixes #4302 Refs #2706

comment:40 by elexis, 7 years ago

In 20046:

JS Cleanup.

Don't change the Upgrade costs in the entity state when displaying the Upgrade tooltip.

Refs #2706, D829

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

comment:42 by elexis, 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 elexis, 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 elexis, 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 elexis, 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 Freagarach, 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 elexis, 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.

comment:48 by Silier, 3 years ago

In 24088:

Do not allow upgrading when entity is producing and vice versa.

Before this patch, when entity was upgrading and producing and finished upgrading before production, production was canceled. That meant player assumed unit/tech will be ready in certain time but it will not. Also fixing interference between upgrade and production animations.

Differential Revision: D2652
Reviewed by: bb
Comments by: Stan, Freagarach
Fixes: #5749
Refs: #2706

Note: See TracTickets for help on using tickets.