As described here, units often construct buildings at a slower rate than they should. This should fix that.
Details
Details
- Reviewers
fatherbushido wraitii - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP19625: Fix the building construction rate for multiple units.
Test that buildings are constructed at the correct rate.
Diff Detail
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
Comment Actions
Add yourself to the credits as described in https://trac.wildfiregames.com/wiki/SubmittingPatches.
Comment Actions
As said on the suggestion thread, nice catch with this bug.
I think ultimately we ought to change the "Build" function to run on a timer when at least one unit is building, so that we don't recompute the exact same thing for X different units several times, but that's for later.
Comment Actions
Note that we have mostly the same part of code in Repairable code (which seems a bit more clean and didn't have that floor issue).
It's also funny that we had the test checked the formula without the floor (but don't caught that as we don't get decimals in it).
| ps/trunk/binaries/data/mods/public/simulation/components/Foundation.js | ||
|---|---|---|
| 272 | That formula is itself a bit strange. Here we have two cases:
That means that if we ask to build 5, if maxHealth is 1000, if we this.GetBuildRate() * this.buildMultiplier is 0.5, then we'll build 5? I am a bit tired so be lenient if I am wrong. | |
Comment Actions
I didn't know if there would be any unintended consequences so I just made the minimal change by removing floor. But yes I agree that the max/min stuff is wrong or unnecessary. (Max health is checked during cmpHealth.Increase(deltaHP).) So I think this will work the same:
var deltaHP = work * this.GetBuildRate() * this.buildMultiplier;
You're asking if we could ever have this.GetBuildRate() * this.buildMultiplier < 1. I think the smallest GetBuildRate() can be is 5000/1000 = 5hp/s for a wonder (or a field), so we would need buildMultiplier < 0.2. To get that, we'd need over 200 units building it since 200**0.7/200 = 0.204. So I don't think we ever see work > work * this.GetBuildRate() * this.buildMultiplier during a game.
Comment Actions
Sure you did good in taking care of only what you noticed ;-) (It's not a complain)
But yes, we can have that during a 'game', one should not rely on current template values.
Comment Actions
Yes, I was only exploring when that could happen. And to be extra clear, in those cases it should still use deltaHP = work * this.GetBuildRate() * this.buildMultiplier, not deltaHP = work. So the current code is bugged, but it's rare/impossible to produce the bug given the current template values.
While I was fixing this I noticed that the build time tooltip was wrong. I tried to patch that with D522, so please take a look at that if you haven't already. I'll add a screenshot and a bit more explanation.