Allow units without Cost component and use it for Relics
ClosedPublic
Actions

Authored by elexis on Apr 14 2017, 4:00 PM.

Details

Reviewers

Sandarac

Commits

rP19419: Allow units without Cost component and use it for the Catafalque template.

Summary

As mentioned by leper in rP19345, it should be possible to have units without a Cost component.
In particular for relics having the component is pointless as the cost should be strictly zero.

Test Plan

Search for IID_Cost in the code and see that it is only expected by foundations and repairable units. The other components have checks in place besides this one.

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

elexis created this revision.Apr 14 2017, 4:00 PM

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/764/ for more details.

Harbormaster completed remote builds in B1114: Diff 1243.Apr 14 2017, 6:39 PM

elexis added a reviewer: Sandarac.Apr 16 2017, 6:42 AM

The diff seems to to be okay - note that there is no check in TryConstructBuilding in Commands.js (I mention this because it seems to be the most risky of the missing checks, but this is for buildings).

This revision is now accepted and ready to land.Apr 16 2017, 7:37 PM

In D334#13433, @Sandarac wrote:

The diff seems to to be okay - note that there is no check in TryConstructBuilding in Commands.js (I mention this because it seems to be the most risky of the missing checks, but this is for buildings).

That code places a foundation, so see Foundation.js

	if (!cmpCost)
		error("A foundation must have a cost component to know the build time");

and to me it seems that using some hardcoded 100second buildtime if we don't have Cost.BuildTime isn't specified would be ugly and not helping. So I'd rather keep this dependency for all repairable entities (see GetRepairRate) and Foundations.

Thanks for the review!

In D334#13448, @elexis wrote:
In D334#13433, @Sandarac wrote:

The diff seems to to be okay - note that there is no check in TryConstructBuilding in Commands.js (I mention this because it seems to be the most risky of the missing checks, but this is for buildings).

That code places a foundation, so see Foundation.js
	if (!cmpCost)
		error("A foundation must have a cost component to know the build time");

Notice however that the unchecked call cmpCost.GetResourceCosts(player) is done before the call to InitialiseConstruction in TryConstructBuilding, which was my original point (in any case not really relevant, as errors will be thrown anyways if the foundation/building does not have a cost component - as they should be - in that particular case).