Configurable Secondary Skills

International board Development
#1

I want to implement make secondary skills moddable, by introducing JSON config for them.

While looking at the code, I wondered something. In CGHeroInstance::updateSkill(), two different ways of getting a bonus object are used:

// used by luck, morale and diplomacy
Bonus *b = getBonusLocalFirst(Selector::type(Bonus::SURRENDER_DISCOUNT).And(Selector::sourceType(Bonus::SECONDARY_SKILL)))

// used by all others
Bonus * b = getExportedBonusList().getFirst(Selector::typeSubtype(Bonus::SECONDARY_SKILL_PREMY, which).And(Selector::sourceType(Bonus::SECONDARY_SKILL)))

Why is this the case, and does it have to be that way?

Also, I’m thinking about the JSON format for secondary skills. At the moment I have something like this in mind:

{
"name":"skill name",

"basicIcon":"icon name",
"advancedIcon":"icon name",
"expertIcon":"icon name",

"basicBonuses":<bonus format>,...],
"advancedBonuses":<bonus format>,...],
"expertBonuses":<bonus format>,...],

"whitelistFactions": "faction",..], // only necropolis heroes can have Necromancy
"blacklistFactions": "faction",..], // necropolis heroes can't have Leadership

"maxlevelsMight":0, // for wisdom and spell schools
"maxlevelsMagic":0
}

What do you think of this?

#2

Levels must be present as array even if we don’t support 3+ level skills yet:

		"levels" : {
			"basic":{
				"bonuses":<bonus format>,...],
				"icon":"icon name",
				etc...
			},
			"advanced":{
				"bonuses":<bonus format>,...],
				"icon":"icon name",
				etc...
			}
		},
#3

About bonuses. Code is inconsistent there because this way (with help of @AVS) I attempted to fix corruptions within bonus system. Issue was that some of that code modified “cache” of bonus lists instead of actual data. You can find related commits with “git blame”. Sadly looking on that code now I’m no longer sure there is no code with wrong behavior. :blush:

Unfortunately it’s impossible to improve bonus system further until noncopyableBonusSystemNode branch merged. And that won’t happen till we start breakage of save format. :neutral_face:

PS: So if you decide to change something related to bonuses please make sure to work on top of that branch.

#4

Second is right way to modify (only modify existing objects) local bonuses. (getExportedBonusList return actual container) Be careful with client-server synchronization.

I like different design

{
"base":{<attributes for all skill levels if any>},
"basic":{"icon":"", "bonuses":{} },
"advanced": ...
}

Also try to avoid json arrays - they are bad for modding.

#5

Great idea, i hope you can make it!

That’s bad idea, since secondary skills mod doesn’t know how many new factions there will be.
Now secondary skills are written in heroClass:
wiki.vcmi.eu/index.php?title=Hero_Classes_Format

	// Chance to get specific secondary skill on level-up
	// Skills not listed here will be considered as unavailable, including universities

I think it’s wrong idea, i suppose you change this to that way:

// Chance to get specific secondary skill on level-up
// Skills with chance to get =0 here will be considered as unavailable, including universities
// Skills non listed here will have default chance on level-up
#6

?
I think it’s not needed. Required secSkills/primarySkills for skill can be written in levels.

Secondary skill needs “affinity” : “magic”/“might” altrough.

#7

Will do it that way instead.

It is true that secondary skills won’t know all factions. However, hero classes won’t know all secondary skills either. I think the best solution is doing it as you suggested; secondary skills need to be explicitly banned from the hero class. However, it would be troublesome if someone makes a mod with a secondary skill only meant for one faction (like necromancy), because the modder would then have to mod all existing hero classes (even other ones added by mods). A field in the secondary skill config, called “defaultChance” would solve that issue. That value will be used, if it is not expicitly specified in config.

Wisdom and spell schools have the property that heroes are guaranteed to get them as a choice every n:th level; that’s what I intended these fields for. What would the “affinity” field be useful for?

#8

Maybe it not needed, I don’t remember now.
In “heroClass” we have “affinity”, which determines, might or magic skills hero will preferably exercise.

#9

A question:

How do translations work? The same as other mods (by overriding JSON config) or somehow else? I couldn’t figure it out by looking through the code, and I need to know this to implement changes to secondary skills…

#10

By overriding JSON. Also its recommended to have translatable strings in separate JSON files (but in same schema) - f.e. see “bonuses.json” “bonuses_texts.json” .

#11

At the moment I have gotten stuck on handling heroes levelling up and proposing new skills. It is really difficult because there are a lot of special cases to handle, and even more so due to the fact that it should be configurable whether or not those special cases apply…

I will likely refactor the skill on level up selection away from CGHeroInstance, because I think it should be possible to override the default behaviour when scripting is supported.

#12

How is the progress going?
Will it be made with same bonuses system as spells and hero/creature bonuses?

#13

Still stuck at the same problem I mentioned above. And yes, it will use the same bonus system.

#14

Some of standard skills allow SECONDARY_SKILL_PREMY bonuses in %, and some are not. Maybe firstly make them all resposible to bonuses % and move level % to json configs.
Maybe standard secondary skills could be left as they are now, and only add new skills modding posibility?

#15

Are there any news in implementing secondary skills modding and adding?
Don’t see GitHub commits on this topic :confused:

#16

I got stuck as mentioned above, then I didn’t have any time to work on it for a long time. I still want to do this at some point in the future, but I’ll likely have to take a different approach, such as refactoring CGHeroInstance first.

#17

Seems like there will not be new sec. Skills for a long time, sadly. I hoped it will appear in nearest versions.