[Refactoring] Effects and traits

[crutchmaster]

1. Multiple traits conditions:

has_trait("MUZZLE") || has_trait("MUZZLE_BEAR") || has_trait("MUZZLE_LONG") || has_trait("MUZZLE_RAT")

Decision: Add method “has_trait_flag(std::string flag)” and one or more flags for traits.

2. Traits stat bonuses

    if (has_trait("HOLLOW_BONES")) {
        movecost *= .8f;
    }
...
int player::rust_rate(bool return_stat_effect) const
...
     if (has_trait("FORGETFUL")) {
        ret *= 1.33;
    }
...

Decision: add next structure to trait:

map<string, stat_bonus> bonuses;

where stat_bonus is

struct stat_bonus {string stat (str, dex, rust_rate etc), bool increase (+/-), bool multiply, (* or not), int i, float f, int order};

Next, if you need get movecost mod from traits you do something like

int movecost = 100;
calc_stat_bonus("movecost",&movecost);

method calc_stat_bonus run over all stats, get all that have stat “movecost”, reorder them and calculate bonus. All information about stat bonuses can be store in jsons.
3. Harcoded effects
For penalty/bonuses way like traits
For code :

• Add proceed_effect_player() virtual method.
• Add player *p; field to effect.
• Make class for each effect inherited from this.
• Remove huge part of code from player.cpp to small classes

4. Conditions for effects. Parser for simply conditions like (str > 10), (str > 6 && dex > 4) + methods get_stat(string/enum stat_name), set_stat(string/enum stat_name, string/int/float value)

[Coolthulhu]

1. Multiple traits conditions

This one looks good. Could cut down on some large OR blocks.

2. Traits stat bonuses

The overall idea is good, but this structure looks too generic. As in, trying to encompass too many options at once.

[ol][li]Stat bonus would be better off as an enum rather than string. This would speed up processing and guarantee that selected stat exists.[/li]
[li]Not sure about the order variable. Some sort of ordering is necessary, but a single int wouldn’t guarantee proper ordering, unless all json values had unique values of it, which would require constant maintenance when adding new mutations. I’d just group additions and multiplications and apply one group first, then the other one, depending on the stat type.[/li]
[li]No need to store both int and float. Could be just float.[/li]
[li]increase is redundant - can be expressed by having the value just be negative.[/li]
[li]After the above, splitting the map into multiplications and additions could allow the map to be split into two map<stat_type, float> - one for multiplication, one for addition. This would make them easier to apply with order being dependent on addition/multiplication[/li][/ol]

3. Harcoded effects

The class based approach sounds like what we have in iuse_actor.h/.cpp and mattack_actors.h/.cpp. This is good for when many types share common hardcoded effects.
Alternatively, it could use a simpler system, like our iexamine and iuse functions, with just pointers to functions. This wouldn’t allow as much customization, but would be easier and shorter to write.

Overall sounds good to me.

4. Conditions for effects. Parser for simply conditions like (str > 10), (str > 6 && dex > 4) + methods get_stat(string/enum stat_name), set_stat(string/enum stat_name, string/int/float value)

I don’t really see a good use of this.
Could be relatively hard to write and process.
Any examples where this would be useful? The only examples I can think of right now would be lowering effects of poison with increased strength, lowering effects of fumbling with dexterity and lowering chances of scratching self due to formication with intelligence.

[crutchmaster]

2. Traits stat bonuses

Stat bonus would be better off as an enum rather than string. This would speed up processing and guarantee that selected stat exists.

It is good idea, but new stats must be hardcoded. Workaround - generate id in loading procedure.

Not sure about the `order` variable. Some sort of ordering is necessary, but a single int wouldn't guarantee proper ordering, unless all json values had unique values of it, which would require constant maintenance when adding new mutations. I'd just group additions and multiplications and apply one group first, then the other one, depending on the stat type.

