perhaps you can use 'option' instead of upKey as in Upgrade code

then use something like choice = template.Upgrade[upKey] wich is used many times

are you sure that's right?
With a quick overlook,
I think the key (tech_type) don't use the upKey (I guess getEntityValue function doesn't fit here).

same

It seems constistent with GuiInterface stuff.

you can use a for of as you only use upgrades[idx] and never idx itself
(it seems)

same here?

Not sure if we should nuke that.
The patch needs that to work?

I mean, imo we could keep the own resources cost (even if it's not use).
If the patch needs it to be nuked, imo it's bad (we could add another way to check that).

The entity itself can/must have his own Cost.
Moreover, the time at least is needed for the repair rate.

Since that question was asked by me in the ticket as well, adding a XML comment might be appropriate.

I could live with such an xml comment, and I understand why you want to add it but again it seems imo pointless (perhaps also a matter of taste).
We must have in mind that the intrinsic cost of an entity is something different (can even be not correlated) to the upgrade costs.
An entity can also have a cost component even if in common games (rms map) that entity is not in any Builder / ProductionQueue list (see other/celt_longhouse.xml...)
Moreover here, we just have to not touch at it, so how would look like such a comment? "Here you want perhaps remove the cost entry but don't do it?".
;-)

<!-- Inherit Cost, as the BuildTime determines the repair rate -> just to avoid further repetitions of the question. I could also live with not adding it though ;)

sure, I would emphasize that it's also bad to remove (or to have to remove) the not time ressource cost.

Yes, I'm sure that's right. If it wasn't, then the cost value wouldn't appear correctly in the structure tree.

How?

let phase = g_ParsedData.phaseList.indexOf(GetPhaseOfTemplate(upgrade.data)) < structPhaseIdx) ? structInfo.phase : GetPhaseOfTemplate(upgrade.data);`

or

let phase = GetPhaseOfTemplate(upgrade.data);
phase = g_ParsedData.phaseList.indexOf(phase) < structPhaseIdx ? structInfo.phase : phase;

Second one, as we don't want to repeat executing the function

It was more, how would a ternary make it better in this case, as imho neither option is more readable than what exists.

That's not a proof.
I mean if someone wants to use those ones in simulation.
Are you sure the modifications will be applied?
Could you check again (else I will :p)?

Indeed seems not really worth here (bb said "could" and not "must" btw)
(I guess it's more a matter of own style here).

Ah yes, sorry, things like a = b ? c : a should be avoided.

Applying P6 temporarily on top of the changes listed thus far, causes GUIInterface (the only simulation component currently using globalscripts/Templates::GetTemplateDataHelper()) to print out the entity name and upgrade cost of the first upgrade in any template it comes across (And no template has more than one upgrade, yet.) It yields the following output (repeated over and over):

WARNING: other/palisades_rocks_gate : ({stone:0, wood:20, time:5})
WARNING: structures/{civ}_defense_tower : ({stone:100, wood:50, time:100})
WARNING: structures/{civ}_wall_gate : ({stone:60, time:10})

You don't get what I meant.
I meant getEntityValue doesn't seems to fit here (relating to modification key).
I will describe it after.

What fatherbushido is referring to (I guess) is the fact that the ingame structree does not take any researched technologies into account when drawing structree. But this is done with all values see f.e. the walk/run speeds. This might needs to be fixed or all values, but that lies outside the scope of this patch imo.

Just wondering: can't those || undefined be removed?

Looping over an array with 1 element? nuke?

Is this new upgrade.entity not used only once, so can be inlined?

Just write a test file with TechnologyManager (just create your modification directly) and load your GetTemplateDataHelper with the player arg (don't forget to also load the ValueModification) helper.

Huh. Weird. There is indeed a problem (or two), but it's not here - its within the code of the Upgrade component... (patch for that incoming)

Test code: P18