Page MenuHomeWildfire Games

Support for phenotype-tag in VisualActor.
Needs ReviewPublic

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

Details

Reviewers
wraitii
Summary

This allows a "{phenotype}" replacement string in the VisualActor-node. Also allows a "random" gender, which has a chance for the types defined in the "PossibleGenders" node.
Gender is replaced with 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
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8018
Build 13050: Vulcan BuildJenkins
Build 13049: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Freagarach retitled this revision from Feature request: Random gender. to Feature request: Random gender in VisualActor..Fri, Jun 7, 6:56 PM
Vulcan added a comment.Fri, Jun 7, 6:58 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/Identity.js
| 123| »   let·gender·=·!!this.template.Gender·==·"random"·?·randBool()·?·"male"·:·"female"·:·this.template.Gender·||·"male";·//·ugly·default
|    | [NORMAL] JSHintBear:
|    | Confusing use of '!'.
Executing section cli...

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

Angen added a comment.Fri, Jun 7, 8:32 PM

yes, needs to be serialised

binaries/data/mods/public/simulation/components/Identity.js
150

This is wrong here, pick up random gender in init function. This will return random gender for the same entity every time you call it.

elexis added a subscriber: elexis.Fri, Jun 7, 9:22 PM

(Feature was also requested by wowgetoffyourcellphone and others, as it's what AoK has. The voice would also depend on this identity property, not stating that it has to be done in this patch.)

binaries/data/mods/public/simulation/components/Identity.js
150

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence#Table

Instead of the "ugly default" it could error out if the template is broken.
I suppose not everything that has an Identity also has a Gender (buildings?)

Why hardcode male and female to begin with? What about space alien stuff with random fictive genders?

vladislavbelov added inline comments.
binaries/data/mods/public/simulation/components/Identity.js
31

Just wondering, why some choices start with upper case and some with lower case?

source/simulation2/components/CCmpVisualActor.cpp
71

Useless added member, should be a local variable.

211

It's not a good practice to repeat a search (m_BaseActorString.find(L"{gender}")).

212

We should avoid a hardcoded string in many places and magic numbers.

Freagarach marked an inline comment as done.Sat, Jun 8, 8:39 AM
In D1955#81447, @elexis wrote:

( ... The voice would also depend on this identity property, not stating that it has to be done in this patch.)

IIRC that is already the case?

binaries/data/mods/public/simulation/components/Identity.js
150

I suppose not everything that has an Identity also has a Gender (buildings?)

I found out that also buildings are male,,,

Instead of the "ugly default" it could error out if the template is broken.

Well the template is not broken when no gender is specified, since it is currently optional in the schema.

Why hardcode male and female to begin with? What about space alien stuff with random fictive genders?

Agreed, but without such a list, I cannot randomly choose between them right? Unless there is a way to check in the "Identity.js" which choices are allowed so that this function can choose between all of them (minus the "random")?

Freagarach updated this revision to Diff 8375.Sat, Jun 8, 8:39 AM
  • Moved random function to Init.
  • Hopefully serialised properly.
Vulcan added a comment.Sat, Jun 8, 8:46 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/Identity.js
| 114| »   »   "gender"·=·this.gender;
|    | [MAJOR] ESLintBear:
|    | Parsing error: Unexpected token =

binaries/data/mods/public/simulation/components/Identity.js
| 102| »   this.gender·=·!!this.template.Gender·==·"random"·?·randBool()·?·"male"·:·"female"·:·this.template.Gender·||·"male";·//·ugly·default
|    | [NORMAL] JSHintBear:
|    | Confusing use of '!'.

binaries/data/mods/public/simulation/components/Identity.js
| 114| »   »   "gender"·=·this.gender;
|    | [NORMAL] JSHintBear:
|    | Expected ':' and instead saw '='.

binaries/data/mods/public/simulation/components/Identity.js
| 114| »   »   "gender"·=·this.gender;
|    | [MAJOR] JSHintBear:
|    | Expected '}' to match '{' from line 113 and instead saw ';'.

binaries/data/mods/public/simulation/components/Identity.js
| 114| »   »   "gender"·=·this.gender;
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Identity.js
| 115| »   };
|    | [MAJOR] JSHintBear:
|    | Unrecoverable syntax error. (63% scanned).
Executing section cli...

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

