Fix D92/rP19600 not handling upgradeable units correctly.
ClosedPublic
Actions

Authored by s0600204 on May 18 2017, 11:37 PM.

Details

Reviewers

wowgetoffyourcellphone

Commits

rP19711: Fix rP19600 not handling upgradeable units correctly.

Trac Tickets

#4079

Summary

@wowgetoffyourcellphone reported having problems on the forums with the Structure Tree when running his Delenda Est mod on top of the current (at the time of writing this) svn state.

This is due to this mod having something that vanilla-0ad doesn't have - units that can be individually upgraded.

Part of the changes proposed in D92 was code intended to detect these units-with-upgrade-options and display them with the Unit Trainers on the right-hand side of the structree. As vanilla-0ad doesn't have any such units, clearly this code was not tested fully (by myself or the reviewers of that revision) and thus was committed broken in rP19600.

This revision seeks to fix that failure.

Test Plan

Run 0ad with the Delenda Est mod enabled, and view the Structure Tree. It should draw correctly, and not throw any unexpected warnings or errors (see notes).

Note: You will get a warning about the Iphicratean Reforms technology belonging the athen civ not being researchable anywhere. This is an intentional design decision by @wowgetoffyourcellphone, and can be ignored.

Note: You will get errors about the structree not having enough space, ie:

ERROR: "cart" has more structures in phase phase_village than can be supported by the current GUI layout
ERROR: The trainer units of "maur" have more production icons than can be supported by the current GUI layout

These are outside the scope of this revision, and can be safely ignored (for now).

Note: The two warnings from globalscripts/l10n.js are caused by Delenda Est's ptol_hero_cleopatra_3 aura lacking both a name and a description.

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.May 18 2017, 11:37 PM

Build is green

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

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

Harbormaster completed remote builds in B1798: Diff 2020.May 19 2017, 12:48 AM

s0600204 mentioned this in D295: Relocate and Rearrange structree code.May 19 2017, 3:42 PM

Hi, I tried these files and I don't see the Upgrade units being displayed in the Trainer bar. Is that the intention?

In D517#23537, @wowgetoffyourcellphone wrote:

Hi, I tried these files and I don't see the Upgrade units being displayed in the Trainer bar. Is that the intention?

You still have an old copy of globalscripts/Templates.js in your mod (hint, hint).

Oh thank you. I forgot it was in globalscripts. I thought it was in the components folder and when I saw it wasn't there, I thought I had removed it. I removed the offending file and reapplied the patch.

Result:

Everything works as you advertised! 'Upgrade' units are now in the Trainer Units column, though now the title of "Trainer Units" for that space seems a bit odd to me. Unfortunately, it'll take some thinking to come up with a better title for that space.

Suggested changes/extensions:

For buildings only, it would be great, if it has a production queue, to see the Upgraded building in the Phase line that it becomes available in. That way we can see the techs and/or units it has. Really good examples are the Stone Towers for all civs and Royal Barracks for Macedonians. The Stone Towers offer up some techs that would be useful to see in the tree, and the Royal Barracks allows the training of champions, which would also be useful to see in the tree. Either that, or put that information in the tooltip?

@wowgetoffyourcellphone wrote:

For buildings only, it would be great, if it has a production queue, to see the Upgraded building in the Phase line that it becomes available in. That way we can see the techs and/or units it has.

Good point, that should be done. Yet another thing that vanilla-0ad doesn't have but your mod does (in vanilla, all structure upgrades (with the exception of wall-gates) can be built independently at a later point in the game).

I initially thought that this should be fairly easy to do. And in a way, I was right. Displaying Upgrades next to the structures - fairly easy. Not showing Upgrades that don't have production queues (such as wall gates) - a little messy, but also reasonably easy. However, displaying the correct information inside the tooltips of the Upgrades - nope!

If we take the stone towers as an example, athen_defense_tower_stone displays in its tooltip a cost of "100 stone". This is inaccurate as the cost to get a stone tower is the cost of the base structure (athen_defense_tower: 100 wood) plus the cost of the upgrade (100 stone). Thus, the cost displayed in the upgrade's tooltip (when it's drawn separately) should be "100 wood, 100 stone" (along with a message informing the player that its an upgrade), or not shown at all. And the cost is not the only inaccuracy (the stone tower incorrectly appears in the village phase). To modify the structree to show upgrades-with-production-queues next to structures, and to do it properly, requires, it would seem, a fairly sizable code change.

So, what I'd like to suggest is that if you "accept" this revision, I can then commit it so as to get the changes into vanilla-0ad. That would mean that you could remove the copy of globalscripts/Templates.js from your mod (one less file for you to have to track changes to...).

Then, once D295 is also reviewed, accepted and committed (because I think the changes that will need to be made will be easier and cleaner after that), I can revisit the upgrades-with-production-queues problem and find a way to display them (and properly). Okay?

elexis awarded a token.May 31 2017, 2:56 AM

wowgetoffyourcellphone accepted this revision.Jun 1 2017, 11:09 AM

This revision is now accepted and ready to land.Jun 1 2017, 11:09 AM

In D517#23832, @s0600204 wrote:

So, what I'd like to suggest is that if you "accept" this revision, I can then commit it so as to get the changes into vanilla-0ad. That would mean that you could remove the copy of globalscripts/Templates.js from your mod (one less file for you to have to track changes to...).

Then, once D295 is also reviewed, accepted and committed (because I think the changes that will need to be made will be easier and cleaner after that), I can revisit the upgrades-with-production-queues problem and find a way to display them (and properly). Okay?

Sounds good. Revision accepted.

wowgetoffyourcellphone awarded a token.Jun 1 2017, 11:12 AM

Closed by commit rP19711: Fix rP19600 not handling upgradeable units correctly. (authored by s0600204). · Explain WhyJun 1 2017, 8:29 PM

This revision was automatically updated to reflect the committed changes.