Page MenuHomeWildfire Games

Support "phenotype"-tag in VisualActor.
ClosedPublic

Authored by Freagarach on Jun 5 2019, 5:31 PM.

Details

Summary

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.

Test Plan

Use the "{phenotype}"-tag in the "VisualActor"-node and all should be fine.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Freagarach added inline comments.Jun 30 2019, 8:12 AM
binaries/data/mods/public/simulation/components/Identity.js
145 ↗(On Diff #8553)

The hardcoding of "male" is still a necessity here because the sounds use it. The hardcoding can be removed (by defining a phenotype for every template) in a seperate patch I guess.

Freagarach updated this revision to Diff 8651.Jun 30 2019, 8:15 AM
Freagarach marked an inline comment as done.
  • 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

Stan added a comment.Jun 30 2019, 10:36 AM

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 ↗(On Diff #8651)

@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.

Freagarach marked 3 inline comments as done.Jun 30 2019, 4:25 PM
In D1955#84411, @Stan wrote:

You might want to look at Vulkan's warnings as well. I wish he would write inlines but that might be messy.

I usally do, perphaps I forgot since I've been editing UnitAI.js a lot lately, which throws too many warnings ;)

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.

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 ;)

Freagarach updated this revision to Diff 8655.EditedJun 30 2019, 4:36 PM
Freagarach marked an inline comment as done.

Removed male default.

Angen added inline comments.Jun 30 2019, 4:57 PM
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

Freagarach updated this revision to Diff 8700.Jul 3 2019, 11:05 AM
Freagarach marked an inline comment as done.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1883/display/redirect

Freagarach updated this revision to Diff 8747.Jul 6 2019, 9:45 AM
Freagarach marked 3 inline comments as done.
Freagarach retitled this revision from Support for phenotype-tag in VisualActor. to Support "phenotype"-tag in VisualActor..
Freagarach edited the summary of this revision. (Show Details)

Check for cmpIdentity in CCmpVisualActor.cpp.

Vulcan added a comment.Jul 6 2019, 9:49 AM

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

wraitii requested changes to this revision.Jul 14 2019, 6:47 PM
This revision now requires changes to proceed.Jul 14 2019, 6:47 PM

Also you have no phenotype for, for example, gold mines. I'm not sure any values makes sense except "default".

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 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 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.

Freagarach updated this revision to Diff 9133.Fri, Jul 26, 6:52 PM
Freagarach marked 4 inline comments as done.
  • 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

wraitii added inline comments.Sat, Jul 27, 6:13 PM
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.

Freagarach updated this revision to Diff 9145.Sun, Jul 28, 7:40 AM

Removed deserialiser.

Freagarach marked an inline comment as done.Sun, Jul 28, 7:40 AM

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

wraitii requested changes to this revision.Sun, Jul 28, 1:25 PM

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

This revision now requires changes to proceed.Sun, Jul 28, 1:25 PM
Freagarach updated this revision to Diff 9155.Sun, Jul 28, 3:07 PM
  • Proper default.
  • 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

wraitii commandeered this revision.Sun, Jul 28, 4:45 PM
wraitii edited reviewers, added: Freagarach; removed: wraitii.

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.

wraitii updated this revision to Diff 9158.Sun, Jul 28, 4:46 PM

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

Freagarach added inline comments.Thu, Aug 1, 9:20 AM
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?

wraitii added inline comments.Thu, Aug 1, 10:09 AM
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.

Freagarach commandeered this revision.Thu, Aug 1, 10:43 AM
Freagarach edited reviewers, added: wraitii; removed: Freagarach.
Freagarach updated this revision to Diff 9183.Thu, Aug 1, 10:46 AM

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.

Freagarach edited the summary of this revision. (Show Details)Thu, Aug 1, 10:47 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/278/display/redirect

Freagarach updated this revision to Diff 9184.Thu, Aug 1, 10:52 AM

Fixed test.

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

wraitii added inline comments.Thu, Aug 1, 3:20 PM
source/simulation2/components/CCmpVisualActor.cpp
575 ↗(On Diff #9184)

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)

Freagarach updated this revision to Diff 9188.Thu, Aug 1, 7:11 PM
Freagarach marked an inline comment as done.

Replaced boost (how ironic).

Vulcan added a comment.Thu, Aug 1, 7:17 PM

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

wraitii accepted this revision.Thu, Aug 1, 8:42 PM

Tested, working, code looks neat and clean.

This revision is now accepted and ready to land.Thu, Aug 1, 8:42 PM
This revision was automatically updated to reflect the committed changes.

Thanks @wraitii for committing this!