Page MenuHomeWildfire Games

Load damageTypes from "simulation/data/damagetypes"-files.
AbandonedPublic

Authored by Freagarach on May 30 2019, 12:57 PM.

Details

Reviewers
bb
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#4801
Summary

What the title says.

Test Plan

Go through many user-cases to check that nothing is overlooked.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7765
Build 12643: Vulcan BuildJenkins
Build 12642: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/DamageTypes.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/DamageTypes.js
|  28|  28| 	// Sort arrays by specified order
|  29|  29| 	let typeSort = (a, b) =>
|  30|  30| 		a.order < b.order ? -1 :
|  31|    |-		a.order > b.order ? +1 : 0;
|    |  31|+			a.order > b.order ? +1 : 0;
|  32|  32| 
|  33|  33| 	this.damageTypeData.sort(typeSort);
|  34|  34| 	this.damageTypeCodes.sort((a, b) => typeSort(
Executing section cli...

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

Nescio added a subscriber: Nescio.May 31 2019, 7:40 PM

Why the subtypes? Damage types (crush, hack, pierce) are children of attack types (melee, ranged, slaughter), not the other way around.

Stan added a subscriber: Stan.May 31 2019, 7:42 PM
Stan added inline comments.
binaries/data/mods/public/globalscripts/DamageTypes.js
6–8

Do we need all those containers ?

10–14

I think you should cache that function call (let fileNames = functionResult) we can't assume the compiler is smart enough to inline this.

16–33

Why that condition ? Should we go on any way ?

32

I think you can just sort like this .sort(a => a.order)

Freagarach added a comment.EditedMay 31 2019, 7:51 PM
In D1936#80372, @Nescio wrote:

Why the subtypes? Damage types (crush, hack, pierce) are children of attack types (melee, ranged, slaughter), not the other way around.

I copied a large part from the "Resources.js" globalscript, and thought that maybe a subtype might be useful. The current are just placeholders. (Viz. "Magic" damage with subtypes "Fire", "Ice" etc.; or the other way around.) Personally I think it is not useful. See summary.

binaries/data/mods/public/globalscripts/DamageTypes.js
6–8

Nope :) But perhaps we do? ;)
I thought is was easier to remove redundant code that was already in, than to add code ;)

10–14

Okay, this is how the Resources-globalscript does it, but that is subideal?

16–33

Resources.js again. See first point in summary.

Stan added inline comments.May 31 2019, 7:59 PM
binaries/data/mods/public/globalscripts/DamageTypes.js
10–14

Why ? If for some reason in the future, that function takes 5 sec to execute, one will be happy it's not called too often :)

16–33

I have no final say in this, but I believe it should be unified somehow. so if lower case here, lower case everywhere. One could also check if the first char is capitalized and the rest lowercase. Dunno about attack with spaces in the name.

Perhaps it's just me, but I have a hard time trying to imagine what a damage subtype could be, conceptually. Resource subtypes I can understand: you gather fruit, fish, grain, meat at different rates and you get food. But damage types? If fire, ice, lightning magic all affect the same armour (magic), then they're effectively the same; and if they would have different armour types (fire, ice, lightning), then they would be effectively fully different damage types.
The trac ticket doesn't mention subtypes either. Wouldn't your code be much cleaner without?

For the translation, you need to focus only in the GUI part.
Look specially to common/tooltips.js

binaries/data/mods/public/globalscripts/DamageTypes.js
10–14

The class is cached in session and simulation already.

Polakrity added inline comments.May 31 2019, 8:54 PM
binaries/data/mods/public/globalscripts/DamageTypes.js
22

Don't translate here.

Freagarach marked 6 inline comments as done.EditedJun 1 2019, 9:05 AM

For the translation, you need to focus only in the GUI part.
Look specially to common/tooltips.js

Apparently it was double translated, for the GUI also translated them (if I checked it correctly).

In D1936#80390, @Nescio wrote:

Perhaps it's just me, but I have a hard time trying to imagine what a damage subtype could be, conceptually. Resource subtypes I can understand: you gather fruit, fish, grain, meat at different rates and you get food. But damage types? If fire, ice, lightning magic all affect the same armour (magic), then they're effectively the same; and if they would have different armour types (fire, ice, lightning), then they would be effectively fully different damage types.
The trac ticket doesn't mention subtypes either. Wouldn't your code be much cleaner without?

