Details
- Reviewers
elexis - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22797: Move other/wallset_palisade to structures/wallset_palisade, refs #4770, D1002…
Check it breaks nothing.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... binaries/data/mods/public/maps/random/survivalofthefittest_triggers.js | 275| » for·(let·point·of·triggerPoints) | | [NORMAL] ESLintBear (no-shadow): | | 'point' is already declared in the upper scope. binaries/data/mods/public/maps/random/survivalofthefittest_triggers.js | 275| » for·(let·point·of·triggerPoints) | | [NORMAL] ESLintBear (no-shadow): | | 'point' is already declared in the upper scope. Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1381/display/redirect
As mentioned probably in other revision proposals already, this move is good, we want to remove the folders with miscellaneous files, such as other/ (or special/ which we burnt our fingers on with territory_pull), or the trigger snow wolf, treasure moves, ...
(For reference, review done upon PM request, and the PM request done because I recommended to ask for reviewers :P)
I will also fix the ESLint warning, since Jenkins asked so kindly, and Krinkle would probably show up with a patch otherwise.
I recreated the patch actually with an svn mv command and a replace of the pathname in *xml, *js, *json, then compared our results.
The raw patch downloaded from Phabricator actually doesn't include the target file of the move, so I'm glad I did it this way.
The rest of our changes are identical.
I suppose those other/palisades_*.xml files should be moved too, but you didn't do it in this patch to make it easier to get the individual diffs committed, in which case I would agree with that.
In particular someone might wonder whether it wouldn't be better to have one palsiade set per civ (possibly inheriting the common one to minimize duplication) to make it more consistent with the stone walls.
Tested the patch by starting survival and seeing treasures, and starting the game and building palisades without nuclear fallout.
Thanks for the patch Nescio, and sorry that we have so limited capacity.
Thanks for reviewing and committing, I appreciate it!
I recreated the patch actually with an svn mv command and a replace of the pathname in *xml, *js, *json, then compared our results.
The raw patch downloaded from Phabricator actually doesn't include the target file of the move, so I'm glad I did it this way.
The rest of our changes are identical.
So what I did was individually svn mv the files, then update the files that used them, and finally ran arc diff --preview to upload the patch. Is there something I should have done differently?
I suppose those other/palisades_*.xml files should be moved too, but you didn't do it in this patch to make it easier to get the individual diffs committed, in which case I would agree with that.
Yeah, D1796 simplified the palisades' inheritence, this one moved the wallset template, the next one will move the palisade templates to structures/ as well. Afterwards another will move the remaining structures from other/ to structures/.
In particular someone might wonder whether it wouldn't be better to have one palsiade set per civ (possibly inheriting the common one to minimize duplication) to make it more consistent with the stone walls.
That seems rather unnecessary: all palisades of all factions are statistically identical and use the same actors, so there is no need to add palisade wallset templates for each civ. In contrast, that each civ has its own outpost template is because their actors have different garrison flags.
Is there something I should have done differently?
Probably not, mostly I don't trust this thing with moved files. Supposedly it applies well depending on the command to apply the patch.
That seems rather unnecessary: all palisades of all factions are statistically identical and use the same actors, so there is no need to add palisade wallset templates for each civ. In contrast, that each civ has its own outpost template is because their actors have different garrison flags.
Unnecessary now yes.
But it would provide room to specify different templates for different civs.
Though if noone works on that indeed it's of limited (as in close to nil) use.
The filenames would be a bit more consistent I guess :P
Probably not, mostly I don't trust this thing with moved files. Supposedly it applies well depending on the command to apply the patch.
Thanks for the clarification; I don't really understand phabricator's inner workings.
Unnecessary now yes.
But it would provide room to specify different templates for different civs.
Though if noone works on that indeed it's of limited (as in close to nil) use.
The filenames would be a bit more consistent I guess :P
To me it seems to be just file inflation. If and when someone would create civ-specific palisades, they could add a simulation template then, but right now I fail to see the benefit of adding thirteen identical files.