Details

Reviewers

Commits

rP19379: Fix application of tech modifications within the Upgrade component. Patch by…

Summary

The problem is that the Upgrade component passes a string (the name of a template) where ApplyValueModificationsToTemplate expects an object (the template data).

(This is exacerbated by the fact that it's not even the correct template name. In this part of the code, you're modifying the cost of a given upgrade, the values of which are stored within the template of the current entity, not in the template of the upgrade.)

~~The second problem is that the component doesn't provide the correct modifications path.~~ My mistake... ?

Thanks to @fatherbushido for drawing my attention to this (even if I'm not too sure he meant to ?)

Test Plan

P18 was a test written for D92, but also happens to test the Upgrade component.

Copy the paste's content into a new file within the folder binaries/data/mods/public/simulation/tests
Comment out tests marked as T1, T2, T4 and T5 within that file (and save the file)
Run the usual test application found in binaries/system application.

If it runs successfully, then all should be well.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

s0600204 created this revision.Feb 17 2017, 1:11 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/365/ for more details.

Harbormaster completed remote builds in B536: Diff 538.Feb 17 2017, 1:56 AM

Imo we should keep the not option related modification, dunno. The matter you encounter in D92 is more related to the fact that the subfunction getentityvalue doesn t fit in that case.
(wild quick comment)

In D146#5172, @fatherbushido wrote:

Imo we should keep the not option related modification, dunno.

I humbly disagree. Upgrade/Cost/* would never apply to anything.

In D146#5182, @s0600204 wrote:

In D146#5172, @fatherbushido wrote:

Imo we should keep the not option related modification, dunno.

I humbly disagree. Upgrade/Cost/* would never apply to anything.

That time it's more a design remark.
I mean it's weird if we must use a tech/global aura for each kind of upgrade to modify upgrade costs/time.

(edit: moreover, tell me if I m wrong, but iirc the value/property name used for modifications is just a key in the modifications cache objects stored in manager, it doesn't need to match with any template entry).

Breakage in the Upgrade code, colour me shocked, shocked I say!

For the tech identifier used we could provide Upgrade/Cost/ and Upgrade/foo_bar/Cost. Applying to all and only to foo_bar respectively. We could also rip it out given that it is broken, but I guess fixing it and making it work for the specific upgrade case only should be fine.

binaries/data/mods/public/simulation/components/Upgrade.js
129 ↗	(On Diff #538)	`templateName` would be a better name for this line, but not for the next. However this one can just be inlined in the next one, might be a little long though.
130 ↗	(On Diff #538)	Move this out of the loop, it doesn't depend on the loop variable, and just because it might end up outside of the loop once the JIT optimizations kick in doesn't mean we should write code that does useless work.

s0600204 mentioned this in D92: Show entity upgrades in the Structure Tree.Feb 17 2017, 4:42 PM

In D146#5194, @leper wrote:

For the tech identifier used we could provide Upgrade/Cost/ and Upgrade/foo_bar/Cost. Applying to all and only to foo_bar respectively.

We could, but I think that can wait for another day. (It would also need a note put somewhere informing modders which is applied first...)

Nice to see you around, by the way.

binaries/data/mods/public/simulation/components/Upgrade.js
130 ↗	(On Diff #538)	Good point.

Correct misunderstanding on my part, and move a variable

s0600204 marked 2 inline comments as done.Feb 17 2017, 5:16 PM

For the tech identifier used we could provide Upgrade/Cost/ and Upgrade/foo_bar/Cost. Applying to all and only to foo_bar respectively.

We could, but I think that can wait for another day. (It would also need a note put somewhere informing modders which is applied first...)

True, not complicating it if it needn't be is to be preferred.

Nice to see you around, by the way.

Same.

binaries/data/mods/public/simulation/components/Upgrade.js
125 ↗	(On Diff #544)	This seems like we could extend the TemplateManager to just return the right thing (eg GetCurrentTemplate()), but a quick search only shows one other occurence of this (in Cost.js), so probably not worth it.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/368/ for more details.

Harbormaster completed remote builds in B539: Diff 544.Feb 17 2017, 6:30 PM

Well, I finally look at it.
There is another thing wrong.
The time/resources are got in two places. One is for GuiInterface, the other is for the Upgrade component itself to reduce/reward upgrade cost.
One allows local auras modifications (see GetResourcesCost) and not the other one.

We could perhaps use ApplyValueModificationsToEntity in both cases. I don't remember at the moment I write that if that's done (iirc it's not but if so we should perhaps store the substracted cost to avoid someone using a local aura to have a bigger reward using this local aura).
Else using ApplyValueModificationsToTemplate in both cases is ok too and avoid those complication. Perhaps better in fact :)

fatherbushido added inline comments.Mar 1 2017, 5:19 PM

binaries/data/mods/public/simulation/components/Upgrade.js
144 ↗	(On Diff #544)	(It's ok to not check null Player here (in case the entity is in the destroy process) to get error and catch components which do bad stuff.)

fatherbushido requested changes to this revision.Mar 1 2017, 9:38 PM

This revision now requires changes to proceed.Mar 1 2017, 9:38 PM

Use ApplyValueModificationsToEntity instead of ApplyValueModificationsToTemplate

(Test in P18 has also been updated to take into account changes made to this revision)

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/457/ for more details.

Harbormaster completed remote builds in B667: Diff 689.Mar 3 2017, 2:53 AM

fatherbushido added inline comments.Mar 3 2017, 8:16 AM

binaries/data/mods/public/simulation/components/Upgrade.js
230 ↗	(On Diff #689)	Perhaps not in the initial scope of the patch but we should copy/clone/cache the subtraced resources here in case we reward them.
256 ↗	(On Diff #689)	Perhaps not in the initial scope of the patch but that needs to be fixed, we should cache/clone values in Upgrade. Correct me if I m wrong.

Good catch, though it only affects the GUI. In fact Upgrade.prototype.GetUpgrades should just use GetUpgradeTime and GetUpgradeCosts (passing the correct template), it seems to me.
@fatherbushido is imo correct that the cost should be cached on upgrade start, so that we don't accidentally give back fewer or more ressources than necessary.

Cache resource cost so as to return correct quantities on upgrade cancellation
Remove duplicated code

s0600204 marked 2 inline comments as done.Mar 3 2017, 7:03 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/463/ for more details.

Harbormaster completed remote builds in B677: Diff 700.Mar 3 2017, 8:05 PM

In game tests are ok.
Give me your opinion about the inlined remarks. (Tell me if I am wrong!)
Else it's ok.

binaries/data/mods/public/simulation/components/Upgrade.js
207 ↗	(On Diff #700)	Quick remark before final review. The GetUpgrades() method won't need that case. So I don't really see why keeping that here. (Quickly looking).
232 ↗	(On Diff #700)	I would move L226 here (and cloning) and removing L229 if nuking L205-L207

I set "request changes", in fact it's "request your input".

This revision now requires changes to proceed.Mar 20 2017, 10:19 PM

s0600204 added inline comments.Apr 3 2017, 10:00 PM

binaries/data/mods/public/simulation/components/Upgrade.js
207 ↗	(On Diff #700)	Why wouldn't it need this case?
232 ↗	(On Diff #700)	L226 needs to be before L227, else `cmpPlayer.TrySubtractResources()` gets passed an empty object.

fatherbushido added inline comments.Apr 3 2017, 10:25 PM

binaries/data/mods/public/simulation/components/Upgrade.js
207 ↗	(On Diff #700)	I don't remember! I was asking your opinion about that so perhaps I wasn't conviced myself. That method is called by GetUpgrades (mainly a gui stuff) and in Upgrade methods. Uhm, I will recover memory if you don't before.
232 ↗	(On Diff #700)	Sure I meant L226 let res = this.GetResourceCosts(template); L227 if (!cmpPlayer.TrySubtractResources(res)) And cloning/caching in this.expendedResources only if we effectively substract something.

fatherbushido added inline comments.Apr 3 2017, 10:28 PM

binaries/data/mods/public/simulation/components/Upgrade.js
207 ↗	(On Diff #700)	Well I start to remember, while performing an upgrade we doesn't display anything.

To be more precise, my wonder is the following: the internal cloning/caching should be done imo only
in Upgrade.prototype.Upgrade which is the place where we need it and not in the getter.
(Just a wonder ;-)).

refs D281

In D146#11457, @fatherbushido wrote:

To be more precise, my wonder is the following: the internal cloning/caching should be done imo only
in Upgrade.prototype.Upgrade which is the place where we need it and not in the getter.
(Just a wonder ;-)).

No, the clone is needed in the getter not the setter. We're using it to protect the value of this.expendedResources, not the value derived from the template (which is recalculated every gui update anyhow).

In D146#11459, @wraitii wrote:

refs D281

If the plan is to replace the Upgrade component, should work on this revision continue? If the entire file is to be scrubbed? (Honest query, not intending to sound grumpy or cause offence.)

binaries/data/mods/public/simulation/components/Upgrade.js
207 ↗	(On Diff #700)	We might not display the cost values whilst an upgrade is in progress but `GetUpgrades()` is still run for selected entities and the values calculated are passed to the gui (which then doesn't show them to the player). There is a possibility that the gui may be altered at a later date to display costs so players can see how much they'd get back. Also, remember that the simulation/entity states are also passed to the AI (via its proxy) and whilst it might not do anything with it yet, the AI might one day become smart enough to cancel upgrades-in-progress in response to, say, a sudden attack where it needs the resources.
232 ↗	(On Diff #700)	Whilst I can use a temporary `res` variable, I disagree on moving the `clone` to here. It exists where it is to protect the value of the `expendedResources` property from being unintentionally altered by subsequent code (either in `GetUpgrades()` or external to the `Upgrade` component).

There is a possibility that the gui may be altered at a later date to display costs so players can see how much they'd get back. Also, remember that the simulation/entity states are also passed to the AI (via its proxy) and whilst it might not do anything with it yet, the AI might one day become smart enough to cancel upgrades-in-progress in response to, say, a sudden attack where it needs the resources.

Perfect argument (I had the first in mind, but not at all the second). Thanks for your input!

fatherbushido added inline comments.Apr 4 2017, 5:04 PM

binaries/data/mods/public/simulation/components/Upgrade.js
232 ↗	(On Diff #700)	forget that, it was in the case we do things here and not in the getter.

If the plan is to replace the Upgrade component, should work on this revision continue? If the entire file is to be scrubbed? (Honest query, not intending to sound grumpy or cause offence.)

Well ideally the plan would be to review D281 to check if this problem still applies. I'm fairly certain the cost thing is still in, since I've put a TODO for that, but not sure about the technology issue itself.
In the meantime, this can get accepted/committed imo since I don't expect D281 to be reviewed in the next few minutes…

Use a local variable instead of a component property.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/695/ for more details.

Harbormaster completed remote builds in B1011: Diff 1098.Apr 4 2017, 10:21 PM

Reverted most recent change.

In a forum private message, @fatherbushido wrote:

Dunno if the last change was needed.

I'd agree.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/699/ for more details.

Harbormaster completed remote builds in B1018: Diff 1110.Apr 5 2017, 5:18 PM

Closed by commit rP19379: Fix application of tech modifications within the Upgrade component. Patch by… (authored by fatherbushido). · Explain WhyApr 5 2017, 5:28 PM

This revision was automatically updated to reflect the committed changes.

s0600204 mentioned this in D519: Add test for Upgrade Component.May 19 2017, 6:26 PM