Itms awarded a token.Sat, Jun 8, 9:25 AM
Freagarach updated this revision to Diff 8376.Sat, Jun 8, 12:20 PM
Freagarach marked an inline comment as done.
Freagarach retitled this revision from Feature request: Random gender in VisualActor. to Gender-tag in VisualActor..
Freagarach edited the summary of this revision. (Show Details)
  • FIxed typo's in serialise.
  • Local variable in "CCmpVisualActor.cpp".
Freagarach added inline comments.Sat, Jun 8, 12:22 PM
source/simulation2/components/CCmpVisualActor.cpp
211

Any advice on what to change then?

212

Any advice on what to change then?

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/Identity.js
| 102| »   this.gender·=·!!this.template.Gender·==·"random"·?·randBool()·?·"male"·:·"female"·:·this.template.Gender·||·"male";·//·ugly·default
|    | [NORMAL] JSHintBear:
|    | Confusing use of '!'.
Executing section cli...

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

wraitii requested changes to this revision.Sat, Jun 8, 1:11 PM
wraitii added a subscriber: wraitii.

I like the feature, haven't really looked at the implementation (except for inline comment), but this needs to be renamed. Beyond the fact that "gender" is more likely to lead to flame-wars, this should be usable for more things than genders, such as a unit that could be elvish or dwarfish in appearance for example, or variants of robot, or "child" vs "adult" for example.

I would suggest "phenotype", which has fewer connotations and is more generic.

binaries/data/mods/public/simulation/components/Identity.js
106

As elexis said, I think you should define possible genders in the template on top of allowing a specific one, so that we can have space alien stuff with random fictive genders? or even fantasy races.

This revision now requires changes to proceed.Sat, Jun 8, 1:11 PM
Stan added a comment.Sat, Jun 8, 1:41 PM

Gender is used in every template... That should be renamed in another patch

I would suggest "phenotype", which has fewer connotations and is more generic.

That is a good name!
Should we try to allow more than one? So that one can have an "elvish" "child" or will that be too much?

binaries/data/mods/public/simulation/components/Identity.js
106

Agreed, but without such a list, I cannot randomly choose between them right? Unless there is a way to check in the "Identity.js" which choices are allowed so that this function can choose between all of them (minus the "random")?

Any help on this would be appreciated :)

In D1955#81519, @Stan wrote:

Gender is used in every template... That should be renamed in another patch

?

Should we try to allow more than one? So that one can have an "elvish" "child" or will that be too much?

TBH I don't think that's necessary, just have an "Elvish child" phenotype. Our actor system isn't _that_ flexible.

binaries/data/mods/public/simulation/components/Identity.js
106

Change the schema to be something like

<Identity>
	<PossibleGenders datatype="tokens">male female</PossibleGenders>
	<Gender>random</Gender>
</Identity>

And then just use PossibleGenders as your list. You can then override it in children templates and all, which is convenient.

Stan added a comment.Sat, Jun 8, 1:51 PM

I meant if gender was to be replaced by phenotype.

In D1955#81528, @Stan wrote:

I meant if gender was to be replaced by phenotype.

Ah, I didn't know that the sound group used "gender" already.

I think it's better to go with phenotype first still and then rename stuff.

Stan added a comment.EditedSat, Jun 8, 2:06 PM

Fair enough :) But he will have to fix all the voices as well ?

Or will he have to add a get phentotype function that does the same thing as get gender until this is sorted out ?

Freagarach added a comment.EditedSat, Jun 8, 3:22 PM
In D1955#81537, @Stan wrote:

Fair enough :) But he will have to fix all the voices as well ?
Or will he have to add a get phentotype function that does the same thing as get gender until this is sorted out ?

It would seem logical to me to add a "phenotype"-function now and prepare the C-file for a "phenotype".
< Kindly ignoring the gender being imposed on me. >

Stan added a comment.Sat, Jun 8, 3:35 PM
In D1955#81537, @Stan wrote:

Fair enough :) But he will have to fix all the voices as well ?
Or will he have to add a get phentotype function that does the same thing as get gender until this is sorted out ?

It would seem logical to me to add a "phenotype"-function now and prepare the C-file for a "phenotype".
< Kindly ignoring the gender being imposed on me. >

Oh sorry :)

Freagarach added a comment.EditedSat, Jun 8, 3:46 PM
In D1955#81559, @Stan wrote:

Oh sorry :)

No problem, I just thought it was ironic in the light of part of the discussion ;)

Shouldn't we be making a "VisualActor.js" component instead?
That way, we might also be able to change the appearance of an entity in-game? (Cf. this question.)

Stan added a comment.Sat, Jun 8, 3:49 PM

You can already do so :) you need to Query the IID_Visual component :)

In D1955#81565, @Stan wrote:

You can already do so :) you need to Query the IID_Visual component :)

I know I can, but there is no(t yet a) function to change the VisualActor. So I thought I might be nicer to mimic Sound, in which the CCmp gets the sound from "Sound.js"-component. That way it would be easier to add different tags to the visualActor without having to change the C-code.

Stan added a comment.Sat, Jun 8, 4:20 PM

You could but currently we just delete the entity and create a new one for promotion. Usually you want to change the template as well

In D1955#81568, @Stan wrote:

You could but currently we just delete the entity and create a new one for promotion. Usually you want to change the template as well

Is that because of performance? Because the only thing why it is currently necessary to change the template is the visualActor ;)

Stan added a comment.Sat, Jun 8, 5:03 PM

The template stats are different :)

Freagarach added a comment.EditedSat, Jun 8, 5:29 PM
In D1955#81570, @Stan wrote:

The template stats are different :)

Nope ;) There are usually three things changed in the templates/units from "_b" to "_a": Rank, Promotion, VisualActor. I found no Attack/Armour upgrades. The actual stat change is tech-based.

Freagarach updated this revision to Diff 8386.Sat, Jun 8, 5:34 PM
Freagarach marked 3 inline comments as done.
Freagarach edited the summary of this revision. (Show Details)
  • Support for a list of possible genders.
  • Changed C-code to phenotype.

Saving and loading works fine.

Freagarach added inline comments.Sat, Jun 8, 5:48 PM
source/simulation2/components/CCmpVisualActor.cpp
212

Would you suggest using a "VisualActor.js" component in simulation then? (For this and above.)

Vulcan added a comment.Sat, Jun 8, 5:55 PM

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

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

Stan added a comment.Sat, Jun 8, 6:02 PM

Sorry I meant it could be done and it was actually done before. One could also change a unit into another one.

vladislavbelov added inline comments.Sat, Jun 8, 6:03 PM
source/simulation2/components/CCmpVisualActor.cpp
196

You don't need the variable for the whole scope of the function, put it in the usage scope.

Also we don't use m_ prefix for local variables.

211

I'd write it like:

const std::wstring pattern = L"{phenotype}";
auto patternPos = baseActorString .find(pattern);
if (patternPos != std::wstring::npos)
    baseActorString.replace(patternPos, pattern.length(), cmpIdentity->GetPhenotype());

Also I think that the name may content more than one pattern, then this code won't work. So:

#include <boost/algorithm/string.hpp>
// ...
boost::replace_all(baseActorString , "{phenotype}", cmpIdentity->GetPhenotype());
Freagarach marked an inline comment as not done.EditedSun, Jun 9, 12:18 PM
In D1955#81585, @Stan wrote:

