Page MenuHomeWildfire Games

Grouped civ bonuses into a single folder
ClosedPublic

Authored by Nescio on Apr 26 2018, 12:39 PM.

Details

Reviewers
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22083: Group civ bonuses into a single folder
Summary

Currently:

simulation/data/civs/ contains the descriptions of all civilizations
simulation/data/auras/teambonuses/ contains the team bonuses of all civilizations
simulation/data/auras/units/catafalques/ contains the catafalque bonuses of all civilizations
simulation/data/auras/units/heroes/ contains the hero bonuses of all civilizations

However, civilization bonuses are distributed somewhat arbitrarily into separate carthaginians/, celts/, hellenes/, mauryans/, and persians/ folders

This patch groups all existing civilization bonus files into a single folder: simulation/data/technologies/civbonuses/
Also implemented a more uniform naming scheme. These changes should make things easier to maintain and allow for future additions.

Test Plan

Should be unproblematic.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5975
Build 9967: Vulcan BuildJenkins
Build 9966: arc lint + arc unit

Event Timeline

Nescio created this revision.Apr 26 2018, 12:39 PM
Vulcan added a subscriber: Vulcan.Apr 26 2018, 12:43 PM

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

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

wraitii added a reviewer: Restricted Owners Package.May 14 2018, 11:55 AM

I like this, how do others feel?

Stan requested changes to this revision.Jan 5 2019, 4:52 PM
Stan added a subscriber: Stan.

@wraitii sounds nice, though probably needs a rebase.

This revision now requires changes to proceed.Jan 5 2019, 4:52 PM
Nescio updated this revision to Diff 7279.Jan 5 2019, 8:39 PM

Updated.

Nescio added a comment.Jan 5 2019, 8:42 PM

For the record, arc diff --preview gave the following error:

Linting...
 Exception 
Command failed with error #1!
COMMAND
svn propget 'svn:mime-type' '/home/b/0ad/binaries/data/mods/public/simulation/data/technologies/civbonuses/cart_walls.json'@

STDOUT
(empty)

STDERR
svn: warning: W200017: Property 'svn:mime-type' not found on '/home/b/0ad/binaries/data/mods/public/simulation/data/technologies/civbonuses/cart_walls.json@'
svn: E200000: A problem occurred; see other errors for details

(Run with `--trace` for a full exception trace.)

therefore I used I used svn propset svn:mime-type text/plain *.json again.

Vulcan added a comment.Jan 5 2019, 8:46 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/927/

Anyone of @s0600204 @wraitii @bb has a strong opinion against that ?
I will break delenda_est and probably terra_magna and milleniumad.

Still like this as I think we should put all civ-specific techs in the civ folders.

elexis added a subscriber: elexis.Jan 24 2019, 2:21 AM
In D1472#70810, @Stan wrote:

will break delenda_est and probably terra_magna and milleniumad.

Perhaps you can let the author fix that if/after the patch was committed. This can be done by a pull request too without commit access.

I certainly agree with having civbonuses in a directory separate from the other techs, so that one can view and compare all civ bonuses without doing custom fileselection and filtering. (Dunno about civ folders right now.)

Stan accepted this revision.Jan 24 2019, 8:31 AM

I'll commit it when I have some time. and update the mods accordingly and make a PR on de. Thanks for the input.

This revision is now accepted and ready to land.Jan 24 2019, 8:31 AM
In D1472#61121, @Nescio wrote:

iirc I initialized that topic in the forum. So I permit myself to put my input here.
I think that a change is needed when there is a problem. Here the main problem was the issue with "multilabel" (sword tech for Mauryas and Iberians) which broke the actual schema.
But the issue on the way to solve it is:

  • do we (you) create a folder for civbonuses aka technologies specific to one (or more - but not too many?) civ.
  • do we (you) create a folder for civbonuses aka technologies specific to one (or more - but not too many?) civ and autoresearched.

Grouping autoresearched techs into a separate folder (and not introducing civ specific folders) would allow someone to look at all autoresearched techs without doing a filecontent search, but by opening that folder. So that's a small use case for adding that directory if not introducing civ folders.
If introducing civ folders, we'd have 12 to 13 new directory that contain only one or two autoresearched techs? That doesn't sound like a balanced filetree, so in that case the directory could be argued to be avoided.
There are techs that apply to multiple civs, which would break the sorting pattern when using civ folders. One could copy the tech per civ, but that would imply the redundancy problems if they are meant to be logically the same tech.
(So I guess one could argue multiple ways.)