Order need for cases like [code] if (has_trait("TRAIT1") ret *= 0.5; //order == 1 if (has_trait("TRAIT2") ret += 10; //order == 0 if (has_trait("TRAIT4") ret -= 5; //order == 0 if (has_trait("TRAIT3") ret *= 2; //order == -1 [/code] Not needing do order for all bonuses. Default order = 0 - no prioriny for + or -. If bonus must calc first - order = 1 or more, last -1 or less. For group with order "0" no difference for calc priority order. [code]No need to store both int and float. Could be just float.[/code] Ok, but it not so nice (damn perfectionism!). What about union?

`increase` is redundant - can be expressed by having the value just be negative.

Yes. I make it so complex.

After the above, splitting the map into multiplications and additions could allow the map to be split into two map - one for multiplication, one for addition. This would make them easier to apply with order being dependent on addition/multiplication

No reason for split. You got data from one json, split to two map and in future do 2 cycles. Also calc order may be "*++*" or "*+*+" or "+**+" etc.

3. Harcoded effects

The class based approach sounds like what we have in iuse_actor.h/.cpp and mattack_actors.h/.cpp. This is good for when many types share common hardcoded effects. Alternatively, it could use a simpler system, like our iexamine and iuse functions, with just pointers to functions. This wouldn't allow as much customization, but would be easier and shorter to write.

Main idea - place ALL logic for trait/effect/mutations/ect in one place. Best way - class. In other case we have code for every turn, every 30 mins, eating, fighting, code when player get hit and other. If someone want add new effect they should dig all code and add lots of edits instead making one class.

[crutchmaster]

[quote=“Coolthulhu, post:2, topic:11292”]I don’t really see a good use of this.
Could be relatively hard to write and process.
Any examples where this would be useful? The only examples I can think of right now would be lowering effects of poison with increased strength, lowering effects of fumbling with dexterity and lowering chances of scratching self due to formication with intelligence.[/quote]
Yes, i review code and find only few places where it using. So many work and not so useful.

    if( has_effect("cig") || addiction_level(ADD_CIG) > 0 ) {
        healing_factor *= 0.5;
    }
    if( has_effect("drunk") || addiction_level(ADD_ALCOHOL) > 0 ) {
        healing_factor *= 0.5;
    }

Stop! I remembered. It need for weapon effect (stun, knock off, slow down etc) based on stats of attacker, target and weapon.

[Rivet]

I like this, particularly the has_trait_flag part!

[Coolthulhu]

It is good idea, but new stats must be hardcoded.

It was will always have to be the case anyway. A new stat is a new player field.

Not needing do order for all bonuses. Default order = 0 - no prioriny for + or -. If bonus must calc first - order = 1 or more, last -1 or less. For group with order "0" no difference for calc priority order.

But consider someone making a bad mod with + and * in the same order.
This would require parsing all effects to make sure + and * never share an order when affecting the same stat and refusing to load the game if they do.
Or at least making one of + or * go before the other when of equal order.

Ok, but it not so nice (damn perfectionism!). What about union?

Storing two values and using only one of them based on third would be even less nice. Union would require a wrapper to prevent someone accessing the wrong member of it. Something like, `bonus.get_int()` that would check if stored value is an int and return it if so or cast it if it isn't. And the same for float.

Float would be enough. We don’t have effects that add values so high that float loses precision.

Also calc order may be "*++*" or "*+*+" or "+**+" etc.

But honestly, what for? There are very few calculations that have addition and multiplication in mixed order. I can only think of `player::run_cost`, which does it not because of a conscious design decision, but just because it was written over the years like that. If needed, it could be simplified to one with addition after multiplication.

Stop! I remembered. It need for weapon effect (stun, knock off, slow down etc) based on stats of attacker, target and weapon.

But those wouldn’t be just booleans, but calculations.
Something like attacker_strength * 4 + attacker_size * 2 - defender_size * 4 - defender_dodge > rng(10, 20).
Simplifying it to booleans would introduce strict breakpoints or a large arrays of str > 10 AND size < 3 OR str > 9 AND size < 2 etc.
Moving entire formulas to json would be a lot of work.

[kevin.granade]

For the stat effects, we would definitely want to simplify how they are applied instead of trying to migrate them all directly to json. Json representations should be less expressive than code

[crutchmaster]

It was will always have to be the case anyway. A new stat is a new player field.

TRAITS :) not stats.

But consider someone making a bad mod with + and * in the same order.

And got random bonus. Someone problems.

This would require parsing all effects to make sure + and * never share an order when affecting the same stat and refusing to load the game if they do. Or at least making one of + or * go before the other when of equal order.

Just whire "DON'T PLACE + AND * IN ONE ORDER GROUP" and check incoming commits.

