#2045 closed defect (fixed)
[PATCH] Cache technology modifiers based on both name and value
Reported by: | O.Davoodi | Owned by: | O.Davoodi |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 15 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description
Currently we only cache the technology modifiers based on the name of the value. In the current code, if the value changes, it is not registered in the TechManager and the older value is returned.
It has already broken the "TARDIS" cheat, which changes the player's maximum population. It can also cause problems in future (for example, when implementing a stat changer trigger effect).
To fix this, we can make the game cache the modifier based on both name and value.
Attachments (3)
Change History (13)
by , 11 years ago
Attachment: | techmanager-cache-value.patch added |
---|
comment:1 by , 11 years ago
Keywords: | patch added |
---|
comment:2 by , 11 years ago
Keywords: | cache technology modifier removed |
---|
I'm quite certain that delete this.modificationCache[valueName][ent];
will not work as it should be cacheBase instead of ent.
follow-up: 6 comment:5 by , 11 years ago
I'm not sure if clearModificationCache()
works correctly, as I think the break could lead to missing some modifications. If you have two modifications with different curValue
s clearing the cache will just remove the first cached value, so I'd opt for removing the break
.
I'm not really sure if the current way is the best way of handling this, but I can't think of a better way currently and if it works (and is properly tested) it should be committed IMO.
While we're at it we could add some tests for the TechnologyManager
.
comment:6 by , 11 years ago
I'm not sure if
clearModificationCache()
works correctly, as I think the break could lead to missing some modifications. If you have two modifications with differentcurValue
s clearing the cache will just remove the first cached value, so I'd opt for removing thebreak
.
This is the case if the older modifications are actually not removed, because I don't see any way for us to have two caches with different curValue's unless we actually have not cleared the cache somewhere else.
While we're at it we could add some tests for the
TechnologyManager
.
I'm afraid I don't know how to do it. I have seen the test folders but don't know what do they do exactly.
comment:7 by , 11 years ago
Keywords: | review added |
---|---|
Milestone: | Alpha 14 → Alpha 15 |
by , 11 years ago
Attachment: | techmanager-cache-values.diff added |
---|
comment:8 by , 11 years ago
I think my patch is a bit cleaner, conceptually. As it doesn't allow one ent with two different old values to be cached.
comment:10 by , 10 years ago
Keywords: | review removed |
---|
A patch to fix the issue