Opened 3 years ago

Closed 17 months ago

Last modified 3 months 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 wraitii)

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

Download all attachments as: .zip

Change History (57)

Changed 3 years ago by wraitii

Attachment: UpgradeComponent.patch added

Changed 3 years ago by wraitii

Attachment: UpgradeComponent.2.patch added

Updated.

comment:1 Changed 3 years ago by wraitii

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.

Changed 3 years ago by wraitii

Attachment: UpgradeComponent.3.patch added

RC

comment:2 Changed 3 years ago by wraitii

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

Changed 3 years ago by wraitii

Attachment: UpgradeComponent.4.patch added

Actual RC

comment:3 Changed 3 years ago by Itms

Hi wraitii, is this ready for A17?

comment:4 Changed 3 years ago by Itms

Milestone: Alpha 17Alpha 18

comment:5 Changed 3 years ago by Niek

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 3 years ago by Niek (previous) (diff)

comment:6 Changed 3 years ago by wraitii

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 Changed 3 years ago by Niek

Thank you for answering :)

comment:8 Changed 3 years ago by Lionkanzen

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 Changed 3 years ago by leper

Milestone: Alpha 18Alpha 19

comment:10 Changed 2 years ago by wraitii

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

comment:11 Changed 2 years ago by mimo

Milestone: Alpha 19Alpha 20

Changed 2 years ago by wraitii

Attachment: UpgradeComponent.5.patch added

Updated for A20

Changed 2 years ago by wraitii

Attachment: UpgradeComponent.6.patch added

Updated following elexis' comments.

comment:12 Changed 21 months ago by Itms

Milestone: Alpha 20Alpha 21

Changed 20 months ago by sanderd17

Attachment: UpgradeComponent.7.patch added

Update to A21 (SVN)

comment:13 Changed 20 months ago by sanderd17

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 Changed 20 months ago by fatherbushido

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 20 months ago by fatherbushido (previous) (diff)

comment:15 Changed 20 months ago by elexis

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)

Changed 20 months ago by fatherbushido

Attachment: UpgradeComponent.diff added

comment:16 Changed 20 months ago by sanderd17

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.

Changed 20 months ago by sanderd17

Attachment: UpgradeComponent.8.patch added

Changed 19 months ago by stanislas69

Attachment: UpgradeComponent.9.patch added

Rebase, command.js wouldn't apply.

comment:17 Changed 19 months ago by elexis

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

comment:18 in reply to:  16 Changed 19 months ago by elexis

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 Changed 18 months ago by wraitii

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.

Changed 18 months ago by sanderd17

Attachment: UpgradeComponent.10.patch added

Changed 18 months ago by wraitii

Fixed everything.

Changed 18 months ago by wraitii

Attachment: upgrade.png added

corresponding icon, goes in session/icons/

comment:20 Changed 18 months ago by wraitii

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 Changed 18 months ago by wraitii

Keywords: review added

comment:22 Changed 18 months ago by elexis

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

Changed 17 months ago by wraitii

comment:23 Changed 17 months ago by wraitii

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.

Changed 17 months ago by wraitii

comment:24 Changed 17 months ago by wraitii

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 Changed 17 months ago by elexis

Keywords: review removed

comment:26 Changed 17 months ago by fatherbushido

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)

http://trac.wildfiregames.com/raw-attachment/ticket/2706/screenshot.jpg

Last edited 17 months ago by fatherbushido (previous) (diff)

Changed 17 months ago by fatherbushido

Attachment: screenshot.jpg added

Changed 17 months ago by fatherbushido

Attachment: returncost.diff added

retrun cost when capturing or deleting

comment:27 Changed 17 months ago by Lionkanzen

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

comment:28 Changed 17 months ago by wraitii

In 18470:

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

comment:29 Changed 17 months ago by wraitii

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 Changed 17 months ago by fatherbushido

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

Last edited 17 months ago by fatherbushido (previous) (diff)

comment:31 Changed 17 months ago by elexis

In 18471:

Fix tests. Patch by fatherbushido, refs #2706.

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

comment:32 Changed 17 months ago by elexis

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 Changed 16 months ago by mimo

In 18532:

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

comment:34 Changed 16 months ago by fatherbushido

In 18586:

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

comment:35 Changed 14 months ago by elexis

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 Changed 14 months ago by elexis

In 18787:

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

comment:37 Changed 14 months ago by elexis

In 18791:

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

comment:38 Changed 14 months ago by fatherbushido

In 18796:

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

comment:39 Changed 13 months ago by Imarok

In 18927:

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

comment:40 Changed 3 months ago by elexis

In 20046:

JS Cleanup.

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

Refs #2706, D829

Last edited 3 months ago by stanislas69 (previous) (diff)
Note: See TracTickets for help on using tickets.