This.isInfinite is no longer serialized

Is this is the only behavior change in this commit? Behavior changes should be mentioned in the commit message (and before cosmetics).

@Imarok, do you have an opinion on the implementation of the serialization change?

Okay noted for next time.
Used a Boolean to store whether the resource was exhausted instead of checking it twice.
Bottom function now has early returns.

In rP22103#32453, @elexis wrote:

This.isInfinite is no longer serialized

Is this is the only behavior change in this commit? Behavior changes should be mentioned in the commit message (and before cosmetics).

@Imarok, do you have an opinion on the implementation of the serialization change?

Oh, f*ck. Have overlooked that. Thank you for noticing.
Afaics it's ok to remove it, but as I'm quite a simulation noob, what do you think @elexis ?

The default review questions are:

• What are the properties and implications of the existing implementation? Which of them are desirable and which would be preferably avoidable?
• What are the properties and implications of the proposed implementation? Comparing to the previous implementation, are the stakeholders (players, devs, modders, players hardware components like memory and cpu) better off?
• Which alternative implementation can we imagine?
• Which implementation of all these will be the best?
• Synthesis: Can we imagine a new implementation that keeps the advantages of all proposed models while leaving as few disadvantages as possible?

In rP22103#32457, @elexis wrote:

The default review questions are:

• What are the properties and implications of the existing implementation? Which of them are desirable and which would be preferably avoidable?
• What are the properties and implications of the proposed implementation? Comparing to the previous implementation, are the stakeholders (players, devs, modders, players hardware components like memory and cpu) better off?
• Which alternative implementation can we imagine?
• Which implementation of all these will be the best?
• Synthesis: Can we imagine a new implementation that keeps the advantages of all proposed models while leaving as few disadvantages as possible?

Sure. Guess what I thought about before doing my last post. ^^ I even looked through the whole history of that property. But as I'm not that experienced with simulation, I can't say for sure that not serializing that property is ok. For me it seems to be good and won't cause issues, but I can't say for sure because of a lack of experience in that field.

The only alternative implementation that I can imagine is to keep the cache property, thus avoid the repeated parsing + infinity test, while still not serializing the property.
That means there is a higher memory footprint (not unlikely tens of thousands of trees) * few bytes but also many less CPU cycles.

Secondly, there is at least one more cache variable, why not treat that equally.

Serialization is easy, there is a default serialization function that just converts every member property to bytes. If the simulation component has an own Serialize function, that one will be called instead, and the return object will be serialized. Same with the deserialize function. (It's in the ComponentManager cpp file IIRC)

From D1718#inline-34356

possibly cached in a non-serialized variable? (the latter is easy, there are examples of sim components that replace Serialize and Deserialize, the Serialize function just returns an object with all values to be serialized (i.e. excluding the cahce variables) and the Deserialize function copies all these given object properties to this and reinitializes the cache variable). As there already are some cache variables, this could be done separately.

So the question is whether IsInfinite is called so often that it's worth the cache variable, considering that there can be 10k resource supply entities (trees), or whether the additional memory usage is worse; same for this.cachedType.

How could we decide the memory/performance tradeoff?

In this patch, IsInfinite() is called only in TakeResources, i.e. comparatively rarely, the string to number conversion is cheap and the isinfinity test is cheap too.
A JS Number can store what a C++ double can store, i.e. up to 2^53, so at least 64bit for every resourcesupply entity. That'd be at least 8 byte * 10k trees = 80KB. Probably 250KB with all the memory management overhead. Quite considerable if we only need it very rarely.
Actually there could be 150 gatherers * 8 players = 1200 units calling TakeResources every second.
But the cmpFogging.Activate(); will have a much worse performance effect than the isInfinite function call.
The patch was meant as a preparation for D1718, that also only uses it once upon Init.

So the question is probably more whether we want to recommend using unserialized cache variables in general or whether we want to recommend not using them in general.
Given that we want to optimize out the simulation performance as much as possible, even at tolerable memory cost, it's probably better to use them?

Perhaps even better would be not having to convert to a number to begin with using +this.template.Amount, but changing the template manager to construct an object where that property is a number already.

(TLDR: blabla no concern)