Sorry I meant it could be done and it was actually done before. One could also change a unit into another one.

Yes, but that requires an extra template. When loading a game all templates get read by the AI, so with many templates it can take a long time to load (cf. this post). That seems unnecessary when the only thing you want to change is the VisualActor?

Back to this topic: When we would use a simulation component, wouldn't adding more tags (by mods or in vanilla) be a lot easier? Right now we are just hardcoding something in.
So a, perhaps ignorant, question: what is the advantage of using the C-cmp in this matter over a sim-cmp?
My ideas:

  • Advantages:
    • Faster (I don't know how much it differs though)
    • ...?
  • Disadvantages:
    • Less moddable.
    • ...?

I'm mainly asking this because I saw how it is done with Sounds.
I'm happy to be proven wrong if there are flaws in my line of thinking though!

Stan added a comment.Sun, Jun 9, 12:28 PM

The advantage of the C++ cmp is it can be used by the engine while if it's only js you can't :) Usually though it only requires a small subset of the code so it can be fine with a little subset of functions like it is the case here. I think the other function in the interface is not even needed.

It's likely not any faster. If the component was written completely in C++ it would be though. Usually there is like 6000 instructions executed in c++ instead of like 800 for js. So it's order of magnitude faster when the component is c++

I'm advocating for the XMl template to be put in different xml files (and not js or c++) and loaded during the game because multiple languages in the same file is an anti pattern. It would also allow people to see code examples without having to look in the source.

In any case though when tweaking schemas you need to tweak the code. In js though since nothing is private you could probably use something like

Cmpwhatehe.template.sometag without having to add more code in the component. But that's ugly and it should be discouraged.

In D1955#81644, @Stan wrote:

The advantage of the C++ cmp is it can be used by the engine while if it's only js you can't :) Usually though it only requires a small subset of the code so it can be fine with a little subset of functions like it is the case here. I think the other function in the interface is not even needed.

What I meant was to alter the C-cmp to look in a "VisualActor.js" sim-cmp rather than in "Identity.js" so that the replacing of the tag could be done in the former. It seems more logical to look for code related to a VisualActor in "VisualActor.js" and not in "Identity.js". The "VisualActor.js" can then use information from the Identity.js" to use and replace the tag.

In D1955#81644, @Stan wrote:

I'm advocating for the XMl template to be put in different xml files (and not js or c++) and loaded during the game because multiple languages in the same file is an anti pattern. It would also allow people to see code examples without having to look in the source.

Understandable.

In D1955#81644, @Stan wrote:

In any case though when tweaking schemas you need to tweak the code. In js though since nothing is private you could probably use something like
Cmpwhatehe.template.sometag without having to add more code in the component. But that's ugly and it should be discouraged.

But the tags do not need to be defined in a "VisualActor.js" sim-cmp, they can just be merged in it. The tags itself can then be obtained from different shema's (e.g. Identity, Health etc.). The thing the "VisualActor.js" would need access to is the "template.Actor" and that is the difficult/ugly thing then?

@Freagarach Are you planning to update the diff based on feedback so far?

Stan added a comment.Mon, Jun 10, 4:38 PM

What I meant was to alter the C-cmp to look in a "VisualActor.js" sim-cmp rather than in "Identity.js" so that the replacing of the tag could be done in the former. It seems more logical to look for code related to a VisualActor in "VisualActor.js" and not in "Identity.js". The "VisualActor.js" can then use information from the Identity.js" to use and replace the tag.

Well actually no, because the Gender is an identity thing, not a visual actor one :) That's what you do when you call the CmpPointer<Identity>. If it was a visualactor.js, you'd just move the issue :)

In D1955#81644, @Stan wrote:

But the tags do not need to be defined in a "VisualActor.js" sim-cmp, they can just be merged in it. The tags itself can then be obtained from different shema's (e.g. Identity, Health etc.). The thing the "VisualActor.js" would need access to is the "template.Actor" and that is the difficult/ugly thing then?

