Changeset View
Standalone View
binaries/data/mods/public/simulation/components/ModificationsManager.js
- This file was added.
function ModificationsManager() {} | |||||
ModificationsManager.prototype.Schema = | |||||
"<empty/>"; | |||||
ModificationsManager.prototype.Init = function() | |||||
{ | |||||
// TODO: | |||||
// - add a way to show an icon for a given modification ID | |||||
// > Note that aura code shows icons when the source is selected, so that's specific to them. | |||||
leper: This behaviour is still ugly, then again this is just moving it. | |||||
Not Done Inline ActionsI can't see a really easy way to change this unless we start using different functions for serialisation and OOS-check? wraitii: I can't see a really easy way to change this unless we start using different functions for… | |||||
// - support stacking modifiers (MultiKeyMap handles it but not this manager). | |||||
// The cache computes values lazily when they are needed. | |||||
// Helper functions remove items that have been changed to ensure we stay up-to-date. | |||||
this.cachedValues = new Map(); // Keyed by property name, entity ID, original values. | |||||
Done Inline ActionsI'm just going to assume that there'll be a comment here that'll state "(this line intentionally left blank.)". leper: I'm just going to assume that there'll be a comment here that'll state "(this line… | |||||
// When changing global modifications, all entity-local caches are invalidated. This helps with that. | |||||
// TODO: it might be worth keying by classes here. | |||||
this.playerEntitiesCached = new Map(); // Keyed by player ID, property name, entity ID. | |||||
this.modifsStorage = new MultiKeyMap(); // Keyed by property name, entity. | |||||
this.modifsStorage._OnItemModified = (prim, sec, itemID) => this.ModificationsChanged.apply(this, [prim, sec, itemID]); | |||||
}; | |||||
ModificationsManager.prototype.Serialize = function() | |||||
{ | |||||
// The value cache will be affected by property reads from the GUI and other places so we shouldn't serialize it. | |||||
// Furthermore it is cyclically self-referencing. | |||||
// We need to store the player for the Player-Entities cache. | |||||
let players = []; | |||||
this.playerEntitiesCached.forEach((_, player) => players.push(player)); | |||||
StanUnsubmitted Not Done Inline ActionsCan't you just do : let players = this.playerEntitiesCached Or use https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Objets_globaux/Array/from Stan: Can't you just do :
```lang=js
let players = this.playerEntitiesCached
```
Or use https… | |||||
wraitiiAuthorUnsubmitted Done Inline ActionsMap() isn't behaving nicely. wraitii: Map() isn't behaving nicely. | |||||
return { | |||||
"modifsStorage": this.modifsStorage.Serialize(), | |||||
Not Done Inline ActionsMaybe !this[propName] merged with the other if. Stan: Maybe !this[propName] merged with the other if. | |||||
"players": players | |||||
}; | |||||
}; | |||||
ModificationsManager.prototype.Deserialize = function(data) | |||||
{ | |||||
this.Init(); | |||||
this.modifsStorage.Deserialize(data.modifsStorage); | |||||
Done Inline ActionsWhy not call it thrice? leper: Why not call it thrice? | |||||
Not Done Inline ActionsGood f-ing question. wraitii: Good f-ing question. | |||||
data.players.forEach(player => this.playerEntitiesCached.set(player, new Map())); | |||||
}; | |||||
/** | |||||
Not Done Inline Actionslet players = this.playerentities.slice() ? https://www.samanthaming.com/tidbits/35-es6-way-to-clone-an-array Stan: let players = this.playerentities.slice() ?
https://www.samanthaming.com/tidbits/35-es6-way-to… | |||||
* Inform entities that we have changed possibly all values affected by that property. | |||||
* It's not hugely efficient and would be nice to batch. | |||||
Not Done Inline ActionsQuotes around object properties. Stan: Quotes around object properties. | |||||
* Invalidate caches where relevant. | |||||
*/ | |||||
ModificationsManager.prototype.ModificationsChanged = function(propertyName, entity) | |||||
{ | |||||
let playerCache = this.playerEntitiesCached.get(entity); | |||||
this.InvalidateCache(propertyName, entity, playerCache); | |||||
Not Done Inline ActionsWhat are versions, and why are they needed? leper: What are versions, and why are they needed? | |||||
Not Done Inline ActionsThey're used for the caching. When a property changes, the version is incremented, so we can lazily recomput it. It's indeed not the most obvious thing, I've tried improving that. wraitii: They're used for the caching. When a property changes, the version is incremented, so we can… | |||||
if (playerCache) | |||||
{ | |||||
let cmpPlayer = Engine.QueryInterface(entity, IID_Player); | |||||
if (cmpPlayer) | |||||
this.SendPlayerModificationMessages(propertyName, cmpPlayer.GetPlayerID()); | |||||
} | |||||
else | |||||
Engine.PostMessage(entity, MT_ValueModification, { "entities": [entity], "component": propertyName.split("/")[0], "valueNames": [propertyName] }); | |||||
}; | |||||
ModificationsManager.prototype.SendPlayerModificationMessages = function(propertyName, player) | |||||
{ | |||||
// TODO: it would be preferable to be able to batch this (i.e. one message for several properties) | |||||
Not Done Inline ActionsAt least here we only locally notify everyone twice. leper: At least here we only locally notify everyone twice. | |||||
Engine.PostMessage(SYSTEM_ENTITY, MT_TemplateModification, { "player": player, "component": propertyName.split("/")[0], "valueNames": [propertyName] }); | |||||
// AIInterface wants the entities potentially affected. | |||||
Not Done Inline ActionsEarly return; to nuke else. Stan: Early return; to nuke else. | |||||
// TODO: improve on this | |||||
let cmpRangeManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager); | |||||
let ents = cmpRangeManager.GetEntitiesByPlayer(player); | |||||
Engine.BroadcastMessage(MT_ValueModification, { "entities": ents, "component": propertyName.split("/")[0], "valueNames": [propertyName] }); | |||||
}; | |||||
ModificationsManager.prototype.InvalidatePlayerEntCache = function(valueCache, propertyName, entsMap) | |||||
{ | |||||
Not Done Inline ActionsMight want to be efficient in the first place, auras have always been a bottleneck. Stan: Might want to be efficient in the first place, auras have always been a bottleneck. | |||||
entsMap = entsMap.get(propertyName); | |||||
if (entsMap) | |||||
{ | |||||
// Invalidate all local caches directly (for simplicity in ApplyModifications). | |||||
entsMap.forEach(ent => valueCache.set(ent, new Map())); | |||||
entsMap.clear(); | |||||
} | |||||
}; | |||||
ModificationsManager.prototype.InvalidateCache = function(propertyName, entity, playerCache) | |||||
{ | |||||
Not Done Inline ActionsCan't we guess who is gonna really need to be affected to reduce the amount of entities ? Some kind of filter if you will. Stan: Can't we guess who is gonna really need to be affected to reduce the amount of entities ? Some… | |||||
let valueCache = this.cachedValues.get(propertyName); | |||||
if (!valueCache) | |||||
return; | |||||
if (playerCache) | |||||
this.InvalidatePlayerEntCache(valueCache, propertyName, playerCache); | |||||
else | |||||
valueCache.set(entity, new Map()); | |||||
Not Done Inline ActionsBraces Stan: Braces | |||||
}; | |||||
/** | |||||
* @returns originalValue after modifications. | |||||
Not Done Inline ActionsI guess you can merge all three. Stan: I guess you can merge all three. | |||||
*/ | |||||
ModificationsManager.prototype.FetchModifiedProperty = function(classesList, propertyName, originalValue, target) | |||||
{ | |||||
let modifs = this.modifsStorage.GetItems(propertyName, target); | |||||
if (!modifs.length) | |||||
Not Done Inline ActionsProfiling for array.foreach, and standard for, also isn't it bad practice to delete in a foreach loop ? Stan: Profiling for array.foreach, and standard for, also isn't it bad practice to delete in a… | |||||
return originalValue; | |||||
return GetTechModifiedProperty(modifs, classesList, originalValue); | |||||
}; | |||||
/** | |||||
* @returns originalValue after modifications | |||||
*/ | |||||
ModificationsManager.prototype.Cache = function(classesList, propertyName, originalValue, entity) | |||||
{ | |||||
let cache = this.cachedValues.get(propertyName); | |||||
Not Done Inline Actionsmerge all three, and invert the first ? Stan: merge all three, and invert the first ? | |||||
if (!cache) | |||||
cache = this.cachedValues.set(propertyName, new Map()).get(propertyName); | |||||
let cache2 = cache.get(entity); | |||||
if (!cache2) | |||||
Not Done Inline ActionsOne could probably argue for using .has when checking for existence. leper: One could probably argue for using .has when checking for existence. | |||||
cache2 = cache.set(entity, new Map()).get(entity); | |||||
let value = this.FetchModifiedProperty(classesList, propertyName, originalValue, entity); | |||||
cache2.set(originalValue, value); | |||||
return value; | |||||
}; | |||||
/** | |||||
* Caching system in front of FetchModifiedProperty(), as calling that every time is quite slow. | |||||
* This recomputes lazily. | |||||
* Applies per-player modifications before per-entity modifications, so the latter take priority; | |||||
* @param propertyName - Handle of a technology property (eg Attack/Ranged/Pierce) that was changed. | |||||
* @param originalValue - template/raw/before-modifications value. | |||||
Note that if this is supposed to be a number (i.e. you call add/multiply on it) | |||||
You must make sure to pass a number and not a string (by using + if necessary) | |||||
* @param ent - ID of the target entity | |||||
Not Done Inline ActionsCan probably be done with .any. leper: Can probably be done with .any. | |||||
* @returns originalValue after the modifications | |||||
*/ | |||||
ModificationsManager.prototype.ApplyModifications = function(propertyName, originalValue, entity) | |||||
{ | |||||
Not Done Inline ActionsI believe you can use Map.prototype.keys() to only get the properties then use some and return that :) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/keys Stan: I believe you can use Map.prototype.keys() to only get the properties then use some and return… | |||||
Done Inline ActionsYou're right about Keys, but map doesn't implement some() as said in the comment above. wraitii: You're right about Keys, but map doesn't implement some() as said in the comment above. | |||||
Not Done Inline ActionsBut keys do right ? since it's an array. Stan: But keys do right ? since it's an array. | |||||
Not Done Inline ActionsShould we return null or an empty collection ? Stan: Should we return null or an empty collection ? | |||||
let newValue = this.cachedValues.get(propertyName); | |||||
if (newValue) | |||||
StanUnsubmitted Not Done Inline ActionsSeems a bit weird at first read isn't it ? Stan: Seems a bit weird at first read isn't it ? | |||||
wraitiiAuthorUnsubmitted Done Inline Actionsit's much faster than the alternative. wraitii: it's much faster than the alternative. | |||||
{ | |||||
newValue = newValue.get(entity); | |||||
if (newValue) | |||||
{ | |||||
newValue = newValue.get(originalValue); | |||||
Not Done Inline ActionsMissing a this., same for a few others below. leper: Missing a `this.`, same for a few others below. | |||||
Not Done Inline ActionsThanks, forgot to cover that in the tests, added it now. wraitii: Thanks, forgot to cover that in the tests, added it now. | |||||
if (newValue) | |||||
return newValue; | |||||
} | |||||
} | |||||
// Get the entity ID of the player / owner of the entity, since we use that to store per-player modifications | |||||
// (this prevents conflicts between player ID and entity ID). | |||||
let ownerEntity = QueryOwnerEntityID(entity); | |||||
if (ownerEntity == entity) | |||||
ownerEntity = null; | |||||
newValue = originalValue; | |||||
Not Done Inline ActionsThis is nearly identical to the global one, why not use a helper that gets the map passed as a parameter instead of duplicating the code. Also applies to most other things in this file. leper: This is nearly identical to the global one, why not use a helper that gets the map passed as a… | |||||
Not Done Inline ActionsThe differences are the kind that are annoying to abstract (local must go one step deeper in the tree) and I don't think it'd actually abstract anything or be clearer in the Add/Remove functions to have an even more generic helper. I already added one for the heavy lifting, as you've noted. That particular duplication is a good candidate though. wraitii: The differences are the kind that are annoying to abstract (local must go one step deeper in… | |||||
Not Done Inline ActionsGiven that it is quite short now, I'm not sure if the helper is actually needed anymore. leper: Given that it is quite short now, I'm not sure if the helper is actually needed anymore. | |||||
let cmpIdentity = Engine.QueryInterface(entity, IID_Identity); | |||||
if (!cmpIdentity) | |||||
return originalValue; | |||||
let classesList = cmpIdentity.GetClassesList(); | |||||
// Apply player-wide modifications before entity-local modifications. | |||||
if (ownerEntity) | |||||
{ | |||||
let pc = this.playerEntitiesCached.get(ownerEntity).get(propertyName); | |||||
if (!pc) | |||||
pc = this.playerEntitiesCached.get(ownerEntity).set(propertyName, new Set()).get(propertyName); | |||||
pc.add(entity); | |||||
newValue = this.FetchModifiedProperty(classesList, propertyName, newValue, ownerEntity); | |||||
Not Done Inline ActionsWell... leper: Well... | |||||
} | |||||
newValue = this.Cache(classesList, propertyName, newValue, entity); | |||||
return newValue; | |||||
Not Done Inline ActionsWe don't really have a convention for internal methods. Some things use(d?) lower case for those, others might have used a leading _. Neither is really nice. (Just a general sentiment.) leper: We don't really have a convention for internal methods. Some things use(d?) lower case for… | |||||
Not Done Inline ActionsI switched to leading _ as the function names were getting long, but yes. Meh, I suppose? wraitii: I switched to leading _ as the function names were getting long, but yes. Meh, I suppose? | |||||
Not Done Inline ActionsAs said we don't have a convention, might be worth changing that at some point. leper: As said we don't have a convention, might be worth changing that at some point. | |||||
}; | |||||
/** | |||||
* Alternative version of ApplyModifications, applies to templates instead of entities. | |||||
* Only needs to handle global modifications. | |||||
Not Done Inline ActionsWouldn't it be better to pass [single value] everywhere to remove that need ? Stan: Wouldn't it be better to pass [single value] everywhere to remove that need ? | |||||
*/ | |||||
Not Done Inline ActionsApparently the usage of helpers is also inconsistent. leper: Apparently the usage of helpers is also inconsistent. | |||||
ModificationsManager.prototype.ApplyModificationsTemplate = function(propertyName, originalValue, template, player) | |||||
{ | |||||
Not Done Inline ActionsTo preempt someone else, start a sentence with the correct case. leper: To preempt someone else, start a sentence with the correct case. | |||||
Not Done Inline ActionsWhy is "Code bug: " needed, it is an error, what else should it be? leper: Why is "Code bug: " needed, it is an error, what else should it be? | |||||
if (!template || !template.Identity) | |||||
Not Done Inline ActionsIs this done a lot, if yes, why not store the data in a way that makes those accesses nicer? leper: Is this done a lot, if yes, why not store the data in a way that makes those accesses nicer? | |||||
Not Done Inline ActionsDone when adding/removing a modifier. I suppose it could be made nicer, but I wanted a list to have a consistent, known ordering (which an object wouldn't have iirc?). I could use a secondary structure, if it proves being performance-intensive. wraitii: Done when adding/removing a modifier. I suppose it could be made nicer, but I wanted a list to… | |||||
return originalValue; | |||||
let cmpPlayerManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager); | |||||
return this.FetchModifiedProperty(GetIdentityClasses(template.Identity), propertyName, originalValue, cmpPlayerManager.GetPlayerByID(player)); | |||||
Not Done Inline ActionsThe internal ID seems interesting, but also listing the propertyName might help users more. leper: The internal ID seems interesting, but also listing the propertyName might help users more. | |||||
Not Done Inline ActionsFair enough, though this modificationID conflict should only happen with user-defined IDs (in sim code, it's indicative of a code bug elsewhere), hence why I gave the ID. wraitii: Fair enough, though this modificationID conflict should only happen with user-defined IDs (in… | |||||
}; | |||||
/** | |||||
* For efficiency in InvalidateCache, keep playerEntitiesCached updated. | |||||
*/ | |||||
ModificationsManager.prototype.OnGlobalPlayerEntityChanged = function(msg) | |||||
Not Done Inline ActionsThat sounds like a weird error message, maybe remove the "code bug:" part. Stan: That sounds like a weird error message, maybe remove the "code bug:" part. | |||||
{ | |||||
if (msg.to != INVALID_PLAYER && !this.playerEntitiesCached.has(msg.to)) | |||||
this.playerEntitiesCached.set(msg.to, new Map()); | |||||
if (msg.from != INVALID_PLAYER && this.playerEntitiesCached.has(msg.from)) | |||||
Not Done Inline ActionsWhy not do that here, instead of delaying it? Also why is this needed? leper: Why not do that here, instead of delaying it?
Also why is this needed? | |||||
Not Done Inline ActionsThe cache I implemented lazily recomputes value when accessed, if necessary. Necessary being "version of cache < version of modifications". I didn't want to have to update all values when a property changes because that might be unexpectedly slow and we might do far too much work, and it seemed more sensible. IIRC the aura code currently recomputes on any change, but I would have to check to be sure, it's fairly complex. wraitii: The cache I implemented lazily recomputes value when accessed, if necessary. Necessary being… | |||||
Not Done Inline ActionsYes, but if we increase the version, we know that the currently cached one is invalid and has to be updated on the next run. So we could just remove that specific piece from the cache and not have to keep trac of versions at all. The next access will populate the cache with the new value again, keeping the lazy recomputation. leper: Yes, but if we increase the version, we know that the currently cached one is invalid and has… | |||||
Not Done Inline ActionsRight. I think I didn't do it this way originally because I cached on "Ent - property - value" and thus this meant looping over all entities in the cache. The problem is that if I want to clean up old entities, I need to loop over all properties if I switch those around, but that's probably about as fast and nicer in the end so OK. wraitii: Right. I think I didn't do it this way originally because I cached on "Ent - property - value"… | |||||
{ | |||||
this.playerEntitiesCached.get(msg.from).forEach(propName => this.InvalidateCache(propName, msg.from)); | |||||
this.playerEntitiesCached.delete(msg.from); | |||||
Not Done Inline ActionsLast I checked that's not how we wrote those comments. leper: Last I checked that's not how we wrote those comments. | |||||
Not Done Inline ActionsI thought that was JSDoc style for one-liners? wraitii: I thought that was JSDoc style for one-liners? | |||||
Not Done Inline ActionsIIRC we don't have one-liners for JSDoc. leper: IIRC we don't have one-liners for JSDoc. | |||||
} | |||||
}; | |||||
Not Done Inline ActionsI'd have never guessed that given the function name. leper: I'd have never guessed that given the function name. | |||||
/** | |||||
Done Inline ActionsWhy is this here? leper: Why is this here? | |||||
Not Done Inline ActionsSecret illuminati stuff. wraitii: Secret illuminati stuff. | |||||
* Handle modifications when an entity changes owner. | |||||
Not Done Inline ActionsThe comment about starting comments with capital letters applied to the whole patch (same as quite a few other remarks). leper: The comment about starting comments with capital letters applied to the whole patch (same as… | |||||
* We do not retain the original modifications for now. | |||||
*/ | |||||
ModificationsManager.prototype.OnGlobalOwnershipChanged = function(msg) | |||||
{ | |||||
if (msg.from == INVALID_PLAYER || msg.to == INVALID_PLAYER) | |||||
Not Done Inline ActionsThis seems more complicated that this need be. Why not remove something if count reaches 0? leper: This seems more complicated that this need be.
Why not remove something if count reaches 0? | |||||
Not Done Inline ActionsCan't because of the lazy cache. Though I probably, in this situation, could just go and remove the cache too. wraitii: Can't because of the lazy cache. Though I probably, in this situation, could just go and remove… | |||||
Not Done Inline ActionsWarn seems to be the wrong word here. leper: Warn seems to be the wrong word here. | |||||
return; | |||||
Not Done Inline ActionsThat might warrant a TODO. leper: That might warrant a TODO. | |||||
// Invalidate all caches. | |||||
Done Inline ActionsThis is the majorly wonky bit in this code, but it avoids duplications. wraitii: This is the majorly wonky bit in this code, but it avoids duplications. | |||||
for (let propName of this.cachedValues.keys()) | |||||
this.InvalidateCache(propName, msg.entity); | |||||
let owner = QueryOwnerEntityID(msg.entity); | |||||
if (!owner) | |||||
return; | |||||
Not Done Inline ActionsWell that warn seems pointless. leper: Well that warn seems pointless. | |||||
let cmpIdentity = Engine.QueryInterface(msg.entity, IID_Identity); | |||||
if (!cmpIdentity) | |||||
return; | |||||
Not Done Inline ActionsWhen is GetPlayerID undefined? Why not check for -1? Why call it twice two lines apart? leper: When is GetPlayerID undefined? Why not check for -1? Why call it twice two lines apart? | |||||
Not Done Inline ActionsSince the player ID is set in InitGame.js, I think on the first update (when auto-researched techs are researched) it might be undefined, but I'll have to check to be sure. I don't have to check for -1 because this is a component of the player entity, and I don't believe a player entity can have ID -1. Changed the latter part. wraitii: Since the player ID is set in InitGame.js, I think on the first update (when auto-researched… | |||||
Not Done Inline ActionsMight be helpful to document that. Or make sure that this doesn't happen if it shouldn't. leper: Might be helpful to document that. Or make sure that this doesn't happen if it shouldn't. | |||||
Not Done Inline Actions@Itms coala/eslint/jshint (unsure) had weird behaviour on this one, first telling me that the {} were unnecessary but then not understand why there were two tabs after the else. wraitii: @Itms coala/eslint/jshint (unsure) had weird behaviour on this one, first telling me that the… | |||||
let classes = cmpIdentity.GetClassesList(); | |||||
// Warn entities that our values have changed. | |||||
// Local modifications will be added by the relevant components, so no need to check for them here. | |||||
Done Inline ActionsCan we get this a little more vague and non-descriptive maybe? leper: Can we get this a little more vague and non-descriptive maybe? | |||||
let modifiedComponents = {}; | |||||
let playerModifs = this.modifsStorage.GetAllItems(owner); | |||||
Not Done Inline ActionsThat TODO should probably be done. leper: That TODO should probably be done. | |||||
Not Done Inline ActionsAgreed, should this go in this patch? wraitii: Agreed, should this go in this patch?
Note that this code should probably still remain, as it'd… | |||||
Not Done Inline ActionsINVALID_PLAYER Stan: INVALID_PLAYER | |||||
for (let propertyName in playerModifs) | |||||
{ | |||||
// We only need to find one one tech per component for a match. | |||||
Not Done Inline ActionsINVALID_PLAYER Stan: INVALID_PLAYER | |||||
let component = propertyName.split("/")[0]; | |||||
// Only inform if the modification actually applies to the entity as an optimisation. | |||||
// TODO: would it be better to call FetchModifiedProperty here and compare values? | |||||
playerModifs[propertyName].forEach(modif => { | |||||
if (!DoesModificationApply(modif, classes)) | |||||
return; | |||||
if (!modifiedComponents[component]) | |||||
modifiedComponents[component] = []; | |||||
Not Done Inline ActionsUnused. leper: Unused. | |||||
modifiedComponents[component].push(propertyName); | |||||
}); | |||||
} | |||||
for (let component in modifiedComponents) | |||||
Not Done Inline ActionsINVALID_PLAYER Stan: INVALID_PLAYER | |||||
Engine.PostMessage(msg.entity, MT_ValueModification, { "entities": [msg.entity], "component": component, "valueNames": modifiedComponents[component] }); | |||||
elexisUnsubmitted Not Done Inline Actions(Wondering if one could avoid object construction by somehow creating the message objects to be sent directly instead of using an intermediary object array) elexis: (Wondering if one could avoid object construction by somehow creating the message objects to be… | |||||
wraitiiAuthorUnsubmitted Done Inline ActionsThis ValueModification message is quite slow too, since the "component" filter means we always send it redundantly to at least 8/9 times too many components, which is rather slow. I think we're reaching a hard limit of our current messaging system and I think we'll just have to improve it to really improve things. wraitii: This ValueModification message is quite slow too, since the "component" filter means we always… | |||||
}; | |||||
/** | |||||
* The following functions simply proxy MultiKeyMap's interface. | |||||
*/ | |||||
elexisUnsubmitted Not Done Inline ActionsSounds unneeded if it could also call the functions of that variable directly. Might even help with performance. elexis: Sounds unneeded if it could also call the functions of that variable directly. Might even help… | |||||
wraitiiAuthorUnsubmitted Done Inline ActionsIt becomes cumbersome to write cmpModificationsManager.modifsStorage.AddItem, but I guess not more so than cmpModificationsManager.addModification, which is why I went with cmpModificationsManager.addModif Perhaps I should just merge this back with MultiKeyMap. From profiling, having an intermediary function changes nothing measurable. wraitii: It becomes cumbersome to write `cmpModificationsManager.modifsStorage.AddItem`, but I guess not… | |||||
ModificationsManager.prototype.AddModif = function(primaryKey, ModifID, Modif, secondaryKey, stackable = false) { | |||||
return this.modifsStorage.AddItem(primaryKey, ModifID, Modif, secondaryKey, stackable); | |||||
}; | |||||
Not Done Inline ActionsIf we already check a few things based on the type of that exact parameter, why not actually check this? leper: If we already check a few things based on the type of that exact parameter, why not actually… | |||||
Not Done Inline ActionsThink that's just been moved around (and rewritten without a second thought), you're right. wraitii: Think that's just been moved around (and rewritten without a second thought), you're right. | |||||
Not Done Inline ActionsHm on second thought it's not that easy: string values are perfectly legitimate if only "replace" is called on them (and I had a "token" patch at some point). The only way to evaluate this correctly would be to go through all modifications for that property and check if any is add/mul. That might mean be doing a lot of work for a sanity check. @elexis opinion on this? wraitii: Hm on second thought it's not that easy: string values are perfectly legitimate if only… | |||||
ModificationsManager.prototype.AddModifs = function(ModifID, Modifs, secondaryKey, stackable = false) { | |||||
return this.modifsStorage.AddItems(ModifID, Modifs, secondaryKey, stackable); | |||||
}; | |||||
ModificationsManager.prototype.RemoveModif = function(primaryKey, ModifID, secondaryKey, stackable = false) { | |||||
Not Done Inline Actionshuh! leper: huh! | |||||
return this.modifsStorage.RemoveItem(primaryKey, ModifID, secondaryKey, stackable); | |||||
}; | |||||
ModificationsManager.prototype.RemoveAllModifs = function(ModifID, secondaryKey, stackable = false) { | |||||
return this.modifsStorage.RemoveAllItems(ModifID, secondaryKey, stackable); | |||||
}; | |||||
Not Done Inline ActionsI returned undefined here to hopefully break the user code (fail fast), is that seen as a good idea? wraitii: I returned undefined here to hopefully break the user code (fail fast), is that seen as a good… | |||||
ModificationsManager.prototype.HasModif = function(primaryKey, ModifID, secondaryKey) { | |||||
return this.modifsStorage.HasItem(primaryKey, ModifID, secondaryKey); | |||||
}; | |||||
ModificationsManager.prototype.HasAnyModif = function(ModifID, secondaryKey) { | |||||
return this.modifsStorage.HasAnyItem(ModifID, secondaryKey); | |||||
}; | |||||
Not Done Inline ActionsWait, now there are global and local versions too? leper: Wait, now there are global and local versions too? | |||||
Not Done Inline ActionsThat's the version for the cache (i.e. the last version at which this was computed), and yes there's always been a local and a global copy (it's just that since the lists are different in the {this} objects, they're named the same. wraitii: That's the version for the cache (i.e. the last version at which this was computed), and yes… | |||||
ModificationsManager.prototype.GetModifs = function(primaryKey, secondaryKey, stackable = false) { | |||||
return this.modifsStorage.GetItems(primaryKey, secondaryKey, stackable); | |||||
}; | |||||
ModificationsManager.prototype.GetAllModifs = function(secondaryKey, stackable = false) { | |||||
return this.modifsStorage.GetAllItems(secondaryKey, stackable); | |||||
}; | |||||
Engine.RegisterSystemComponentType(IID_ModificationsManager, "ModificationsManager", ModificationsManager); | |||||
Not Done Inline ActionsThis seems slightly pointless since we had that exact data just a few lines ago. leper: This seems slightly pointless since we had that exact data just a few lines ago. | |||||
Not Done Inline ActionsIt was actually needed (the key might not exist and we don't know what triggered the need to update) but it's gone in the new version. wraitii: It was actually needed (the key might not exist and we don't know what triggered the need to… | |||||
Not Done Inline ActionsWhy is this check done only here, if none of the rest matters if that is the case? leper: Why is this check done only here, if none of the rest matters if that is the case? | |||||
Not Done Inline ActionsGood point. wraitii: Good point. | |||||
Not Done Inline ActionsDidn't we update that thing just a few lines ago? Why do this again? leper: Didn't we update that thing just a few lines ago? Why do this again? | |||||
Not Done Inline ActionsSame as for the other if. leper: Same as for the other if. | |||||
Not Done Inline ActionsThis function is too complex, I'm not even sure anymore what it does by now. leper: This function is too complex, I'm not even sure anymore what it does by now. | |||||
Not Done Inline ActionsHm, you're probably right. Admittedly I had trouble writing it correctly, so it's definitely in need of a little chopping up. wraitii: Hm, you're probably right. Admittedly I had trouble writing it correctly, so it's definitely in… | |||||
Not Done Inline ActionsAlso had trouble writing the refactored version, in fact. Good thing I added tests. wraitii: Also had trouble writing the refactored version, in fact. Good thing I added tests. | |||||
Not Done Inline ActionsWe had that already, also this one even lacks a *. leper: We had that already, also this one even lacks a *. | |||||
Not Done Inline Actions
What do you mean? wraitii: > We had that already
What do you mean? | |||||
Not Done Inline ActionsRandom inconsistent whitespace in random places. leper: Random inconsistent whitespace in random places. | |||||
Not Done Inline ActionsI think the auto patching from coala did that one, should have checked the output. wraitii: I think the auto patching from coala did that one, should have checked the output. | |||||
Not Done Inline ActionsThen why do we have strange intermediate temporary workaround-like code in here? leper: Then why do we have strange intermediate temporary workaround-like code in here? | |||||
Not Done Inline ActionsAnd why do we get such new code? leper: And why do we get such new code? | |||||
Not Done Inline ActionsIt's not new code, it's basically the code from L214/233 of TechnologyManager.js in svn. I was merely commenting that the implementation could probably be made faster. I didn't implement the caching because:
wraitii: It's not new code, it's basically the code from L214/233 of TechnologyManager.js in svn. I was… | |||||
Done Inline ActionsCould be inlined, given taht it is used only once. leper: Could be inlined, given taht it is used only once. | |||||
Done Inline ActionsWhy is this done after doing some work that wouldn't be needed if this branch is hit? leper: Why is this done after doing some work that wouldn't be needed if this branch is hit? | |||||
Done Inline ActionsSame. leper: Same. | |||||
Not Done Inline ActionsOr is this the way to do a JSDoc one liner? @elexis wraitii: Or is this the way to do a JSDoc one liner? @elexis | |||||
Not Done Inline ActionsForgot to use the new helper in that one, will do. wraitii: Forgot to use the new helper in that one, will do. | |||||
Not Done Inline ActionsSo many spaces, and yet it ends up not being properly aligned. Either do it properly, or don't do it. leper: So many spaces, and yet it ends up not being properly aligned.
Either do it properly, or don't… | |||||
Not Done Inline ActionsInconsistent case again. (Yes, I do complain about that because if no care is taken to get even something that simple right, or consistent with the one less than 10 lines above, then why should one even be able to assume that about the code.) leper: Inconsistent case again. (Yes, I do complain about that because if no care is taken to get even… | |||||
Not Done Inline ActionsGiven how complicated this got I by now quite prefer just cleaning the corresponding cache in case we update a value. That would be one place to invalidate something, instead of invalidating it, marking it as such, then waiting for other places to figure out if that happened, only to do something that we could have already done, without needless complexity that makes this whole thing hard to read, understand, debug, and reason about. leper: Given how complicated this got I by now quite prefer just cleaning the corresponding cache in… | |||||
Not Done Inline ActionsHow often is this actually used? (I'd check, but somehow my browser decided that searching is not that important, and the one hotkey for that is eaten by phabricator...) leper: How often is this actually used? (I'd check, but somehow my browser decided that searching is… | |||||
Not Done Inline ActionsHm, given the other helper (_FetchModifiedProperty) I used, it's not super useful indeed. wraitii: Hm, given the other helper (_FetchModifiedProperty) I used, it's not super useful indeed. | |||||
Not Done Inline ActionsStill broken. leper: Still broken. | |||||
Not Done Inline ActionsErf, it looks fine on my screen because the spaces align with the tabs which is why I missed it. wraitii: Erf, it looks fine on my screen because the spaces align with the tabs which is why I missed it. | |||||
Not Done Inline ActionsPointless (). Also is it guranteed that something has IID_Player, if it is just owned by someone? Not that it would change anything in this case as we'd just fail loudly. leper: Pointless ().
Also is it guranteed that something has IID_Player, if it is just owned by… | |||||
Not Done Inline Actionsthis.entity is the player ID, so that's pretty guaranteed to have IID_Player. wraitii: this.entity is the player ID, so that's pretty guaranteed to have IID_Player. | |||||
Not Done Inline ActionsWhy can that, should that happen ? Stan: Why can that, should that happen ? | |||||
Done Inline ActionsSee comments above. wraitii: See comments above. | |||||
Not Done Inline ActionsBraces here. Stan: Braces here. | |||||
Done Inline ActionsI need to check if this is still needed, I'm not sure. wraitii: I need to check if this is still needed, I'm not sure. |
This behaviour is still ugly, then again this is just moving it.