I certainly agree! If someone finds a use later on they can add it again.

binaries/data/mods/public/globalscripts/DamageTypes.js
6–8

Only the DataObj is unnecessary. Although the Data can be a local variable.

16–33

Agreed, but then every template should be changed so that the damage types use lower case. (Using lower case codes is consistent with other codes (e.g. resources, civs, etc.).)

32

Nope ;)

Freagarach updated this revision to Diff 8263.EditedJun 1 2019, 9:05 AM
Freagarach marked an inline comment as done.
Freagarach edited the summary of this revision. (Show Details)
  • Removed redundant containers.
  • Removed subtypes, as they are deemed unnecessary.
  • Removed translation from this file.
Vulcan added a comment.Jun 1 2019, 9:08 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/DamageTypes.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/globalscripts/DamageTypes.js
|  24|  24| 	// Sort arrays by specified order
|  25|  25| 	let typeSort = (a, b) =>
|  26|  26| 		a.order < b.order ? -1 :
|  27|    |-		a.order > b.order ? +1 : 0;
|    |  27|+			a.order > b.order ? +1 : 0;
|  28|  28| 
|  29|  29| 	damageTypeData.sort(typeSort);
|  30|  30| 	this.damageTypeCodes.sort((a, b) => typeSort(
Executing section cli...

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

You can remove descriptions if they are not really descriptive and used for the moment .
Maybe change the name of GetTypes() -> GetCodes() to reflect what it really returns and to be consistent with resources.

binaries/data/mods/public/globalscripts/DamageTypes.js
16–33

You must change this if you let Uppercase codes.

code is what is used in the templates and data files, and name is what is displayed by the GUI and marked for translation, right?

Currently we have (template_structure_civic_civil_centre.xml):

<Cost>
  <PopulationBonus>20</PopulationBonus>
  <BuildTime>500</BuildTime>
  <Resources>
    <food>0</food>
    <wood>500</wood>
    <stone>500</stone>
    <metal>500</metal>
  </Resources>
</Cost>

But also:

<Attack>
  <Ranged>
    <Hack>0.0</Hack>
    <Pierce>12.0</Pierce>
    <Crush>0.0</Crush>
    <MaxRange>72.0</MaxRange>
    <MinRange>0.0</MinRange>
    <PrepareTime>1200</PrepareTime>
    <RepeatTime>2000</RepeatTime>
    <Delay>0</Delay>
    <Projectile>
      <Speed>75.0</Speed>
      <Spread>1.5</Spread>
      <Gravity>9.81</Gravity>
      <LaunchPoint y="3"/>
    </Projectile>
    <PreferredClasses datatype="tokens">Human</PreferredClasses>
    <RangeOverlay>
      <LineTexture>outline_border.png</LineTexture>
      <LineTextureMask>outline_border_mask.png</LineTextureMask>
      <LineThickness>0.175</LineThickness>
    </RangeOverlay>
  </Ranged>
</Attack>

So perhaps than should be changed to the following, for consistency?

  <Attack>
    <Ranged>
      <Damage>
        <hack>0.0</hack>
        <pierce>12.0</pierce>
        <crush>0.0</crush>
      </Damage>
      <MaxRange>72.0</MaxRange>
      ...
  </Ranged>
</Attack>

I'm not sure it's within the scope of this patch, though. (Also, <Armour> and <Loot>.)

Stan added a comment.Jun 1 2019, 3:36 PM

But then it would not be consistent with the syntax in the rest of the templates, which would lead to utter confusion I guess. Also yeah I think that's out of the scope :)

@Freagarach what about capture ? I think this will be the hardest to extract.

Nescio added a comment.EditedJun 1 2019, 5:36 PM

In principle I think D1936 and D1938 are a welcome improvement; I was merely wondering whether or not it would make sense to wrap the damage types inside a <Damage> node and yes, thus change all relevant templates; cf. D1171.
As for capture attack, it has a Value (which affects Capture Points), not damage types (which affect Armour and Health). I don't think Value should become a damage type; for comparison, time and population are resources that are treated differently from the others.

You can remove descriptions if they are not really descriptive and used for the moment .
Maybe change the name of GetTypes() -> GetCodes() to reflect what it really returns and to be consistent with resources.

I'll try to add some interesting descriptions.
I wanted to change that, but that means changing code on a lot of occasions, is that in scope here?

In D1936#80491, @Nescio wrote:

code is what is used in the templates and data files, and name is what is displayed by the GUI and marked for translation, right?

Aye.

So perhaps than should be changed to the following, for consistency?

  <Attack>
    <Ranged>
      <Damage>
        <hack>0.0</hack>
        <pierce>12.0</pierce>
        <crush>0.0</crush>
      </Damage>
      <MaxRange>72.0</MaxRange>
      ...
  </Ranged>
</Attack>

I'm not sure it's within the scope of this patch, though. (Also, <Armour> and <Loot>.)

Definitely out of scope but should have, IMHO.

In D1936#80505, @Stan wrote:

But then it would not be consistent with the syntax in the rest of the templates, which would lead to utter confusion I guess. Also yeah I think that's out of the scope :)

Why would it be not consistent with the syntax in the rest of templates? I think it would make it *more* consistent. (E.g. a unit costs "Resources", what resources? "food" and "wood"; a unit does "Damage", what damage? "crush" and "pierce".)

In D1936#80505, @Stan wrote:

@Freagarach what about capture ? I think this will be the hardest to extract.

I'm sorry, I do not understand it fully, what about capture? Do you mean in the scope of the suggestion of @Nescio ? In that case: nay, it is not that hard, it is a special attack and has mostly different code, as should "Slaughter" have IMHO, but that is another topic ;) If you do not mean in the scope of that suggestion, would you like to elaborate on what you mean, please?

(Sidenote: If we were to rewrite the "Attack" I would, with an eye on #252, suggest:

  <Attack>
    <anyName>
      <Type>Ranged</Type>
      <Damage>
        <hack>0.0</hack>
        <pierce>12.0</pierce>
        <crush>0.0</crush>
      </Damage>
      <MaxRange>72.0</MaxRange>
      ...
  </anyName>
</Attack>

for maximum moddability.)

Freagarach updated this revision to Diff 8273.Jun 1 2019, 6:12 PM
Freagarach marked an inline comment as done.
Freagarach edited the summary of this revision. (Show Details)
  • Allow uppercase damagetypes. (For now.)
  • Added descriptions of damageTypes.
Freagarach added a subscriber: Restricted Owners Package.Jun 1 2019, 6:12 PM
Freagarach added inline comments.
binaries/data/mods/public/globalscripts/DamageTypes.js
16–33

Thanks, I was waiting the discussion regarding Upper/lowercase. I think it is agreed that that it is currently out of scope to change everything to lowercase, so I'll remove this code. We shouldn't forget adding it back again if/when rewriting to lowercase ;)

Vulcan added a comment.Jun 1 2019, 6:15 PM

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

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

Freagarach marked 2 inline comments as done.Jun 1 2019, 6:19 PM
Stan added a comment.Jun 1 2019, 10:09 PM

Why would it be not consistent with the syntax in the rest of templates? I think it would make it *more* consistent. (E.g. a unit costs "Resources", what resources? "food" and "wood"; a unit does "Damage", what damage? "crush" and "pierce".)

I meant that all the parameters in the template files are starting with a capital. So it would be weird not to have them.

I'm sorry, I do not understand it fully, what about capture? Do you mean in the scope of the suggestion of @Nescio ? In that case: nay, it is not that hard, it is a special attack and has mostly different code, as should "Slaughter" have IMHO, but that is another topic ;) If you do not mean in the scope of that suggestion, would you like to elaborate on what you mean, please?

I mean capture is a type of attack that's hardcoded in a few places. but I guess it will be harder to extract it :)

In D1936#80548, @Stan wrote:

I meant that all the parameters in the template files are starting with a capital. So it would be weird not to have them.

Ah yes, I think I understand. This was not regarding the suggestion of @Nescio ?

In D1936#80548, @Stan wrote:

I mean capture is a type of attack that's hardcoded in a few places. but I guess it will be harder to extract it :)

I'm sorry, it is probably because it is late here, but what do you mean with extracting?

Stan added a comment.Jun 1 2019, 10:25 PM

I mean finding out which file use it so you can make it external :)

