Persian architecture bug ?

[mimo]

With the new technology Persian architecture, buildings are no more fully built.

For example, houses before the tech have health 1200; and after the tech, their max health is increased to 1500, but they are only built up to 1200. You have to reoder your builders to finish it to reach 1500 (houses built before the tech are them automatically upgraded to 1500).

I'm on build 12759 from 0ad.dev/ubuntu

[mimo]

Looking a bit at the code for this tech problem, I see several points on it :

1) foundations never profit from technology, so the building is always build with the original health before the tech. The reason seems to come from these lines in simulation/components/TechnologyManager.js in TechnologyManager.prototype.OnGlobalOwnershipChanged

if (Engine.QueryInterface(msg.entity, IID_Foundation)) return;

Commenting these lines fixes this bug. But I was wondering why these lines were there ? any potential problem removing them ?

2) there is nonetheless a small residual problem after that, which is when a foundation starts before the tech, and finish after. Still the construction does not reach the 100% health, which is because the math used there is not fully correct if the buildTime changes during construction.

that's quite easy to fix, but it depends on what we want the construction to do : in the present implementation, if during the construction the building undergoes an attack and its health decreases by a given amount, the construction will stop at the maxHealth minus this given amount. I think it would be much more logical if the construction goes up to the full health while increasing the buildTime accordingly to repair the damages.

[Yves]

I've made a patch for the first problem. I think it's intended that existing buildings only increase their maxHitpoints because it is more logical and makes sense for gameplay (otherwise it wouldn't make any sense to get this tech before you're under attack).

[Yves]

We had a discussion on IRC. ​http://irclogs.wildfiregames.com/2012-11-18-QuakeNet-%230ad-dev.log To sum it up: OnTechnologyModification in Health.js already contains much or all of the required functionality. I think the consensus was to also remove the "build-time"-modification and increase the cost for this technology. Increased build-time is bad for the very beginning of the game and makes the decision about when the tech should be researched a "fake-decision". If you get full health with less build-time, why should you ever research it early?

[historic_bruno]

Replying to Yves:

I've made a patch for the first problem. I think it's intended that existing buildings only increase their maxHitpoints because it is more logical and makes sense for gameplay (otherwise it wouldn't make any sense to get this tech before you're under attack).

Can you clarify this? It doesn't sound like something we intended but I may be misunderstanding.

About the patch: there's a reason that tech modifications aren't applied in GetMaxHitpoints, namely that in order to handle maintaining current health level when the Health/Max tech is researched, you need to know the pre-modified max hitpoints. By the time you call that function, it's too late. This is why the logic is all in Health.OnTechnologyModification which handles both newly created entities and existing entities when the tech is researched. Both functions can't have the tech modifications logic, or you accidentally apply the modification twice.

#1410 goes into detail on the reasoning behind the current design, though the fact it doesn't work for foundations is an oversight.

[Yves]

I've added another patch. It only fixes the problem that the modified max-health wasn't applied correctly. It doesn't change any other behaviour that was suggested in this thread.

The problems were:

1. As mimo said Foundations were filtered in OnGlobalOwnershipChanged. As far as I understand the classes count and such is used for requirements like "you need x buildings of type y". Foundations shouldn't be counted there because the buildings should actually be built before they count to the required number of buildings. However the foundations have to be taken into account for technology modifications.

2. Foundation.js cached the max-health which is not necessary IMO and caused problems because that cache wasn't refreshed with the tech-modifications

3. Foundation.js used a value "addedHitpoints" for hitpoints that have been added in the build-process. But this value became faulty when the added hitpoints changed to keep the percentage after the max-hitpoints were increased.

[Yves]

In 12960:

Fixes #1700 (Increasing max-health by tech)