Well it would duplicated for once, since you'd do the same in CPP, the schema is in the cpp file.

Freagarach updated this revision to Diff 8416.Mon, Jun 10, 6:34 PM

Applied @vladislavbelov's comments.

Freagarach added a comment.EditedMon, Jun 10, 6:35 PM

@Freagarach Are you planning to update the diff based on feedback so far?

< While wondering what would happen if I were to say "no": > Yes of course :)
I was just waiting to where the discussion would lead.

In D1955#81804, @Stan wrote:

Well actually no, because the Gender is an identity thing, not a visual actor one :) That's what you do when you call the CmpPointer<Identity>. If it was a visualactor.js, you'd just move the issue :)

For the record I'll try to explain more elaborate what I meant:

  • The C-cmp gets the definitive "ActorName" from a "VisualActor.js" (if it were possible) just like the "SoundManager" gets the SoundGroup from "Sound.js" ("PlaySoundGroup").
  • In the "VisualActor.js" any tag(s) is(/are) combined to form the definitive "ActorName" (in this case the phenotype-tag will be set up and configured in "Identity-js").
  • "because the Gender is an identity thing, not a visual actor one" <- Excactly, that is why it is strange in my head to be coding it in the C-cmp xD
Stan added a comment.Mon, Jun 10, 6:38 PM

Well some people say no :) Some also never come back :)

Actually I never understood why the sounf was js. i guess that's because nobody bothered to write an interface.

There is no difference between js and c++ in the sense thzt you could do everything you do in js in c++ and vice versa.

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

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

Freagarach marked 4 inline comments as done.Mon, Jun 10, 7:41 PM
Freagarach retitled this revision from Gender-tag in VisualActor. to Support for gender-tag in VisualActor..Tue, Jun 11, 5:22 PM
Freagarach edited the summary of this revision. (Show Details)
Stan added inline comments.Tue, Jun 11, 6:11 PM
source/simulation2/components/CCmpVisualActor.cpp
211

I wonder if the "{phenotype}" string could not be a constant in the js component instead ?

212

What's the type ? We usually avoid auto not sure why @vladislavbelov suggested it there.

214

can't you replace "{phenotype}" by pattern ?

vladislavbelov added inline comments.Tue, Jun 11, 8:51 PM
source/simulation2/components/CCmpVisualActor.cpp
212

Actually we don't need the patternPos at all if we use boost::replace_all.

Freagarach updated this revision to Diff 8464.Fri, Jun 14, 7:40 PM
Freagarach retitled this revision from Support for gender-tag in VisualActor. to Support for phenotype-tag in VisualActor..
Freagarach edited the test plan for this revision. (Show Details)

Replaced "{phenotype}" by pattern in "boost".

Freagarach marked an inline comment as done.Fri, Jun 14, 7:41 PM
Freagarach added inline comments.
source/simulation2/components/CCmpVisualActor.cpp
212

If I don't use "if (patternPos != std::wstring::npos)" I get a segfault upon map creation.

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

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

wraitii added inline comments.Sat, Jun 15, 11:08 AM
binaries/data/mods/public/simulation/components/Identity.js
151

Seems weird to have both this and the below? Do you need to keep this for the sound for now?

Stan added inline comments.Sat, Jun 15, 11:09 AM
source/simulation2/components/CCmpVisualActor.cpp
212

Maybe it segfaults because cmpIdentity is not defined ? You do not check whether it's here :)

Stan added inline comments.Sat, Jun 15, 11:21 AM
binaries/data/mods/public/simulation/components/Identity.js
151

Yeah it's only called in binaries/data/mods/public/simulation/components/Sound.js

Freagarach updated this revision to Diff 8553.Wed, Jun 19, 7:59 AM
Freagarach marked 2 inline comments as done.
Freagarach edited the summary of this revision. (Show Details)
  • Updated "Sound.js".
  • All "gender" -> "phenotype".

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/1776/display/redirect