Changeset View
Standalone View
binaries/data/mods/public/simulation/components/ResourceSupply.js
function ResourceSupply() {} | function ResourceSupply() {} | |||||||||
ResourceSupply.prototype.Schema = | ResourceSupply.prototype.Schema = | |||||||||
"<a:help>Provides a supply of one particular type of resource.</a:help>" + | "<a:help>Provides a supply of one particular type of resource.</a:help>" + | |||||||||
"<a:example>" + | "<a:example>" + | |||||||||
"<Amount>1000</Amount>" + | "<Max>1000</Max>" + | |||||||||
"<Initial>1000</Initial>" + | ||||||||||
elexis: removed | ||||||||||
"<Type>food.meat</Type>" + | "<Type>food.meat</Type>" + | |||||||||
"<KillBeforeGather>false</KillBeforeGather>" + | "<KillBeforeGather>false</KillBeforeGather>" + | |||||||||
"<MaxGatherers>25</MaxGatherers>" + | "<MaxGatherers>25</MaxGatherers>" + | |||||||||
"<DiminishingReturns>0.8</DiminishingReturns>" + | "<DiminishingReturns>0.8</DiminishingReturns>" + | |||||||||
"<Change>" + | ||||||||||
"<AnyName>" + | ||||||||||
"<Value>2</Value>" + | ||||||||||
"<Interval>1000</Interval>" + | ||||||||||
"</AnyName>" + | ||||||||||
"<Growth>" + | ||||||||||
"<State>alive</State>" + | ||||||||||
"<Value>2</Value>" + | ||||||||||
"<Interval>1000</Interval>" + | ||||||||||
"<UpperLimit>500</UpperLimit>" + | ||||||||||
"</Growth>" + | ||||||||||
"<Rotting>" + | ||||||||||
"<State>dead notGathered</State>" + | ||||||||||
Done Inline Actions
elexis: * Limit should be demonstrated (or must be demonstrated if it becomes non-optiona)
* I think… | ||||||||||
"<Value>-2</Value>" + | ||||||||||
"<Interval>1000</Interval>" + | ||||||||||
"</Rotting>" + | ||||||||||
"<Decay>" + | ||||||||||
"<State>dead</State>" + | ||||||||||
"<Value>-1</Value>" + | ||||||||||
"<Interval>1000</Interval>" + | ||||||||||
"<LowerLimit>500</LowerLimit>" + | ||||||||||
"</Decay>" + | ||||||||||
"</Change>" + | ||||||||||
"</a:example>" + | "</a:example>" + | |||||||||
"<element name='KillBeforeGather' a:help='Whether this entity must be killed (health reduced to 0) before its resources can be gathered'>" + | "<element name='KillBeforeGather' a:help='Whether this entity must be killed (health reduced to 0) before its resources can be gathered'>" + | |||||||||
"<data type='boolean'/>" + | "<data type='boolean'/>" + | |||||||||
"</element>" + | "</element>" + | |||||||||
"<element name='Amount' a:help='Amount of resources available from this entity'>" + | "<element name='Max' a:help='Max amount of resources available from this entity.'>" + | |||||||||
"<choice><data type='nonNegativeInteger'/><value>Infinity</value></choice>" + | "<choice><data type='nonNegativeInteger'/><value>Infinity</value></choice>" + | |||||||||
"</element>" + | "</element>" + | |||||||||
"<optional>" + | ||||||||||
"<element name='Initial' a:help='Initial amount of resources available from this entity, if this is not specified, Max is used.'>" + | ||||||||||
"<choice><data type='nonNegativeInteger'/><value>Infinity</value></choice>" + | ||||||||||
Done Inline Actionsdata type? Silier: data type? | ||||||||||
Done Inline ActionsApparently not. Stan: Apparently not. | ||||||||||
"</element>" + | ||||||||||
Done Inline ActionsBut thats the same as Limit, so there is no purpose for this field, not in this patch (nor in the random starting resources amount patch). elexis: But thats the same as Limit, so there is no purpose for this field, not in this patch (nor in… | ||||||||||
Done Inline ActionsNo it's not the same. Max Amount 5000 Bar displays x/5000 Max Amount 5000 Bar displays x/5000 Amount 4000 Bar displays x/4000 Stan: No it's not the same.
Max Amount 5000
Amount 4000
Limit 4500
Bar displays x/5000
Regen goes… | ||||||||||
Done Inline ActionsAnd why is it useful to implement something like your first example, elexis: And why is it useful to implement something like your first example,
i.e. what is the use case… | ||||||||||
Done Inline ActionsIt's not actually. You're right with the limit thing. Stan: It's not actually. You're right with the limit thing. | ||||||||||
"</optional>" + | ||||||||||
Done Inline Actions
elexis: * Amount is nonNegativeInteger, MaxAmount is nonNegativeDecimal
* MaxAmount still redundant… | ||||||||||
"<element name='Type' a:help='Type and Subtype of resource available from this entity'>" + | "<element name='Type' a:help='Type and Subtype of resource available from this entity'>" + | |||||||||
Resources.BuildChoicesSchema(true, true) + | Resources.BuildChoicesSchema(true, true) + | |||||||||
"</element>" + | "</element>" + | |||||||||
"<element name='MaxGatherers' a:help='Amount of gatherers who can gather resources from this entity at the same time'>" + | "<element name='MaxGatherers' a:help='Amount of gatherers who can gather resources from this entity at the same time'>" + | |||||||||
"<data type='nonNegativeInteger'/>" + | "<data type='nonNegativeInteger'/>" + | |||||||||
"</element>" + | "</element>" + | |||||||||
"<optional>" + | "<optional>" + | |||||||||
"<element name='DiminishingReturns' a:help='The relative rate of any new gatherer compared to the previous one (geometric sequence). Leave the element out for no diminishing returns.'>" + | "<element name='DiminishingReturns' a:help='The relative rate of any new gatherer compared to the previous one (geometric sequence). Leave the element out for no diminishing returns.'>" + | |||||||||
"<ref name='positiveDecimal'/>" + | "<ref name='positiveDecimal'/>" + | |||||||||
"</element>" + | "</element>" + | |||||||||
"</optional>" + | ||||||||||
"<optional>" + | ||||||||||
"<element name='Change' a:help='Optional element containing all the modifications that affects a resource supply'>" + | ||||||||||
"<oneOrMore>" + | ||||||||||
Done Inline ActionsUse case for specifying a Change without Changes? elexis: Use case for specifying a Change without Changes? | ||||||||||
Done Inline ActionsNope actually thanks. Stan: Nope actually thanks. | ||||||||||
"<element a:help='Element defining whether and how a resource supply regenerates or decays'>" + | ||||||||||
Done Inline Actionsdescription duplicated ^ elexis: description duplicated ^
"gathered" -> "gathered from" I believe
also that gather condition was… | ||||||||||
Done Inline Actions-Optional elexis: -Optional | ||||||||||
"<anyName/>" + | ||||||||||
Done Inline Actionsno space before /> elexis: no space before /> | ||||||||||
"<interleave>" + | ||||||||||
"<element name='Value' a:help='The amount of resource added per interval.'>" + | ||||||||||
Done Inline ActionsJust a "flag" would be better than a Boolean, no? lyv: Just a "flag" would be better than a Boolean, no? | ||||||||||
Done Inline ActionsAh yeah indeed, didn't think of it :D Stan: Ah yeah indeed, didn't think of it :D | ||||||||||
"<data type='integer'/>" + | ||||||||||
Done Inline Actionswrong description elexis: wrong description | ||||||||||
"</element>" + | ||||||||||
"<element name='Interval' a:help='The interval in milliseconds.'>" + | ||||||||||
"<data type='positiveInteger'/>" + | ||||||||||
"</element>" + | ||||||||||
Done Inline ActionsDo we actually need the Both option? Since the condition is optional already. elexis: Do we actually need the `Both` option? Since the condition is optional already. | ||||||||||
"<optional>" + | ||||||||||
"<element name='UpperLimit' a:help='The upper limit of the value after which the Change has no effect.'>" + | ||||||||||
"<data type='nonNegativeInteger'/>" + | ||||||||||
"</element>" + | ||||||||||
Done Inline Actionsper tick might be misleading. lyv: `per tick` might be misleading. | ||||||||||
Done Inline ActionsShould use the type the other schemas timer delay/intervals use, same for the other properties elexis: Should use the type the other schemas timer delay/intervals use, same for the other properties | ||||||||||
Done Inline ActionsIt does. Stan: It does. | ||||||||||
Done Inline ActionsDelay decimal, interval integer; yet they use the same value range (milliseconds used by timers) elexis: Delay decimal, interval integer; yet they use the same value range (milliseconds used by timers) | ||||||||||
"</optional>" + | ||||||||||
"<optional>" + | ||||||||||
"<element name='LowerLimit' a:help='The bottom limit of the value after which the Change has no effect.'>" + | ||||||||||
Done Inline ActionsSeems simpler to not have the Delay optional. The code will be one condition shorter and comparing / grepping files will be more information-complete too. elexis: Seems simpler to not have the Delay optional. The code will be one condition shorter and… | ||||||||||
"<data type='nonNegativeInteger'/>" + | ||||||||||
Done Inline ActionsnonNegativeInteger? lyv: `nonNegativeInteger`? | ||||||||||
Done Inline ActionsFair enough. :) Stan: Fair enough. :) | ||||||||||
"</element>" + | ||||||||||
"</optional>" + | ||||||||||
"<optional>" + | ||||||||||
"<element name='State' a:help='What state the entity must be in for the effect to occur.'>" + | ||||||||||
Done Inline ActionsI guess you want to exclude 0 elexis: I guess you want to exclude 0 | ||||||||||
"<list>" + | ||||||||||
"<oneOrMore>" + | ||||||||||
Done Inline Actions
elexis: * I guess you don't want to exclude zero here. :P
* "change has no effect" sounds like a… | ||||||||||
Done Inline ActionsNot it cannot because setAmount won't let it. Stan: Not it cannot because setAmount won't let it. | ||||||||||
Done Inline Actions(change -> Change in the description, name capitalization indicates to the reader that the sentence relates to the the branded version of change that we defined above, not to be confused with whatever connotation the reader thinks about when reading the colloquial change) elexis: (change -> Change in the description, name capitalization indicates to the reader that the… | ||||||||||
Done Inline ActionsRight. Stan: Right. | ||||||||||
"<choice>" + | ||||||||||
"<value>alive</value>" + | ||||||||||
"<value>dead</value>" + | ||||||||||
Done Inline Actionsthis is copypasta as well^ elexis: this is copypasta as well^ | ||||||||||
"<value>gathered</value>" + | ||||||||||
"<value>notGathered</value>" + | ||||||||||
"</choice>" + | ||||||||||
"</oneOrMore>" + | ||||||||||
"</list>" + | ||||||||||
"</element>" + | ||||||||||
"</optional>" + | ||||||||||
"</interleave>" + | ||||||||||
"</element>" + | ||||||||||
"</oneOrMore>" + | ||||||||||
"</element>" + | ||||||||||
"</optional>"; | "</optional>"; | |||||||||
ResourceSupply.prototype.Init = function() | ResourceSupply.prototype.Init = function() | |||||||||
{ | { | |||||||||
// Current resource amount (non-negative) | this.maxAmount = +this.template.Max; | |||||||||
this.amount = this.GetMaxAmount(); | this.amount = +(this.template.Initial || this.GetMaxAmount()); | |||||||||
Done Inline Actions// TODO: Should not serialize this.maxAmount and this.infinite elexis: // TODO: Should not serialize this.maxAmount and this.infinite
(A `refs #` to that… | ||||||||||
Done Inline ActionsSee the cleanup patch for resource supply. If it's committed first no need to add todos. Stan: See the cleanup patch for resource supply. If it's committed first no need to add todos. | ||||||||||
StanUnsubmitted Done Inline Actions
Stan: | ||||||||||
// Including the ones that are tasked but not here yet. | ||||||||||
Done Inline ActionsIf the template says 200 max and it starts with 400, it shouldn't regenerate to 400 after having been gathered to 300, right? If no maxAmount is given, why cap to the template amount, rather than no limit (JS Number max)? A ternary would be nice here (so that the second assignment doesn't overwrite the first assignment, but that there is only one assigment to the final value direclty.) elexis: If the template says 200 max and it starts with 400, it shouldn't regenerate to 400 after… | ||||||||||
Done Inline ActionsBecause in the gui the bar would get out of range. To check this, just set the value to NaN. Stan: Because in the gui the bar would get out of range. To check this, just set the value to NaN. | ||||||||||
Done Inline ActionsThat sound more like a bug in the status bars code rather than a design constraint of ResourceSupply. elexis: That sound more like a bug in the status bars code rather than a design constraint of… | ||||||||||
Done Inline ActionsOne could probably fix the status bar code. But I believe it's rational to assume that the max amount is always superior to the amount. If we really want to set a max regen level which I don't think we do even for units health, I could add a targetAmount tag in the regen blocks, that would prevent decay above and below that variable. Stan: One could probably fix the status bar code. But I believe it's rational to assume that the max… | ||||||||||
Done Inline ActionsThe maxAmount could be defined as the threshold where the effect becomes active or inactive. At least I think it could be used as freedom for modders to specify a template that could start with 600HP but only regenerates to 300HP? elexis: The maxAmount could be defined as the threshold where the effect becomes active or inactive. | ||||||||||
Done Inline ActionsThen the name would be incorrect. Also I'm pretty sure there is some other code doing some checks on that getter... But why not if that's really a wanted feature. Stan: Then the name would be incorrect.
Also I'm pretty sure there is some other code doing some… | ||||||||||
Done Inline ActionsIf someone says current resourcesupply=500 and maxAmount=300, he did not say maxAmount=500, but he said maxAmount=300. elexis: If someone says current resourcesupply=500 and maxAmount=300, he did not say maxAmount=500, but… | ||||||||||
Done Inline ActionsWhat about renaming it maxRegenAmount and making them independent. I will need maxAmount to make the random thing @Nescio is taking about. On that topic, can we do random in JS, or is it OOS material ? Stan: What about renaming it maxRegenAmount and making them independent. I will need maxAmount to… | ||||||||||
Done Inline ActionsThis still seems wrong to me. If someone said maxAmount = 400, then it should be 400. If someone said starting amount = X, then it should be X, regardless whether X is greater or smaller than 400. But it's still easier to define the maxAmount as the amount at which the regenerator stops, rather than the maxAmount that the resource supply can carry, thus not running into that problem in any shape or form. Also notice that maxAmount is entirely irrelevant in case there are no Changes. elexis: This still seems wrong to me. If someone said maxAmount = 400, then it should be 400. If… | ||||||||||
Done Inline ActionsOkay here is how it should work. By default maxAmount = templateAmount Reason for maxAmount presence in the root. The goal is then to use it as the max bound when I will make the random patch Stan: Okay here is how it should work.
By default maxAmount = templateAmount
If maxAmount >… | ||||||||||
Done Inline ActionsThe template should then specify the starting resources amount as either: The starting amount interval could be considered independent from the decay/growth effect thresholds as mentioned. I think sheep starting with random health is a bit weird and I'm not convinced that we want it for vanilla (which is why it's good to split it from the rest that we all can more quickly agree on). Also there's the case for min/max symmetry: Regardless of the examples, it's still true that the template are supposed to define the values of the entity component, so it would be contradict the given definition of the entity component if the entity component would chose a different value than the user specified; unless the definition is changed to reflect that (case for KISS). elexis: The template should then specify the starting resources amount as either:
<StartAmount>
or… | ||||||||||
Done Inline ActionsNotice that not contradicting the template also mean not introducing a variable that this revision serializes. elexis: Notice that not contradicting the template also mean not introducing a variable that this… | ||||||||||
Done Inline ActionsI did not invent units disappearing where there amount goes to 0 if you want me to do that I can add another value that sets the negative amount which Imho is bad. You could also not clear set amount which would make it only decay if touch by an entity. If you hate so much that max amount value I can replace it by a threshold that makes the unit not regenerate over or under if decaying. You lose the ability to have them grow which is nice IMHO. Random was for mines not units. Stan: I did not invent units disappearing where there amount goes to 0 if you want me to do that I… | ||||||||||
this.gatherers = []; | this.gatherers = []; | |||||||||
Done Inline Actionsthis.regenerateTimer elexis: this.regenerateTimer | ||||||||||
this.activeGatherers = []; | ||||||||||
Done Inline Actionsthis.template.Regeneration elexis: `this.template.Regeneration` | ||||||||||
Done Inline ActionsI thought that would error out without !! lyv: I thought that would error out without `!!` | ||||||||||
let [type, subtype] = this.template.Type.split('.'); | let [type, subtype] = this.template.Type.split('.'); | |||||||||
this.cachedType = { "generic": type, "specific": subtype }; | this.cachedType = { "generic": type, "specific": subtype }; | |||||||||
Done Inline ActionsDont' pass this.template.Change[key] to the function when that can just refer to this.template.Change[key] directly. elexis: Dont' pass `this.template.Change[key]` to the function when that can just refer to `this. | ||||||||||
if (this.template.Change) | ||||||||||
{ | ||||||||||
this.timers = {}; | ||||||||||
this.cachedChanges = {}; | ||||||||||
this.RecalculateValues(); | ||||||||||
Done Inline ActionsOnly the increasing changes can increase the max amount. can nuke outer braces. if (this.template.Change[changeKey]) (either undefined (falsy) or object (truthy)) elexis: Only the increasing changes can increase the max amount.
can nuke outer braces.
`if (this. | ||||||||||
Done Inline ActionsShould have maxAmount set before the timer is added, to ensure that nothing that this function could imaginably call reads from maxAmount before its set elexis: Should have maxAmount set before the timer is added, to ensure that nothing that this function… | ||||||||||
} | ||||||||||
}; | }; | |||||||||
Done Inline ActionsUnless you want to change and serialize this.growsWhenAliveOnly in the entity component, the variable is only adding indirection, memory allocation, and two lines of code? Same about the y = +x lines. That stuff is serialized, but do we want that? Which means either the serialization is useful because the it applies the valuechange functions that apply aura/tech effects, or the serialization is useless and should be removed. Also at last I don't recall whether we actually do need to serialize in order to keep tech/aura/entity cmp property stats in sync, or whether we can't just recompute them on init. Right now, ResourceSupply.js has no Serialize function replacement, which means all properties will be serialized and deserialized. this.infinite = !isFinite(+this.template.Amount); is another of these serialized properties that has no reason to exist by the looks of things, unless it should be tech/aura modifiable, and then it would miss the accoring calls. elexis: Unless you want to change and serialize `this.growsWhenAliveOnly` in the entity component, the… | ||||||||||
Done Inline Actions
Ah true, I was looking for another way to do it without adding two lines... But everything I tried save this didn't work so : if (this.template.Growth.GrowsWhenAliveOnly) this.growsWhenAliveOnly = true;
I was wondering about this, techs didn't make sense, but auras definitively do. I will add applymodifications like it's done in the health.js component. Not sure about the serialization, but it should certainly be taken into account. Stan: > if (this.template.Growth.GrowsWhenAliveOnly) is good enough?
Ah true, I was looking for… | ||||||||||
Done Inline Actions(One can also create a *.txt dump of the simstate and see what is stored visually.) elexis: (One can also create a *.txt dump of the simstate and see what is stored visually.) | ||||||||||
Done Inline ActionsHow would one do that ? Stan: How would one do that ? | ||||||||||
Done Inline ActionsEngine.DumpSimState() in the console would do. lyv: `Engine.DumpSimState()` in the console would do. | ||||||||||
ResourceSupply.prototype.IsInfinite = function() | ResourceSupply.prototype.IsInfinite = function() | |||||||||
{ | { | |||||||||
return !isFinite(+this.template.Amount); | return !isFinite(+this.template.Max); | |||||||||
}; | }; | |||||||||
ResourceSupply.prototype.GetKillBeforeGather = function() | ResourceSupply.prototype.GetKillBeforeGather = function() | |||||||||
{ | { | |||||||||
return this.template.KillBeforeGather == "true"; | return this.template.KillBeforeGather == "true"; | |||||||||
}; | }; | |||||||||
ResourceSupply.prototype.GetMaxAmount = function() | ResourceSupply.prototype.GetMaxAmount = function() | |||||||||
{ | { | |||||||||
return +this.template.Amount; | return this.maxAmount; | |||||||||
Done Inline Actions(this duplication would be removed if the types were merged) elexis: (this duplication would be removed if the types were merged) | ||||||||||
Done Inline ActionsSee comment below. Stan: See comment below. | ||||||||||
}; | }; | |||||||||
ResourceSupply.prototype.GetCurrentAmount = function() | ResourceSupply.prototype.GetCurrentAmount = function() | |||||||||
{ | { | |||||||||
return this.amount; | return this.amount; | |||||||||
}; | }; | |||||||||
ResourceSupply.prototype.GetMaxGatherers = function() | ResourceSupply.prototype.GetMaxGatherers = function() | |||||||||
{ | { | |||||||||
return +this.template.MaxGatherers; | return +this.template.MaxGatherers; | |||||||||
}; | }; | |||||||||
ResourceSupply.prototype.GetNumGatherers = function() | ResourceSupply.prototype.GetNumGatherers = function() | |||||||||
{ | { | |||||||||
return this.gatherers.length; | return this.gatherers.length; | |||||||||
}; | }; | |||||||||
/** | /** | |||||||||
* @return {number} - The number of currently active gatherers. | ||||||||||
Done Inline Actionsint → number Stan: int → number | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.GetNumActiveGatherers = function() | ||||||||||
{ | ||||||||||
return this.activeGatherers.length; | ||||||||||
}; | ||||||||||
/** | ||||||||||
* @return {{ "generic": string, "specific": string }} An object containing the subtype and the generic type. All resources must have both. | * @return {{ "generic": string, "specific": string }} An object containing the subtype and the generic type. All resources must have both. | |||||||||
*/ | */ | |||||||||
ResourceSupply.prototype.GetType = function() | ResourceSupply.prototype.GetType = function() | |||||||||
{ | { | |||||||||
return this.cachedType; | return this.cachedType; | |||||||||
}; | }; | |||||||||
/** | /** | |||||||||
Show All 20 Lines | ||||||||||
ResourceSupply.prototype.IsGatheringUs = function(entity) | ResourceSupply.prototype.IsGatheringUs = function(entity) | |||||||||
{ | { | |||||||||
return this.gatherers.indexOf(entity) !== -1; | return this.gatherers.indexOf(entity) !== -1; | |||||||||
}; | }; | |||||||||
/** | /** | |||||||||
* Each additional gatherer decreases the rate following a geometric sequence, with diminishingReturns as ratio. | * Each additional gatherer decreases the rate following a geometric sequence, with diminishingReturns as ratio. | |||||||||
* @return {number} The diminishing return if any, null otherwise. | * @return {number} The diminishing return if any, null otherwise. | |||||||||
*/ | */ | |||||||||
Done Inline Actions-tab elexis: -tab | ||||||||||
ResourceSupply.prototype.GetDiminishingReturns = function() | ResourceSupply.prototype.GetDiminishingReturns = function() | |||||||||
{ | { | |||||||||
if (!this.template.DiminishingReturns) | if (!this.template.DiminishingReturns) | |||||||||
Done Inline ActionsWhich number types does JS provide? elexis: Which number types does JS provide? | ||||||||||
Done Inline Actions"We all float down here" ~ scary clown lyv: "We all float down here" ~ scary clown | ||||||||||
Done Inline ActionsThat fixes "A trailing decimal point can be confused with a dot: '1.'." could also remove the .0 if you think that's better. Stan: That fixes "A trailing decimal point can be confused with a dot: '1.'." could also remove the . | ||||||||||
return null; | return null; | |||||||||
let diminishingReturns = ApplyValueModificationsToEntity("ResourceSupply/DiminishingReturns", +this.template.DiminishingReturns, this.entity); | let diminishingReturns = ApplyValueModificationsToEntity("ResourceSupply/DiminishingReturns", +this.template.DiminishingReturns, this.entity); | |||||||||
if (!diminishingReturns) | if (!diminishingReturns) | |||||||||
return null; | return null; | |||||||||
let numGatherers = this.GetNumGatherers(); | let numGatherers = this.GetNumGatherers(); | |||||||||
if (numGatherers > 1) | if (numGatherers > 1) | |||||||||
return diminishingReturns == 1 ? 1 : (1 - Math.pow(diminishingReturns, numGatherers)) / (1 - diminishingReturns) / numGatherers; | return diminishingReturns == 1 ? 1 : (1 - Math.pow(diminishingReturns, numGatherers)) / (1 - diminishingReturns) / numGatherers; | |||||||||
return null; | return null; | |||||||||
}; | }; | |||||||||
/** | /** | |||||||||
* @param {number} amount The amount of resources that should be taken from the resource supply. The amount must be positive. | * @param {number} amount The amount of resources that should be taken from the resource supply. The amount must be positive. | |||||||||
* @return {{ "amount": number, "exhausted": boolean }} The current resource amount in the entity and whether it's exhausted or not. | * @return {{ "amount": number, "exhausted": boolean }} The current resource amount in the entity and whether it's exhausted or not. | |||||||||
Done Inline Actionsmissing 0, ? lyv: missing `0, `? | ||||||||||
Done Inline ActionsRight. Stan: Right. | ||||||||||
Done Inline Actionslet elexis: let | ||||||||||
*/ | */ | |||||||||
ResourceSupply.prototype.TakeResources = function(amount) | ResourceSupply.prototype.TakeResources = function(amount) | |||||||||
Done Inline ActionsAbove mentioned duplicate limit check. Could pass min/max to this function or rely on the caller having set the limit already and setting here whatever is given. In case the latter is chosen, TakeResources would obviously have to use max(x - y, 0). elexis: Above mentioned duplicate limit check. Could pass min/max to this function or rely on the… | ||||||||||
Done Inline ActionsI don't understand. It's capped so it doesn't break the UI and it's destroyed when it reaches 0. Every code setting the amount should go through set amount. Also we pass the old amount not the new one. Stan: I don't understand. It's capped so it doesn't break the UI and it's destroyed when it reaches 0. | ||||||||||
{ | { | |||||||||
if (this.IsInfinite()) | ||||||||||
return { "amount": amount, "exhausted": false }; | ||||||||||
let oldAmount = this.GetCurrentAmount(); | ||||||||||
this.Change(-amount); | ||||||||||
return { | ||||||||||
"amount": oldAmount - this.GetCurrentAmount(), | ||||||||||
"exhausted": this.amount == 0 | ||||||||||
}; | ||||||||||
}; | ||||||||||
/** | ||||||||||
* Changes the amount of resources; posts messages when necessary and checks timers. | ||||||||||
* | ||||||||||
* @param {number} change - The amount to change the resources with (can be negative). | ||||||||||
* @return {number} - The change in resourceSupply. | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.Change = function(change) | ||||||||||
{ | ||||||||||
// Before changing the amount, activate Fogging if necessary to hide changes | // Before changing the amount, activate Fogging if necessary to hide changes | |||||||||
let cmpFogging = Engine.QueryInterface(this.entity, IID_Fogging); | let cmpFogging = Engine.QueryInterface(this.entity, IID_Fogging); | |||||||||
if (cmpFogging) | if (cmpFogging) | |||||||||
cmpFogging.Activate(); | cmpFogging.Activate(); | |||||||||
if (this.IsInfinite()) | let oldAmount = this.amount; | |||||||||
return { "amount": amount, "exhausted": false }; | this.amount = Math.min(Math.max(oldAmount + change, 0), this.GetMaxAmount()); | |||||||||
let oldAmount = this.GetCurrentAmount(); | ||||||||||
this.amount = Math.max(0, oldAmount - amount); | ||||||||||
let isExhausted = this.GetCurrentAmount() == 0; | // Remove entities that have been exhausted. | |||||||||
// Remove entities that have been exhausted | if (this.amount == 0) | |||||||||
if (isExhausted) | ||||||||||
Engine.DestroyEntity(this.entity); | Engine.DestroyEntity(this.entity); | |||||||||
Engine.PostMessage(this.entity, MT_ResourceSupplyChanged, { "from": oldAmount, "to": this.GetCurrentAmount() }); | // Do not send a message if the resource amount didn't change. | |||||||||
Done Inline ActionsStill sounds wrong to subtract a rate (which ought to be amount/time) from amount. Guess it could be acknowledged and postponed as its already present. elexis: Still sounds wrong to subtract a rate (which ought to be amount/time) from amount. Guess it… | ||||||||||
Done Inline ActionsIs it the variable name that's wrong ? Could be renamed to amount for all I care. Stan: Is it the variable name that's wrong ? Could be renamed to amount for all I care. | ||||||||||
let actualChange = this.amount - oldAmount; | ||||||||||
if (actualChange != 0) | ||||||||||
Engine.PostMessage(this.entity, MT_ResourceSupplyChanged, { | ||||||||||
"from": oldAmount, | ||||||||||
"to": this.amount | ||||||||||
}); | ||||||||||
this.CheckTimers(); | ||||||||||
return actualChange; | ||||||||||
}; | ||||||||||
Done Inline ActionsThis function returns exhausedso that something else can check to remove the resource if it's exhaused? elexis: This function returns `exhaused`so that something else can check to remove the resource if it's… | ||||||||||
Done Inline ActionsSomething else is whatever is calling it so IIRC Petra and UnitAI which means that in any case once the number has reached 0 the resource is exhausted. Stan: Something else is whatever is calling it so IIRC Petra and UnitAI which means that in any case… | ||||||||||
return { "amount": oldAmount - this.GetCurrentAmount(), "exhausted": isExhausted }; | /** | |||||||||
* Sets the amount of resources to the new value. | ||||||||||
* | ||||||||||
* @param {number} newValue - The value to set. | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.SetAmount = function(newValue) | ||||||||||
{ | ||||||||||
this.Change(newValue - this.amount); | ||||||||||
}; | }; | |||||||||
/** | /** | |||||||||
* @param {number} gathererID - The gatherer to add. | * @param {number} gathererID - The gatherer to add. | |||||||||
* @return {boolean} - Whether the gatherer was successfully added to the entity's gatherers list | * @return {boolean} - Whether the gatherer was successfully added to the entity's gatherers list | |||||||||
* or the entity was already gathering us. | * or the entity was already gathering us. | |||||||||
*/ | */ | |||||||||
ResourceSupply.prototype.AddGatherer = function(gathererID) | ResourceSupply.prototype.AddGatherer = function(gathererID) | |||||||||
{ | { | |||||||||
if (!this.IsAvailable()) | if (!this.IsAvailable()) | |||||||||
return false; | return false; | |||||||||
if (this.IsGatheringUs(gathererID)) | if (this.IsGatheringUs(gathererID)) | |||||||||
return true; | return true; | |||||||||
this.gatherers.push(gathererID); | this.gatherers.push(gathererID); | |||||||||
Engine.PostMessage(this.entity, MT_ResourceSupplyNumGatherersChanged, { "to": this.GetNumGatherers() }); | Engine.PostMessage(this.entity, MT_ResourceSupplyNumGatherersChanged, { "to": this.GetNumGatherers() }); | |||||||||
return true; | return true; | |||||||||
}; | }; | |||||||||
Done Inline ActionsCancelTimer doesn't have a return value (which makes it undefined, which then makes it work, but that doesn't seem intentional. Perhaps nicer to delete this.regenerateTimer manually) elexis: CancelTimer doesn't have a return value (which makes it undefined, which then makes it work… | ||||||||||
Done Inline ActionsSo: if (this.regenerateTimer) { Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer).CancelTimer(this.regenerateTimer); this.regenerateTimer = null; } Stan: So:
```
if (this.regenerateTimer)
{
Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer). | ||||||||||
Done Inline Actionsor undefined (null has a use case if we need to distinguish it from undefined) elexis: or `undefined` (`null` has a use case if we need to distinguish it from `undefined`) | ||||||||||
/** | /** | |||||||||
* Declares this gatherer to be actively gathering. I.e. in the UnitAI-state of gathering. | ||||||||||
Done Inline Actions(Does unit refer to the entity wiht resourcesupply? There exist entities with resource supplies that aren't units (trees), am I mistaken?) elexis: (Does unit refer to the entity wiht resourcesupply? There exist entities with resource supplies… | ||||||||||
Done Inline ActionsYes it refers to the entity. Stan: Yes it refers to the entity. | ||||||||||
Done Inline ActionsIt does refer to the entity Stan: It does refer to the entity | ||||||||||
Done Inline ActionsI think unit refers to the gatherer in this particular case. lyv: I think `unit` refers to the gatherer in this particular case. | ||||||||||
Done Inline Actionshere one could loop over the this.timers, or this.generationTimers or whatever it will be called, avoiding the continue condition elexis: here one could loop over the `this.timers`, or `this.generationTimers` or whatever it will be… | ||||||||||
Done Inline ActionsThe best thing I can do is a map, because I need to keep track of the name of the timers, in order to not add them twice. Stan: The best thing I can do is a map, because I need to keep track of the name of the timers, in… | ||||||||||
Done Inline ActionsSounds like a use case for Sets. lyv: Sounds like a use case for [[ https://developer.mozilla.org/en… | ||||||||||
Done Inline ActionsNot sure you can use sets for objects. Actually I was more thinking of a dictionnary http://pietschsoft.com/post/2015/09/05/JavaScript-Basics-How-to-create-a-Dictionary-with-KeyValue-pairs Stan: Not sure you can use sets for objects. Actually I was more thinking of a dictionnary http… | ||||||||||
Done Inline Actions
lyv: > The Set object lets you store unique values of any type, whether primitive values or object… | ||||||||||
Done Inline ActionsActually using a set wouldn't work because I'm checking if object properties exist, not if two objects are equal. Stan: Actually using a set wouldn't work because I'm checking if object properties exist, not if two… | ||||||||||
Done Inline ActionsA regular JS object { "key1": value1, ... } works well as a dictionary. Set and Map are nice, but they can also have downsides, because they can't be serialized using JSON (which is not relevant here but in some cases) for example, and array functions don't work for Sets without conversion. Sets only have the advantage that one doesn't have to check if an item is already in the array when wanting to add one I think. Anyhow, if you're going with hardcoded TimerA TimerB, then you can use this.TimerA, this.TimerB, otherwise an array should work well as a dictionary where the keys are numbers. (Or a Map if there is any real reason to.) Also the properties can only be set if template.RegenBonuses node exists. A property which is never set (and not set to undefined) won't be serialized. elexis: A regular JS object `{ "key1": value1, ... }` works well as a dictionary. Set and Map are nice… | ||||||||||
Done Inline ActionsExcept you can't check that a given instance of a time was set without a dictionary. If I had ten times the same timer there will be 10 cells in my array and I won't be able to see the difference. If it's a dictionary though it will be easy. Stan: Except you can't check that a given instance of a time was set without a dictionary. If I had… | ||||||||||
Done Inline ActionsYou can check whether the timer was set regardless whether you use an array, object or Map as the 'dictionary', i.e. timers[i] would always refer to the i'th item in the template. If you use the current aproach (anyName), then identifying by the custom name would be easier than to identify by the numeric index, i.e. a regular JS object this.timers = {} (that is not set when there are no items) seems the easiest solution. I'd advise to use a container that is an item of this, rather than using this as the container, so that it is impossible to have name collisions (for example if the prototype has an AddTimer function and the template uses the Add name, then this[key + "Timer"] overwrites/returns the function instead of the timer. elexis: You can check whether the timer was set regardless whether you use an array, object or Map as… | ||||||||||
* | ||||||||||
Done Inline Actionsplayer => (also https://en.wikipedia.org/wiki/Prenex_normal_form is slightly more forward to read (i.e. some(!x) instead of !every(x))) elexis: player =>
(also https://en.wikipedia.org/wiki/Prenex_normal_form is slightly more forward to… | ||||||||||
* @param {number} player - The playerID owning the gatherer. | ||||||||||
* @param {number} entity - The entityID gathering. | ||||||||||
Done Inline Actions(didn't check if any logics make sense of the patch, only linting so far. But here it sounds like it could use a return if the code is correct) elexis: (didn't check if any logics make sense of the patch, only linting so far. But here it sounds… | ||||||||||
* | ||||||||||
* @return {boolean} - Whether the gatherer was successfully added to the gatherers list. | ||||||||||
*/ | ||||||||||
Done Inline Actions(this duplication would be removed too) elexis: (this duplication would be removed too) | ||||||||||
Done Inline ActionsSee comment below Stan: See comment below | ||||||||||
Done Inline Actionsif ( hasGatherers can be inlined elexis: if (
hasGatherers can be inlined | ||||||||||
ResourceSupply.prototype.AddActiveGatherer = function(entity) | ||||||||||
{ | ||||||||||
Done Inline ActionsWhy does an added gatherer stop timers? elexis: Why does an added gatherer stop timers? | ||||||||||
Done Inline ActionsSee the name of this ticket. The idea is that when gathering the object stops decaying. Stan: See the name of this ticket. The idea is that when gathering the object stops decaying. | ||||||||||
Done Inline ActionsDoesn't seem realistic to me, food is rotting even faster if one smears ones hands bacteria on it, and the lost resources for gatherers seems not measurable but not significant enough to justify doing such unrealistic things? Otherwise if this feature proposal is the right choice, one can also consider starting a timer until the decay should start. elexis: Doesn't seem realistic to me, food is rotting even faster if one smears ones hands bacteria on… | ||||||||||
Done Inline ActionsThat's fair. Stan: That's fair. | ||||||||||
Done Inline Actionsinstead of key, it should use the same name as found in the template (so it's easier for the reader to figure out what content key contains). For example changeName or changeKey in the current revision of the patch where the effects are called "change". elexis: instead of `key`, it should use the same name as found in the template (so it's easier for the… | ||||||||||
if (!this.AddGatherer(entity)) | ||||||||||
Done Inline ActionsThis duplication would be removed if unifying the two types. I guess there are up to 30 gatherers and most of the time when there is Decay/Growth enabled, the timer will already be running, elexis: This duplication would be removed if unifying the two types.
I guess there are up to 30… | ||||||||||
Done Inline ActionsAh damnit that's a bad copy paste, the above should be growth timer, and the below decaytimer. Stan: Ah damnit that's a bad copy paste, the above should be growth timer, and the below decaytimer. | ||||||||||
Done Inline Actionsmerge the two if-statements with ||, also do the quickest, most often false check first elexis: merge the two if-statements with ||, also do the quickest, most often false check first | ||||||||||
return false; | ||||||||||
Done Inline ActionsIt seems correct to call this only on Init, since either the corpse was just created and the timer is active from then on, or the alive entity created and the timer is active from then on. But then we don't need the !timers check then, because it's always true at that point. elexis: It seems correct to call this only on Init, since either the corpse was just created and the… | ||||||||||
Done Inline ActionsWell since I added the interval modification it's not always true now. Stan: Well since I added the interval modification it's not always true now. | ||||||||||
Done Inline ActionsUse the globally consistent name. Here it uses AddRegenTimer and this.regenTimers, but Change is used elsewhere. Perhaps just this.timers and this.AddTimer`. elexis: Use the globally consistent name. Here it uses AddRegenTimer and this.regenTimers, but Change… | ||||||||||
if (this.activeGatherers.indexOf(entity) == -1) | ||||||||||
this.activeGatherers.push(entity); | ||||||||||
this.CheckTimers(); | ||||||||||
Done Inline ActionschangeWithName still unneeded variable, just pass key and have the callee get the information from this.template.Change elexis: `changeWithName` still unneeded variable, just pass `key` and have the callee get the… | ||||||||||
return true; | ||||||||||
}; | ||||||||||
Done Inline Actions(can pass the key instead of changeWithName here I guess) elexis: (can pass the key instead of changeWithName here I guess) | ||||||||||
Done Inline ActionsAFAIK I can only pass one object. Stan: AFAIK I can only pass one object. | ||||||||||
Done Inline Actionsgood that change argument was removed, but these calls need to be replaced with this.template[key] something elexis: good that change argument was removed, but these calls need to be replaced with this.template… | ||||||||||
/** | ||||||||||
Done Inline Actionsno space before comma elexis: no space before comma | ||||||||||
* @param {number} gathererID - The gatherer's entity id. | * @param {number} gathererID - The gatherer's entity id. | |||||||||
* @todo: Should this return false if the gatherer didn't gather from said resource? | * @todo: Should this return false if the gatherer didn't gather from said resource? | |||||||||
*/ | */ | |||||||||
ResourceSupply.prototype.RemoveGatherer = function(gathererID) | ResourceSupply.prototype.RemoveGatherer = function(gathererID) | |||||||||
{ | { | |||||||||
let index = this.gatherers.indexOf(gathererID); | let index = this.gatherers.indexOf(gathererID); | |||||||||
if (index != -1) | ||||||||||
Done Inline ActionsApplyChange? (if going for "Change") elexis: ApplyChange? (if going for "Change") | ||||||||||
Done Inline ActionsFair enough. Stan: Fair enough. | ||||||||||
{ | ||||||||||
this.gatherers.splice(index, 1); | ||||||||||
Done Inline Actions+ 4 spaces (= length of if (). The idea is that the lines have the same amount of tabs and that after that spaces are used to align the conditions. Like if (bonus.Constraint && (bonus.Constraint == "Alive" && !cmpHealth || (Still arbitrary and tabs for object properties weren't affected by that CC so far, whatever). Also (x && y || !x && z) == x ? y : z if I didn't make an obvious mistake elexis: + 4 spaces (= length of `if (`). The idea is that the lines have the same amount of tabs and… | ||||||||||
Done Inline ActionsSucks to mix tabs and spaces, but okay. Stan: Sucks to mix tabs and spaces, but okay.
I'm not a fan of adding a ternary there. | ||||||||||
Engine.PostMessage(this.entity, MT_ResourceSupplyNumGatherersChanged, { "to": this.GetNumGatherers() }); | ||||||||||
} | ||||||||||
index = this.activeGatherers.indexOf(gathererID); | ||||||||||
if (index == -1) | if (index == -1) | |||||||||
return; | return; | |||||||||
this.activeGatherers.splice(index, 1); | ||||||||||
this.CheckTimers(); | ||||||||||
Done Inline Actionsthis.amount to save a function call. below too. elexis: this.amount to save a function call. below too.
Same for `IsInfinite`. Don't need to call the… | ||||||||||
Done Inline ActionsWas merely using the function to prevent a big case of refactoring, was that function subject to changes in the future. Stan: Was merely using the function to prevent a big case of refactoring, was that function subject… | ||||||||||
Done Inline ActionsA bit hypothetical. Wouldn't be a lot of refactoring, just changing this.infinite to this.IsInfinite(). I'd suggest to use the faster function unless there are actual plans to remove the property, and in that case, one could just remove the property. One would have to check what's worse (doing the conversion as often or allocating the variable for that many entities) elexis: A bit hypothetical. Wouldn't be a lot of refactoring, just changing `this.infinite` to `this. | ||||||||||
Done Inline ActionsWell I'm removing that serialized variable in D1771 Stan: Well I'm removing that serialized variable in D1771 | ||||||||||
Done Inline ActionsWell, not serializing a property (and recreating it from the template and modifications upon deserialization and init) doesn't mean that one can't set and use this.infinite. As far as I know, getters (that only return a variable) are used in simulation component to be called from another component or helper script. (Or perhaps it's a hidden modding feature that a new modfile could overwrite the internally used IsInfinite getter without changing this.infinite? Seems such a mod would have to adapt/replace the other prototype functions too.) elexis: Well, not serializing a property (and recreating it from the template and modifications upon… | ||||||||||
Done Inline ActionsOkay I'll remove it. Stan: Okay I'll remove it. | ||||||||||
}; | ||||||||||
Done Inline Actionselexis: https://developer.mozilla.org/en-US/docs/Glossary/Falsy | ||||||||||
Done Inline ActionsThanks. Stan: Thanks. | ||||||||||
this.gatherers.splice(index, 1); | /** | |||||||||
Engine.PostMessage(this.entity, MT_ResourceSupplyNumGatherersChanged, { "to": this.GetNumGatherers() }); | * Checks whether a timer ought to be added or removed. | |||||||||
*/ | ||||||||||
Done Inline Actionsstring? elexis: string? | ||||||||||
Done Inline ActionsAh yes you are right. Stan: Ah yes you are right. | ||||||||||
ResourceSupply.prototype.CheckTimers = function() | ||||||||||
Done Inline Actions
elexis: * limit undefined
* `||\n`
* `-` looks wrong
* Early return seems wrong, what if the new amount… | ||||||||||
Done Inline ActionsIf limit is optional and this code is used, it can reach numbers smaller than 0 and arbitrary great numbers elexis: If limit is optional and this code is used, it can reach numbers smaller than 0 and arbitrary… | ||||||||||
Done Inline ActionsNo because it's capped by the setAmount function. Stan: No because it's capped by the setAmount function. | ||||||||||
{ | ||||||||||
if (!this.timers || this.IsInfinite()) | ||||||||||
Done Inline Actions(early return, cmpTimer unneeded if it's triggered) elexis: (early return, cmpTimer unneeded if it's triggered) | ||||||||||
return; | ||||||||||
for (let changeKey in this.template.Change) | ||||||||||
Done Inline Actionsnot BuildingAI elexis: not BuildingAI | ||||||||||
{ | ||||||||||
Done Inline Actionscheck for length? Stan: check for length? | ||||||||||
Done Inline ActionsNo, because it could be possible that all timers were stopped and deleted. Freagarach: No, because it could be possible that all timers were stopped and deleted. | ||||||||||
if (!this.CheckState(changeKey)) | ||||||||||
{ | ||||||||||
Done Inline Actionsif (this.regenTimers) is simpler, and harder to break. There are two cases, one when the growing alive entity is destroyed, and one when the decaying corpse is destroyed. I guess they're both covered and covered correctly with this. elexis: if (this.regenTimers) is simpler, and harder to break.
There are two cases, one when the… | ||||||||||
this.StopTimer(changeKey); | ||||||||||
continue; | ||||||||||
} | ||||||||||
let template = this.template.Change[changeKey]; | ||||||||||
if (this.amount < +(template.LowerLimit || -1) || | ||||||||||
this.amount > +(template.UpperLimit || this.GetMaxAmount())) | ||||||||||
{ | ||||||||||
this.StopTimer(changeKey); | ||||||||||
continue; | ||||||||||
} | ||||||||||
Done Inline ActionsIn the other code it checks for this.template.Change && !this.infinite elexis: In the other code it checks for `this.template.Change && !this.infinite` | ||||||||||
if (this.cachedChanges[changeKey] == 0) | ||||||||||
{ | ||||||||||
this.StopTimer(changeKey); | ||||||||||
continue; | ||||||||||
} | ||||||||||
// We need a timer, add one if not present yet. | ||||||||||
// If there is a change of 0, the timer will stop after the first run. | ||||||||||
if (!this.timers[changeKey]) | ||||||||||
this.AddTimer(changeKey); | ||||||||||
Done Inline Actionscontains? That doesn't exist, at least in ES6, does it? Substring tests are wrong, for example ResourceSupply/Change/MyIntervalName/Rate. (Guess one needs to compute the key first then and ensure there are 3 slashes. This would also treat ResourceSupply/Change/MyName/Malformed/Interval correctly) elexis: contains? That doesn't exist, at least in ES6, does it?
Substring tests are wrong, for example… | ||||||||||
} | ||||||||||
}; | ||||||||||
/** | ||||||||||
* This verifies whether the current state of the supply matches the ones needed | ||||||||||
* for the specific timer to run. | ||||||||||
* | ||||||||||
* @param {string} changeKey - The name of the Change to verify the state for. | ||||||||||
* @return {boolean} - Whether the timer may run. | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.CheckState = function(changeKey) | ||||||||||
{ | ||||||||||
let template = this.template.Change[changeKey]; | ||||||||||
if (!template.State) | ||||||||||
Done Inline ActionsThat check can be more strict, elements[0], elements[1] or so are expected to have a specific value too, no? elexis: That check can be more strict, elements[0], elements[1] or so are expected to have a specific… | ||||||||||
return true; | ||||||||||
let states = template.State; | ||||||||||
let cmpHealth = Engine.QueryInterface(this.entity, IID_Health); | ||||||||||
if (states.indexOf("alive") != -1 && !cmpHealth && states.indexOf("dead") == -1 || | ||||||||||
states.indexOf("dead") != -1 && cmpHealth && states.indexOf("alive") == -1) | ||||||||||
return false; | ||||||||||
Done Inline ActionsDelay thing I mentioned, I didn't investigate how often in other code timers are restarted if a local aura appeared, so maybe updating the interval only if the current interval ran out isn't better. Recap: Imagine an aura entity running in and out of aura range of the corpses and the corpses restart their interval each time that happens. If the timer is restarted more often than the interval, then the corpses won't decay anymore at all, even if the aura would only reduce it by < 100%. elexis: Delay thing I mentioned, I didn't investigate how often in other code timers are restarted if a… | ||||||||||
Done Inline ActionsThe restarting-timer problem didn't really go away. elexis: The restarting-timer problem didn't really go away. | ||||||||||
Done Inline ActionsShould we use the regexp \s+ here ? Stan: Should we use the regexp \s+ here ? | ||||||||||
Done Inline ActionsI don't know? Freagarach: I don't know?
It just uses `indexOf` in `BuildRestrictions.js`, so perhaps that can also be… | ||||||||||
let activeGatherers = this.GetNumActiveGatherers(); | ||||||||||
if (states.indexOf("gathered") != -1 && activeGatherers == 0 && states.indexOf("notGathered") == -1 || | ||||||||||
states.indexOf("notGathered") != -1 && activeGatherers > 0 && states.indexOf("gathered") == -1) | ||||||||||
return false; | ||||||||||
return true; | ||||||||||
}; | ||||||||||
/** | ||||||||||
* @param {string} changeKey - The name of the Change to apply to the entity. | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.AddTimer = function(changeKey) | ||||||||||
{ | ||||||||||
if (this.timers[changeKey]) | ||||||||||
return; | ||||||||||
let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer); | ||||||||||
let interval = ApplyValueModificationsToEntity("ResourceSupply/Change/" + changeKey + "/Interval", +(this.template.Change[changeKey].Interval || 1000), this.entity); | ||||||||||
this.timers[changeKey] = cmpTimer.SetInterval(this.entity, IID_ResourceSupply, "TimerTick", interval, interval, changeKey); | ||||||||||
}; | ||||||||||
/** | ||||||||||
* @param {string} changeKey - The name of the change to stop the timer for. | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.StopTimer = function(changeKey) | ||||||||||
{ | ||||||||||
if (!this.timers[changeKey]) | ||||||||||
return; | ||||||||||
let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer); | ||||||||||
cmpTimer.CancelTimer(this.timers[changeKey]); | ||||||||||
delete this.timers[changeKey]; | ||||||||||
}; | ||||||||||
/** | ||||||||||
* @param {string} changeKey - The name of the change to apply to the entity. | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.TimerTick = function(changeKey) | ||||||||||
{ | ||||||||||
let template = this.template.Change[changeKey]; | ||||||||||
if (!template) | ||||||||||
{ | ||||||||||
this.StopTimer(changeKey); | ||||||||||
return; | ||||||||||
} | ||||||||||
if (this.Change(this.cachedChanges[changeKey]) == 0) | ||||||||||
this.StopTimer(changeKey); | ||||||||||
}; | ||||||||||
/** | ||||||||||
* Since the supposed changes can be affected by modifications, and applying those | ||||||||||
* are slow, do not calculate them every timer tick. | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.RecalculateValues = function() | ||||||||||
{ | ||||||||||
this.maxAmount = ApplyValueModificationsToEntity("ResourceSupply/Max", +this.template.Max, this.entity); | ||||||||||
if (!this.timers || this.IsInfinite()) | ||||||||||
return; | ||||||||||
for (let changeKey in this.template.Change) | ||||||||||
this.cachedChanges[changeKey] = ApplyValueModificationsToEntity("ResourceSupply/Change/" + changeKey + "/Value", +this.template.Change[changeKey].Value, this.entity); | ||||||||||
this.CheckTimers(); | ||||||||||
}; | ||||||||||
Done Inline Actionscheck for length? Stan: check for length? | ||||||||||
/** | ||||||||||
* @param {{ "component": string, "valueNames": string[] }} msg - Message containing a list of values that were changed. | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.OnValueModification = function(msg) | ||||||||||
{ | ||||||||||
if (msg.component != "ResourceSupply") | ||||||||||
return; | ||||||||||
if (!this.timers || this.IsInfinite()) | ||||||||||
return; | ||||||||||
this.RecalculateValues(); | ||||||||||
}; | ||||||||||
/** | ||||||||||
* @param {{ "from": number, "to": number }} msg - Message containing the old new owner. | ||||||||||
Done Inline Actionscheck for length? Stan: check for length? | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.OnOwnershipChanged = function(msg) | ||||||||||
{ | ||||||||||
if (msg.to == INVALID_PLAYER) | ||||||||||
{ | ||||||||||
let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer); | ||||||||||
for (let changeKey in this.timers) | ||||||||||
Done Inline Actionsint → number Stan: int → number | ||||||||||
cmpTimer.CancelTimer(this.timers[changeKey]); | ||||||||||
} | ||||||||||
if (!this.timers || this.IsInfinite()) | ||||||||||
return; | ||||||||||
this.RecalculateValues(); | ||||||||||
}; | ||||||||||
/** | ||||||||||
* @param {{ "entity": number, "newentity": number }} msg - Message to what the entity has been renamed. | ||||||||||
Done Inline Actionscheck for length? Stan: check for length? | ||||||||||
*/ | ||||||||||
ResourceSupply.prototype.OnEntityRenamed = function(msg) | ||||||||||
{ | ||||||||||
let cmpResourceSupplyNew = Engine.QueryInterface(msg.newentity, IID_ResourceSupply); | ||||||||||
if (cmpResourceSupplyNew) | ||||||||||
cmpResourceSupplyNew.SetAmount(this.GetCurrentAmount()); | ||||||||||
}; | }; | |||||||||
Done Inline ActionsIt's still unneeded duplication. elexis: It's still unneeded duplication. | ||||||||||
Done Inline ActionsNo more duplication. Stan: No more duplication. | ||||||||||
Done Inline Actionsint → number Stan: int → number | ||||||||||
Engine.RegisterComponentType(IID_ResourceSupply, "ResourceSupply", ResourceSupply); | Engine.RegisterComponentType(IID_ResourceSupply, "ResourceSupply", ResourceSupply); | |||||||||
Done Inline ActionsWhy tie this to the animation? lyv: Why tie this to the animation?
Makes it more unrealistic IMO. | ||||||||||
Done Inline ActionsWell don't get fat when you run do you ? :) Stan: Well don't get fat when you run do you ? :)
UnitAI set this to variants. But it could only… | ||||||||||
Done Inline Actionsif ( elexis: if (
`===` only when the type distinction actually matters | ||||||||||
Done Inline ActionsNoted Stan: Noted | ||||||||||
Done Inline ActionsSince when do we check by the animation whether an entity is dead? elexis: Since when do we check by the animation whether an entity is dead? | ||||||||||
Done Inline ActionsIf you have a better way, let me know. CmpHealth returns the amount of resources instead of the life. Stan: If you have a better way, let me know. CmpHealth returns the amount of resources instead of the… | ||||||||||
Done Inline ActionscmpResourceSupply: am I a joke to you? On a more serious note, that is probably as bad as it sounds. lyv: `cmpResourceSupply: am I a joke to you?`
On a more serious note, that is probably as bad as it… | ||||||||||
Done Inline ActionsAttempt at a more productive comment. lyv: Attempt at a more productive comment.
I guess the real problem here is the fact that the… | ||||||||||
Done Inline Actionsunneeded braces elexis: unneeded braces | ||||||||||
Done Inline ActionsI can upload a short linter script for linux that checks for whitespace issues like these. One could script something similar for windows somehow I guess. But one should still get the habit to never leave the current line unfixed, then one doesn't have to revisit it later. elexis: I can upload a short linter script for linux that checks for whitespace issues like these. One… | ||||||||||
Done Inline Actionsamount - decayRate, wrong units, the first one is a fixed amount, the second one is an amount per time according to the name, right? (So something looks like it should be renamed.) elexis: amount - decayRate, wrong units, the first one is a fixed amount, the second one is an amount… | ||||||||||
Done Inline Actionsold -> oldAmount, so that the reader knows what the variable is about without infering it from the other lines. Perhaps it would be cleaner to just pass the diff. Why is this function not called from SetAmount directly? If it's mandatory to call UpdateSupplyStatus every time after SetAmount, it could even be considered more error prone, as the callers have more responsibility to deal with the consequences of the previous functions, rather than the called function itself knowing what it has to do. (separation of concerns) elexis: `old` -> `oldAmount`, so that the reader knows what the variable is about without infering it… | ||||||||||
Done Inline Actions.SetInterval(\n maybe (at least I recall sanderd17 arguing about L262 appearing to miss a semicolon if one wouldnt read the next line. Also I recall JShint was complaining about these optionally) elexis: .SetInterval(\n maybe (at least I recall sanderd17 arguing about L262 appearing to miss a… | ||||||||||
Done Inline Actionsif (this.template.Growth.GrowsWhenAliveOnly) would be be more ordered (parent -> child -> grandchild). (This either returns an object or undefined, so there's no strict need to test like this) elexis: `if (this.template.Growth.GrowsWhenAliveOnly)` would be be more ordered (parent -> child ->… | ||||||||||
Done Inline ActionsFor some reason, it doesn't work if I do anything else, just returns false. Stan: For some reason, it doesn't work if I do anything else, just returns false. | ||||||||||
Done Inline Actions=== makes it look like == is unsufficient, so better only use it if it is the case (also for consistency with the rest of the code) elexis: `===` makes it look like `==` is unsufficient, so better only use it if it is the case (also… | ||||||||||
Done Inline ActionsWas this the whitespace convention we used for switch? (I only recall it for C++ which uses one tab less, but dont recall about JS, should compare with other JS code). Perhaps if (x && y || a && b) return is shorter. That would remove the warning, but in theory that warning should take place when the template is parsed (it's not the case is it?) elexis: Was this the whitespace convention we used for switch? (I only recall it for C++ which uses one… | ||||||||||
Done Inline ActionsI'm not sure. I'm not a fan of the cpp ones because it looks weird to me and isn't the default formatting in most editors. Yes might be smaller. Not sure about readibili ty. Need to check if choices are checked when parsing templates. Stan: I'm not sure. I'm not a fan of the cpp ones because it looks weird to me and isn't the default… | ||||||||||
Done Inline Actionsif ("GrowsWhenAliveOnly" in this.template.Growth) elexis: `if ("GrowsWhenAliveOnly" in this.template.Growth)`
<->
`if (this.template.Growth. | ||||||||||
Done Inline ActionsFor some reason that return false :( Stan: For some reason that return false :( | ||||||||||
Done Inline Actionsthis.template.Growth.GrowsWhenAliveOnly will be falsy always since it's an empty element. lyv: `this.template.Growth.GrowsWhenAliveOnly` will be falsy always since it's an empty element. | ||||||||||
Done Inline Actions!== undefined elexis: !== undefined | ||||||||||
Done Inline ActionsAddRegenTimer is now unified, but GrowSupply and DecaySupply have exactly the same code, and the call to AddRegenTimer is duplicated too. It could just be: doSomething() elexis: AddRegenTimer is now unified, but GrowSupply and DecaySupply have exactly the same code, and… | ||||||||||
Done Inline Actions-\n elexis: -\n | ||||||||||
Done Inline ActionsWondering if the two functions (called and callee) shouldn't be merged elexis: Wondering if the two functions (called and callee) shouldn't be merged | ||||||||||
Done Inline Actionsget a habit for if ( elexis: get a habit for if ( | ||||||||||
Done Inline ActionsI'm spending do much time in visual Studio I sometimes forget not everything is automatic Stan: I'm spending do much time in visual Studio I sometimes forget not everything is automatic | ||||||||||
Done Inline ActionsMessed up indentation? lyv: Messed up indentation? | ||||||||||
Done Inline Actions&& has a greater operator precedence than ||, so the inner parentheses are unneeded f && (x && y|| a && b) whitespace convention is supposed to be: \tif (foo && \t bar) elexis: && has a greater operator precedence than ||, so the inner parentheses are unneeded f && (x &&… | ||||||||||
Done Inline Actions+ "Timer" unneeded elexis: + "Timer" unneeded | ||||||||||
Done Inline ActionsThought about setting that on init conditionally, but this one would even be better for memory. Wondering whether the just-in-time setting of a property is healthy for serialization and deserialization, but I think it shouldn't be a problem considering that it just generically serializes the JS::Value (CComponentTypeScript::Serialize). elexis: Thought about setting that on `init` conditionally, but this one would even be better for… | ||||||||||
Done Inline Actions(wouldn't reject each property and }); on a separate line over the 129 char long line, but that's arbitrary) elexis: (wouldn't reject each property and }); on a separate line over the 129 char long line, but… |
removed