It's much easier for modders if all civ-bonuses for all factions are simply grouped together in a single directory, hence this proposal.

In D1472#71391, @Nescio wrote:

It's much easier for modders if all civ-bonuses for all factions are simply grouped together in a single directory, hence this proposal.

I agree that

  • civ tech folder is the wrong way (folders are sometimes good but sometimes wrongs - see tags for images or songs for example).
  • grouping civbonuses can be a good thing.

But I still wonder what is a civbonus. (I suggested two definitions.)

Stan added a comment.EditedJan 29 2019, 8:40 PM

So basically does

civbonus = only autoresearched tech specific to a civ
or
civbonus = any tech specific to a civ.

I would tend for the later.

How I understand things:

  • civbonuses are free and automatically researched and are available to one or more factions, but not all
  • special technologies cost resources and are available to one or more factions, but not all
  • general technologies cost resources and are available to all factions
  • advanced_unit_bonus.json, elite_unit_bonus.json, and phase_village.json are free and automatically researched and are available to all factions

What to do with the special technologies can be decided later; they could be grouped together under special/ (see (D888 (outdated)) or mixed with the general technologies (see siege_bolt_accuracy.json).
This patch only groups the civbonuses.

Stan added a comment.Feb 6 2019, 3:13 PM

Still sounds like a nice change.

This revision was automatically updated to reflect the committed changes.
In D1472#71402, @Nescio wrote:

How I understand things:

  • civbonuses are free and automatically researched and are available to one or more factions, but not all
  • special technologies cost resources and are available to one or more factions, but not all
  • general technologies cost resources and are available to all factions
  • advanced_unit_bonus.json, elite_unit_bonus.json, and phase_village.json are free and automatically researched and are available to all factions

What to do with the special technologies can be decided later; they could be grouped together under special/ (see (D888 (outdated)) or mixed with the general technologies (see siege_bolt_accuracy.json).
This patch only groups the civbonuses.

Those definitions sounds reasonnable.

For me the current version doesn't really provide actual benefit (but in fact it could if we want to display those bonuses somewhere without copy pasting things - see old plans).
Though there are still annoying things (for example, I suspect some tech related civ bonuses to not be in that folder).

Also there is still the main issue to fix, aka nuking those civ folders.

This comment was removed by Nescio.
Nescio added a comment.EditedFeb 15 2019, 8:15 PM

True; I'm willing to update D888 to do that, but before that, two things have to be decided first:

  • should we keep unused technologies that were deprecated years ago (D1775)?
  • should special technologies (e.g. siege_bolt_accuracy.json) be grouped together in a special directory or should they be grouped alongside ordinary technologies?
In D1472#72079, @Nescio wrote:

True; I'm willing to update D888 to do that, but before that, two things have to be decided first:

  • should we keep unused technologies that were deprecated years ago (D1775)?
  • should special technologies (e.g. siege_bolt_accuracy.json) be grouped together in a special directory or should they be grouped alongside ordinary technologies?

For the second point, I came to the conclusion to let them in the main directory. As we discussed before, it seems hard to found a schema which fits for all cases.

For the first point, no idea. (I was just interesting in discussing the things I had thought about ;-))

For the second point, I came to the conclusion to let them in the main directory. As we discussed before, it seems hard to found a schema which fits for all cases.

Grouping generic and special technologies together makes sense. In that case we might also consider moving advanced_unit_bonus.json, elite_unit_bonus.json, and phase_village.json into the civbonuses folder and renaming it to autoresearched.

In D1472#72081, @Nescio wrote:

Grouping generic and special technologies together makes sense. In that case we might also consider moving advanced_unit_bonus.json, elite_unit_bonus.json, and phase_village.json into the civbonuses folder and renaming it to autoresearched.

That sounds an option. If I remember correctly, it was discussed to display those techs somewhere in the gui (around the structure tree/encyclopedia/history).

One thing which is still annoying are phase tech (and civ bonuses like techs included in it).

D888 updated.

One thing which is still annoying are phase tech (and civ bonuses like techs included in it).

Yes, phases are annoying. I think the Athenian metal mining civilization bonus should be moved out of their phases.

Also, the civ replacement implemented in A23 (D1024) is causing problems with my mod (see https://wildfiregames.com/forum/index.php?/topic/22779-0abc-mod/&page=8).