What the title says.
Details
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 Build Jenkins Build 12642: arc lint + arc unit
Event Timeline
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? ;) | |
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. |
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. |
binaries/data/mods/public/globalscripts/DamageTypes.js | ||
---|---|---|
22 | Don't translate here. |
Apparently it was double translated, for the GUI also translated them (if I checked it correctly).
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 ;) |
- Removed redundant containers.
- Removed subtypes, as they are deemed unnecessary.
- Removed translation from this file.
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>.)
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.
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.
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?
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.
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'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.)
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 ;) |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/1579/display/redirect
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 :)
Ah yes, I think I understand. This was not regarding the suggestion of @Nescio ?
I'm sorry, it is probably because it is late here, but what do you mean with extracting?
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 ;) )
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 ;)
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.)
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".
binaries/data/mods/public/simulation/components/tests/test_Attack.js | ||
---|---|---|
2 | Do you really need to copy paste this ? |
binaries/data/mods/public/simulation/components/tests/test_Attack.js | ||
---|---|---|
2 | Yes, otherwise the test fails. Unless there is some other option? |
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
binaries/data/mods/public/simulation/components/tests/test_Attack.js | ||
---|---|---|
2 | Do you have an error message ? It usually points out why it fails. |
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?
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).
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/.
"damages" might be better. I have no strong opinion on this, except that they should indeed match.
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.
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.
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: |
binaries/data/mods/public/globalscripts/DamageTypes.js | ||
---|---|---|
47 | string is actually valid jsdoc https://stackoverflow.com/questions/14611995/how-to-specify-an-array-of-objects-as-a-parameter-or-return-value-in-jsdoc |
Indeed.
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. |
If it could, than this diff would be not needed anymore. Keep us posted ;)
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".
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.
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.
binaries/data/mods/public/simulation/data/damagetypes/crush.json | ||
---|---|---|
1 | Underlining. |