Float would be enough. We don't have effects that add values so high that float loses precision.

Ok.

But honestly, what for?

I don't know, i see few places.

Coder, who write it, may be have reasons write that.

Yes. Conditions to code. You convince me.

[Coolthulhu]

TRAITS :) not stats.

Traits will not require new hardcoding. Only new stats.

And got random bonus. Someone problems.

Not handling cases like this is bad design that leads to hard to find bugs. DDA may not be a huge project, but it is not small either. We have to handle all sorts of bugs or they will come back later. Unpredictable bugs are the worst.

Just whire "DON'T PLACE + AND * IN ONE ORDER GROUP" and check incoming commits.

Not everyone is aware of all order variables in given trait/stat group. We need either strict conflict handling or no conflicts - nothing in between. We already have strict conflict handling in most cases - the game will simply print something like “referenced item doesn’t exist” and refuse to load, which makes finding and removing conflicts much easier.

I don't know, i see few places.

Such as? I am not saying that it’s a bad idea altogether, but I just can’t find the justification for that work.
In general, we have + and * order set one way.
I can’t think of a good example in code (other than the move cost function) nor of a good idea to make an use of it.

Coder, who write it, may be have reasons write that.

Possible, but the function is old. Old functions often lack any justification because they were updated by multiple devs with no good idea of what should the order of things be.
The multiple dev thing is another reason why we have to enforce limits properly.

[crutchmaster]

Traits will not require new hardcoding. Only new stats.

New traits not require new hardcoding?

Not handling cases like this is bad design that leads to hard to find bugs. DDA may not be a huge project, but it is not small either. We have to handle all sorts of bugs or they will come back later. Unpredictable bugs are the worst.

Yes, you right. Better don't do possibility for bugs.

Such as? I am not saying that it's a bad idea altogether, but I just can't find the justification for that work. In general, we have + and * order set one way.

player::eat mealtime player::run_cost

Just simple rule: save original logic after refactoring.

[Coolthulhu]

New traits not require new hardcoding?

I mean for stats.
Let’s trace it back to start: you wanted a stat system that stores bonuses in the form of
{ “stat name” : {some_bonuses_here}}

I said that “stat name” could be replaced by stat enum, because we still need a function that parses those stat<->bonus pairs.
This stat name parsing function would still need a hardcoded list of “stat name”<->stat field.

In JSON, the stats would be referred to by their names, but they’d be parsed into the enums when effects were loaded.
For speed and to ensure that the names are correct on game load (rather than when the effect activates).

Just simple rule: save original logic after refactoring.

Yeah, that’s a good rule. But in some cases the original logic doesn’t have any justification and can be reordered.
If I were implementing the whole idea, I’d drop the order option and reorder the functions, but if you want to implement it with the order and have an idea on how to handle “weird” data properly, then it’s fine too.

[kevin.granade]

Just simple rule: save original logic after refactoring.

That’s a tautology, but it is not necessarily the case that you should be refactoring. If it makes things much more complicated, it’s better to change behavior a little and make the interface consistent than to keep everything exactly as it is and make the interface complicated and fragile, which all of the proposed ordering schemes would be.

[crutchmaster]

Ok. I will start coding closer to weekend.

[macrosblackd]

A bit late to the party but since I was just looking at the vision traits and glasses I figured I would chip in my opinion.

JSONifing bonus/penalties:

{
	"effectors" : [
		{
			"type" : "modifier",
			"target_stat" : "dex",
			"value" : 3
		},
		{
			"type" : "multiplier",
			"target_stat" : "melee_hit",
			"value" : 0.85
		}
	]
}

And potential usage:

auto stat_mod = get_modifier_for("some_stat");
stat_mod *= get_multiplier_for("some_stat");

[crutchmaster]

In mutation.h i found:

    std::unordered_map<std::pair<bool, std::string>, int> mods; // Mutation stat mods

Expand this?

[Coolthulhu]

[quote=“crutchmaster, post:15, topic:11292”]In mutation.h i found:

    std::unordered_map<std::pair<bool, std::string>, int> mods; // Mutation stat mods

Expand this?[/quote]

You could start from this, but don’t do effects and traits in one go.
I didn’t see a single PR from you yet and each of those things will most likely be quite big and so will have to be PRed separately.