Gender is replaced with phenotype.
This allows a "{phenotype}" replacement string in the VisualActor-node. Also allows a "random" phenotype chosen from the tokens given in Phenotype.
Details
Use the "{phenotype}"-tag in the "VisualActor"-node and all should be fine.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 7805 Build 12701: Vulcan Build Build 12700: arc lint + arc unit
Event Timeline
- JSDOC.
- Use newPhenotype in SetPhenotype function.
- Removed repetitive choices in help of "Identity.js".
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (indent): | | Expected indentation of 3 tabs but found 4. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Sound.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Sound.js | 43| 43| let phenotype = cmpIdentity.GetPhenotype(); | 44| 44| | 45| 45| let soundName = this.template.SoundGroups[name].replace(/\{lang\}/g, lang) | 46| |- .replace(/\{phenotype\}/g, phenotype); | | 46|+ .replace(/\{phenotype\}/g, phenotype); | 47| 47| let cmpSoundManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_SoundManager); | 48| 48| if (cmpSoundManager) | 49| 49| cmpSoundManager.PlaySoundGroup(soundName, this.entity); Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1847/display/redirect
You might want to look at Vulkan's warnings as well. I wish he would write inlines but that might be messy.
binaries/data/mods/public/simulation/components/Identity.js | ||
---|---|---|
20 ↗ | (On Diff #8651) | I think the last part is kinda misleading. It could get people to think they cannot do what they want. |
106 ↗ | (On Diff #8651) | Pretty sure calling a function without default parameters (param = defaultvalue) without any parameters will trigger some warnings. |
152 ↗ | (On Diff #8651) | I think you could use this as default param. |
binaries/data/mods/public/simulation/components/Sound.js | ||
45 ↗ | (On Diff #8651) | I wonder if all this shouldn't be handled in cmpidentity. To make the other components agnostic. That would simplify the code a lot here and in cmpvisual actor I think you wanted to do it |
source/simulation2/components/CCmpVisualActor.cpp | ||
26 | @wraitii maybe forward class declaration ? |
I think your problems stem from not having set a phenotype in most templates. If you had male everywhere a phenotype is needed, Identity.prototype.SetPhenotype would be much cleaner.
So I think you ought to do it. Though since basically all of our units are male, you could perhaps just put it at the root (template_unit I believe) and make women citizen an exception.
I usally do, perphaps I forgot since I've been editing UnitAI.js a lot lately, which throws too many warnings ;)
Don't forget that nigh everything needs the Phenotype; as I mentioned earlier, buildings are male, as are many GAIA entities (mines, trees).
binaries/data/mods/public/simulation/components/Identity.js | ||
---|---|---|
106 ↗ | (On Diff #8651) | It seems like it is just undefined then, which is right what we want it to be, right? |
binaries/data/mods/public/simulation/components/Sound.js | ||
45 ↗ | (On Diff #8651) | Well, it seems odd to me that we would handle the soundName in "Identity.js". But if it is something that ought to be done, I'm okay with it, no strong opinion from my side ;) |
binaries/data/mods/public/simulation/components/Identity.js | ||
---|---|---|
19–20 ↗ | (On Diff #8655) | .. can be chosen when Phenotype is random. |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1851/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1883/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1919/display/redirect
Pretty close to done, see inlines.
Also you have no phenotype for, for example, gold mines. I'm not sure any values makes sense except "default".
binaries/data/mods/public/simulation/components/Identity.js | ||
---|---|---|
106 ↗ | (On Diff #8747) | Since you're only calling this here, I would actually remove the function entirely and just init here with the template value. If future code wants to be able to change phenotypes, it'll add the function then. |
115 ↗ | (On Diff #8747) | I think it'd still work if you just removed the Identity.prototype.Serialize function altogether |
Well, the function GetPhenotype *is* called for them (i.e. mines, trees, buildings etc. (not fish; don't ask me why)), so I'm not sure that "default" would not throw errors, but I'll test.
Open questions:
I wonder if all this shouldn't be handled in cmpidentity. To make the other components agnostic. That would simplify the code a lot here and in cmpvisual actor I think you wanted to do it
I wonder if the "{phenotype}" string could not be a constant in the js component instead ?
binaries/data/mods/public/simulation/components/Identity.js | ||
---|---|---|
106 ↗ | (On Diff #8747) | Okay, but why remove support? If some mod wants to let entities change phenotype, the function is already here. |
115 ↗ | (On Diff #8747) | Probably, but don't we want to serialise as little as possible? |
I wonder if all this shouldn't be handled in cmpidentity. To make the other components agnostic. That would simplify the code a lot here and in cmpvisual actor I think you wanted to do it
What 'all this' are you referring to?
I wonder if the "{phenotype}" string could not be a constant in the js component instead ?
Meh, don't think we'd want to change that too soon and it's trivial to grep.
binaries/data/mods/public/simulation/components/Identity.js | ||
---|---|---|
106 ↗ | (On Diff #8747) | Meh, trivial to add imo. |
115 ↗ | (On Diff #8747) | It's not really been a concern of JS code, and it's probably better to auto-serialise since then you won't forget to serialise any newly added variable. |
I think what @Stan meant was that the replacing of the tags in the string should be handled not in Sound.js and not in CCmpVisualActor.cpp but in Identity.js.
- Non-unit Phenotype defaults to default.
- More serialisation.
- Removed SetPhenotype-function.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/ICmpIdentity.h | 27| class·ICmpIdentity·:·public·IComponent | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classICmpIdentity:' is invalid C code. Use --std or --language to configure the language. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/243/display/redirect
binaries/data/mods/public/simulation/components/Identity.js | ||
---|---|---|
114 ↗ | (On Diff #9133) | I think you might be able to delete Deserialize() now you're using the default serialiser. |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/ICmpIdentity.h | 27| class·ICmpIdentity·:·public·IComponent | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classICmpIdentity:' is invalid C code. Use --std or --language to configure the language. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/251/display/redirect
This still fails to load properly on UnitDemo.js as you still have some entities with no this.phenotype
Since the component is optional, I would just default this.phenotype to "default" in those situations instead of specifying it in the template.
Otherwise works. Would be nice to add a quick test for GetPossiblePhenotypes
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/ICmpIdentity.h | 27| class·ICmpIdentity·:·public·IComponent | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classICmpIdentity:' is invalid C code. Use --std or --language to configure the language. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/257/display/redirect
Finally remembered to test loading a saved game, and it turns out VisualActor complained, as we were initialising the actor with the phenotype not swapped out as CmpIdentity, being a scripted component, didn't exist. Subscribe to MT_Create to fix this.
Bit more involved so I did it myself.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/ICmpIdentity.h | 27| class·ICmpIdentity·:·public·IComponent | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classICmpIdentity:' is invalid C code. Use --std or --language to configure the language. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/260/display/redirect
binaries/data/mods/public/simulation/components/Identity.js | ||
---|---|---|
20–30 ↗ | (On Diff #9158) | Just a thought: we could make it a choice; either PossiblePhenotypes (which implies a random Phenotype) or a defined Phenotype? Mainly because PossiblePhenotypes has no meaning without a random Phenotype? |
binaries/data/mods/public/simulation/components/Identity.js | ||
---|---|---|
20–30 ↗ | (On Diff #9158) | That sounds annoying to inherit. If your parent is random phenotype, and a child ends up specifying one, it has to delete the "possible phenotypes" thing. Maybe what one could do is that Phenotype is a list of tokens - if it's just one token it has to be that, otherwise it picks randomly from the list. That might work. |
Merged PossiblePhenotypes and Phenotypes. Phenotypes now supports a list. If a list of tokens is given, a random one will be chosen. If only one option is given that will be the phenotype.
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/278/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/ICmpIdentity.h | 27| class·ICmpIdentity·:·public·IComponent | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classICmpIdentity:' is invalid C code. Use --std or --language to configure the language. Executing section JS... | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'Phenotype'. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Identity.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Identity.js | 21| 21| cmpIdentity = ConstructComponent(6, "Identity", { | 22| 22| "Civ": "iber", | 23| 23| "Lang": "iberian", | 24| |- "Phenotype": { "_string": "female" }, | | 24|+ "Phenotype": { "_string": "female" }, | 25| 25| "GenericName": "Iberian Skirmisher", | 26| 26| "SpecificName": "Lusitano Ezpatari", | 27| 27| "SelectionGroupName": "units/iber_infantry_javelinist_b", Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/279/display/redirect
source/simulation2/components/CCmpVisualActor.cpp | ||
---|---|---|
582 | per @elexis suggestion that we ought not use boost here, I'd replace this with: size_t pos = base.find(pattern, pos); while (pos != std::string::npos) { base.replace(pos, pos + 11, cmpIdentity->GetPhenotype()); pos = base.find(pattern, pos + 11); } (needs some testing, but ought to work and be optimised enough) |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/ICmpIdentity.h | 27| class·ICmpIdentity·:·public·IComponent | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classICmpIdentity:' is invalid C code. Use --std or --language to configure the language. Executing section JS... | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'Phenotype'. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Identity.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Identity.js | 21| 21| cmpIdentity = ConstructComponent(6, "Identity", { | 22| 22| "Civ": "iber", | 23| 23| "Lang": "iberian", | 24| |- "Phenotype": { "_string": "female" }, | | 24|+ "Phenotype": { "_string": "female" }, | 25| 25| "GenericName": "Iberian Skirmisher", | 26| 26| "SpecificName": "Lusitano Ezpatari", | 27| 27| "SelectionGroupName": "units/iber_infantry_javelinist_b", Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/283/display/redirect