In D1936#80556, @Stan wrote:

I mean finding out which file use it so you can make it external :)

It seems that a good night's sleep does some miracles :) (It might use "Capture" as a special "anyName" so that no code needs to be changed for that, but I'll see.)
I think we "polute" this diff too much with our discussion, perhaps someone good with words can create a trac-ticket for it so we can discuss it further? (And I have some justification to be working on the coding ;) )

wraitii added reviewers: bb, wraitii, Restricted Owners Package.Jun 2 2019, 6:35 PM
Stan added a comment.Jun 2 2019, 6:45 PM

You can create a trac ticket if you want :) but it's okay to have long discussions here as well.
IMHO Capture should be a special type of attack with an affects property that says whether it's IID_Health or IID_something that gets affected ;)

In D1936#80631, @Stan wrote:

You can create a trac ticket if you want :) but it's okay to have long discussions here as well.

Okay, in the light of the latter, is there a "simple" way of editing (nigh) all templates? For I would like to test my current code of grouping the damage types under "Damage", but "sed" is not able to search and replace newlines :( (Or I just don't know how to do it yet.)

Angen added a subscriber: Angen.Jun 2 2019, 8:45 PM

please fix unit tests

Freagarach added a comment.EditedJun 3 2019, 5:24 PM
In D1936#80638, @Angen wrote:

please fix unit tests

Yeah, I was waiting for the code to be generally okayish, so I didn't need to mess with the tests too often, I'll try to work on the tests Wednesday probably.
EDIT: The template-changing succeeded, I used "perl".

Freagarach updated this revision to Diff 8305.Jun 4 2019, 5:28 PM
Freagarach edited the summary of this revision. (Show Details)
Freagarach edited the test plan for this revision. (Show Details)

Fixed unit tests.

Stan added inline comments.Jun 4 2019, 5:32 PM
binaries/data/mods/public/simulation/components/tests/test_Attack.js
2

Do you really need to copy paste this ?

Freagarach added inline comments.Jun 4 2019, 5:41 PM
binaries/data/mods/public/simulation/components/tests/test_Attack.js
2

Yes, otherwise the test fails. Unless there is some other option?

Vulcan added a comment.Jun 4 2019, 5:57 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'Melee'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|  56|  56| 	});
|  57|  57| 
|  58|  58| 	let cmpAttack = ConstructComponent(attacker, "Attack", {
|  59|    |-		"Melee" : {
|    |  59|+		"Melee": {
|  60|  60| 			"Hack": 11,
|  61|  61| 			"Pierce": 5,
|  62|  62| 			"Crush": 0,
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'Ranged'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|  76|  76| 				}
|  77|  77| 			}
|  78|  78| 		},
|  79|    |-		"Ranged" : {
|    |  79|+		"Ranged": {
|  80|  80| 			"Hack": 0,
|  81|  81| 			"Pierce": 10,
|  82|  82| 			"Crush": 0,
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'Splash'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|  95|  95| 			"RestrictedClasses": {
|  96|  96| 				"_string": "Elephant"
|  97|  97| 			},
|  98|    |-			"Splash" : {
|    |  98|+			"Splash": {
|  99|  99| 				"Shape": "Circular",
| 100| 100| 				"Range": 10,
| 101| 101| 				"FriendlyFire": "false",
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'Capture'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
| 110| 110| 				}
| 111| 111| 			}
| 112| 112| 		},
| 113|    |-		"Capture" : {
|    | 113|+		"Capture": {
| 114| 114| 			"Value": 8,
| 115| 115| 			"MaxRange": 10,
| 116| 116| 		},
|    | [NORMAL] ESLintBear (comma-spacing):
|    | There should be no space before ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
| 141| 141| }
| 142| 142| 
| 143| 143| // Validate template getter functions
| 144|    |-attackComponentTest(undefined, true ,(attacker, cmpAttack, defender) => {
|    | 144|+attackComponentTest(undefined, true,(attacker, cmpAttack, defender) => {
| 145| 145| 
| 146| 146| 	TS_ASSERT_UNEVAL_EQUALS(cmpAttack.GetAttackTypes(), ["Melee", "Ranged", "Capture"]);
| 147| 147| 	TS_ASSERT_UNEVAL_EQUALS(cmpAttack.GetAttackTypes([]), ["Melee", "Ranged", "Capture"]);
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
| 141| 141| }
| 142| 142| 
| 143| 143| // Validate template getter functions
| 144|    |-attackComponentTest(undefined, true ,(attacker, cmpAttack, defender) => {
|    | 144|+attackComponentTest(undefined, true , (attacker, cmpAttack, defender) => {
| 145| 145| 
| 146| 146| 	TS_ASSERT_UNEVAL_EQUALS(cmpAttack.GetAttackTypes(), ["Melee", "Ranged", "Capture"]);
| 147| 147| 	TS_ASSERT_UNEVAL_EQUALS(cmpAttack.GetAttackTypes([]), ["Melee", "Ranged", "Capture"]);
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Attack.js
| 164| 164| 		"Pierce": 10,
| 165| 165| 		"Crush": 0
| 166| 166| 	});
| 167|    |-	
|    | 167|+
| 168| 168| 	TS_ASSERT_UNEVAL_EQUALS(cmpAttack.GetAttackStrengths("Ranged.Splash"), {
| 169| 169| 		"Hack": 0.0,
| 170| 170| 		"Pierce": 15.0,
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'turnLength' found.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|  38|  38| 
|  39|  39| 	let cmpDamage = ConstructComponent(SYSTEM_ENTITY, "Damage");
|  40|  40| 	let cmpTimer = ConstructComponent(SYSTEM_ENTITY, "Timer");
|  41|    |-	cmpTimer.OnUpdate({ turnLength: 1 });
|    |  41|+	cmpTimer.OnUpdate({ "turnLength": 1 });
|  42|  42| 	let attacker = 11;
|  43|  43| 	let atkPlayerEntity = 1;
|  44|  44| 	let attackerOwner = 6;
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|  76|  76| 		"position": targetPos,
|  77|  77| 		"isSplash": false,
|  78|  78| 		"projectileId": 9,
|  79|    |-		"direction": new Vector3D(1,0,0)
|    |  79|+		"direction": new Vector3D(1, 0,0)
|  80|  80| 	};
|  81|  81| 
|  82|  82| 	AddMock(atkPlayerEntity, IID_Player, {
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|  76|  76| 		"position": targetPos,
|  77|  77| 		"isSplash": false,
|  78|  78| 		"projectileId": 9,
|  79|    |-		"direction": new Vector3D(1,0,0)
|    |  79|+		"direction": new Vector3D(1,0, 0)
|  80|  80| 	};
|  81|  81| 
|  82|  82| 	AddMock(atkPlayerEntity, IID_Player, {
|    | [NORMAL] ESLintBear (quote-props):
|    | Unquoted property 'turnLength' found.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 127| 127| 
| 128| 128| 	function TestDamage()
| 129| 129| 	{
| 130|    |-		cmpTimer.OnUpdate({ turnLength: 1 });
|    | 130|+		cmpTimer.OnUpdate({ "turnLength": 1 });
| 131| 131| 		TS_ASSERT(damageTaken);
| 132| 132| 		damageTaken = false;
| 133| 133| 	}
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'hack'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 169| 169| 		"origin": origin,
| 170| 170| 		"radius": 10,
| 171| 171| 		"shape": "Linear",
| 172|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 172|+		"strengths": { "hack": 100, "pierce" : 0, "crush": 0 },
| 173| 173| 		"direction": new Vector3D(1, 747, 0),
| 174| 174| 		"playersToDamage": [2],
| 175| 175| 		"type": "Ranged",
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'pierce'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 169| 169| 		"origin": origin,
| 170| 170| 		"radius": 10,
| 171| 171| 		"shape": "Linear",
| 172|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 172|+		"strengths": { "hack" : 100, "pierce": 0, "crush": 0 },
| 173| 173| 		"direction": new Vector3D(1, 747, 0),
| 174| 174| 		"playersToDamage": [2],
| 175| 175| 		"type": "Ranged",
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 176| 176| 		"attackerOwner": attackerOwner
| 177| 177| 	};
| 178| 178| 
| 179|    |-	let fallOff = function(x,y)
|    | 179|+	let fallOff = function(x, y)
| 180| 180| 	{
| 181| 181| 		return (1 - x * x / (data.radius * data.radius)) * (1 - 25 * y * y / (data.radius * data.radius));
| 182| 182| 	};
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'hack'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 328| 328| 		"origin": new Vector2D(3, 4),
| 329| 329| 		"radius": radius,
| 330| 330| 		"shape": "Circular",
| 331|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 331|+		"strengths": { "hack": 100, "pierce" : 0, "crush": 0 },
| 332| 332| 		"playersToDamage": [2],
| 333| 333| 		"type": "Ranged",
| 334| 334| 		"attackerOwner": 1
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'pierce'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 328| 328| 		"origin": new Vector2D(3, 4),
| 329| 329| 		"radius": radius,
| 330| 330| 		"shape": "Circular",
| 331|    |-		"strengths": { "hack" : 100, "pierce" : 0, "crush": 0 },
|    | 331|+		"strengths": { "hack" : 100, "pierce": 0, "crush": 0 },
| 332| 332| 		"playersToDamage": [2],
| 333| 333| 		"type": "Ranged",
| 334| 334| 		"attackerOwner": 1

binaries/data/mods/public/simulation/components/tests/test_Damage.js
| 138| »   type·=·data.type·=·"Ranged";
|    | [NORMAL] ESLintBear (no-multi-assign):
|    | Unexpected chained assignment.
Executing section cli...

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

Stan added inline comments.Jun 4 2019, 6:02 PM
binaries/data/mods/public/simulation/components/tests/test_Attack.js
2

Do you have an error message ? It usually points out why it fails.

Freagarach added inline comments.Jun 4 2019, 6:07 PM
binaries/data/mods/public/simulation/components/tests/test_Attack.js
2

With the unedited test:

In TestComponentScripts::test_scripts:
/home/,,,/0ad/source/simulation2/components/tests/test_scripts.h:56: Error: Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/helpers") / pathname)
ERROR: JavaScript error: globalscripts/DamageTypes.js line 10
TypeError: Engine.ListDirectoryFiles is not a function
  DamageTypes@globalscripts/DamageTypes.js:10:23
  @simulation/helpers/DamageTypes.js:8:15
  @simulation/components/tests/test_Attack.js:2:1
/home/,,,/0ad/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Attack.js"
/home/,,,/0ad/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)

With the "BuildSchema" omitted:

In TestComponentScripts::test_scripts:
/home/,,,/0ad/source/simulation2/components/tests/test_scripts.h:50: Error: Assertion failed: componentManager->LoadScript(VfsPath(L"simulation/components") / pathname)
ERROR: JavaScript error: simulation/components/Attack.js line 125
TypeError: DamageTypes.BuildSchema is not a function
  @simulation/components/Attack.js:125:1
  @simulation/components/tests/test_Attack.js:14:1
/home/,,,/0ad/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Attack.js"
/home/,,,/0ad/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)

Overall this looks reasonable to me. The tests failing is weird though, they should work by loading the global scripts.
@bb Your opinion on this would be appreciated.

I'm wondering if we really need to specify damage in JSON files though. For one thing these only concerns attack damage (status effects for example support arbitrary damage types, not only damage types specified in the attack types). As things stand, the JSON spec just seems to add needless data and could be abstracted away (templates would just specify whatever damage types they want).

So to me the main question is really "do we need to soft-code damage types anywhere"? Can we get away with just art and templates specification?

I'm wondering if we really need to specify damage in JSON files though. For one thing these only concerns attack damage (status effects for example support arbitrary damage types, not only damage types specified in the attack types). As things stand, the JSON spec just seems to add needless data and could be abstracted away (templates would just specify whatever damage types they want).
So to me the main question is really "do we need to soft-code damage types anywhere"? Can we get away with just art and templates specification?

It seems that, as of the latest discussion in D1938, specifying damage in JSON files could very much be beneficial because the AI also has the damage types hard coded within (including an "importance factor", which can then be read from the files).

Overall this looks reasonable to me. The tests failing is weird though, they should work by loading the global scripts.

Thank you!
Well it seems logical that a test cannot call the Engine? Hence the "setup.js" in "tests" I guess. Although I am not sure whether the "Engine.ListDirectoryFiles" can be emulated correctly?

Shouldn't the folder be called simply damage/, to match the <Damage> node of D1950? For comparison, we have resources/, not `resourcetypes/.

In D1936#80943, @Nescio wrote:

Shouldn't the folder be called simply damage/, to match the <Damage> node of D1950? For comparison, we have resources/, not `resourcetypes/.

"damages" might be better. I have no strong opinion on this, except that they should indeed match.

I'm wondering if we really need to specify damage in JSON files though. For one thing these only concerns attack damage (status effects for example support arbitrary damage types, not only damage types specified in the attack types). As things stand, the JSON spec just seems to add needless data and could be abstracted away (templates would just specify whatever damage types they want).
So to me the main question is really "do we need to soft-code damage types anywhere"? Can we get away with just art and templates specification?

It seems that, as of the latest discussion in D1938, specifying damage in JSON files could very much be beneficial because the AI also has the damage types hard coded within (including an "importance factor", which can then be read from the files).

Mh, yeah, I was wondering where we would need to refer to all possibly damage types. I guess the AI is a possibility. I need to dig deeper, it could just be we can abstract that away in the AI.

Overall this looks reasonable to me. The tests failing is weird though, they should work by loading the global scripts.

Thank you!
Well it seems logical that a test cannot call the Engine? Hence the "setup.js" in "tests" I guess. Although I am not sure whether the "Engine.ListDirectoryFiles" can be emulated correctly?

Hm, we do load some real stuffs in the tests. Will check again.

bb added a comment.Jun 5 2019, 8:34 PM

So to me the main question is really "do we need to soft-code damage types anywhere"? Can we get away with just art and templates specification?

It seems this question is equivalent to asking "do we need a list of all possible damageTypes somewhere in the code?" Retrieving the list from looking at all templates doesn't look like worth it to me. For the sim, the answer seems to be "no", since a type only the present types in an attack schema actually do something, and a nonexisting in a damageReciever can be defaulted to 0. However for the gui consider the case that 1 unit attacks with an unique damageType, than no unit will have that type present in the Armour tooltip, sure the effective value is 0, but shouldn't we communicate that to the player? Considering this I would argue for the need of such a list and thus a softcodation.

binaries/data/mods/public/globalscripts/DamageTypes.js
23–24

(a,b) => a.order - b.order ??

26

The sorting doesn't seem used anywhere (the lines below ignore the order as there shouldn't be duplicate codes anyway, these will bug above...)

26–30

What about this.damageTypeCodes = damageTypeData.sort((a,b) => a.order - b.order).map(damageType => damageType.code); ? and with that remove the assignment in the loop above

38

don't use a tab, just a space and period on the end

47

Array is the returned type, also space instead of tab and period, also no space after [ or before ]

binaries/data/mods/public/simulation/components/tests/test_Attack.js
2

From ComponentManager.cpp:

	// For component script tests, the test system sets up its own scripted implementation of
	// these functions, so we skip registering them here in those cases

In the if block below JSI_VFS::RegisterScriptFunctions_Simulation(m_ScriptInterface); is not called when running the tests, so the tests don't have access to the registered functions (ListDirectoryFiles, ReadFromJSON and FileExists). I don't see a good reason why these functions aren't registered for the tests too, refs rP20576. Moving the call outside the if-block make the test run further (test still error out, but now on real patch related stuff)

binaries/data/mods/public/simulation/data/damagetypes/crush.json
1

These files need to be added to the translation files: binaries/data/mods/public/l10/messages.json

2

with that being all lowercase, I suspect about every template needing a change... see the error messages when starting a game...

4

Those description might be the hardest to get right here, maybe:
"Damage caused by sheer force, mostly effective versus buildings"

Stan added inline comments.Jun 5 2019, 8:37 PM
binaries/data/mods/public/globalscripts/DamageTypes.js
47
wraitii added a comment.EditedJun 5 2019, 9:06 PM
In D1936#81064, @bb wrote:

It seems this question is equivalent to asking "do we need a list of all possible damageTypes somewhere in the code?"

Indeed.

In D1936#81064, @bb wrote:

However for the gui consider the case that 1 unit attacks with an unique damageType, than no unit will have that type present in the Armour tooltip, sure the effective value is 0, but shouldn't we communicate that to the player? Considering this I would argue for the need of such a list and thus a softcodation.

Hm, counterpoint: if we have 10 different damage types this becomes completely overkill. I don't think it's worth special-casing that armour behaviour, we could just state "Armour against other damage is 0" in the GUI.

binaries/data/mods/public/simulation/data/damagetypes/crush.json
4

I would go with "sheer blunt force" perhaps.

Mh, yeah, I was wondering where we would need to refer to all possibly damage types. I guess the AI is a possibility. I need to dig deeper, it could just be we can abstract that away in the AI.

If it could, than this diff would be not needed anymore. Keep us posted ;)

In D1936#81064, @bb wrote:

However for the gui consider the case that 1 unit attacks with an unique damageType, than no unit will have that type present in the Armour tooltip, sure the effective value is 0, but shouldn't we communicate that to the player? Considering this I would argue for the need of such a list and thus a softcodation.

Hm, counterpoint: if we have 10 different damage types this becomes completely overkill. I don't think it's worth special-casing that armour behaviour, we could just state "Armour against other damage is 0" in the GUI.

Well, we also do not explicitly mention that a unit which has no crush damage has a crush damage of "0" do we? (When we do have 10 different damage types and some unit has armour for all of them the UI will be cluttered anyways, so a different way of showing them is probably needed then.)

I guess that the only need for a damage type list would be AI, because they have an "importance factor".

Freagarach added inline comments.Jun 6 2019, 3:58 PM
binaries/data/mods/public/simulation/data/damagetypes/crush.json
2

Hmm, I don't know how this came in. I used Uppercase in all previous diffs.

I guess that the only need for a damage type list would be AI, because they have an "importance factor".

Yes, on that topic I'm not sure it's a good idea to tie that in the JSON data, because JSON isn't easy to expand across mods (if two mods conflict, it's pretty much game over)... I think maybe we should put the AI preferences directly in the AI config files instead.

Anyways I'll dig deeper on this.

I guess that the only need for a damage type list would be AI, because they have an "importance factor".

Yes, on that topic I'm not sure it's a good idea to tie that in the JSON data, because JSON isn't easy to expand across mods (if two mods conflict, it's pretty much game over)... I think maybe we should put the AI preferences directly in the AI config files instead.
Anyways I'll dig deeper on this.

Agreed:

  • Partially fixed AI. If a new damage type is introduced, one still has to change "entityExtend.js" to teach the AI that that damage type is present.

Do the damage strength values need to be coded in the damages files or in the Petra-specific config.js? I would say the latter actually.

Given that in D1938 we're going to make most code damage-type agnostic, I don't think we really need a list anywhere, so I don't think the JSON files are necessary. They could still be useful to provide "default" values for additional metadata, but I don't think that's such a stringent requirement atm.

IMO the global script should be removed following D1938, and the rest with it.

Freagarach planned changes to this revision.Jun 13 2019, 5:32 PM

Given that in D1938 we're going to make most code damage-type agnostic, I don't think we really need a list anywhere, so I don't think the JSON files are necessary. They could still be useful to provide "default" values for additional metadata, but I don't think that's such a stringent requirement atm.
IMO the global script should be removed following D1938, and the rest with it.

I'll abandon when D1938 has landed and has succeeded in making things agnostic. I'll plan changes so it will not be lingering in your review queue.

Freagarach abandoned this revision.Jun 19 2019, 7:37 AM

Due to the state of D1938, this has become obselete :)