Craft in the dark

[sparr]

I added the ability to have flags in recipes. Since there are so few, it's just a std::set for now. If we start having a lot of them, they can get refactored like all the other entity flags have.

I added two new flags to recipes, BLIND_EASY and BLIND_HARD. Either indicates that it is possible to craft an item in the dark (or blind). Both require a skill level of difficulty+2. The latter indicates a slowdown in crafting speed, from 100% speed at just-light-enough to 33% speed in pitch black. The same applies to disassembly.

I added the flags to many recipes. I tried to make sensible decisions about things that seemed like to be trivial or slow to do in the dark.

I modified the crafting gui so that the "can craft?" indicator in the top right redraws as you select different recipes, so some will say "too dark" and some "yes". There's also a new "slow" indicator for BLIND_HARD recipes in low light. Also the middle column of the gui has a "Dark crafting?" row with options "Easy" "Hard" "Impossible".

I modified a number of crafting and disassembly checks that previously used fine_detail_vision_mod() so they now use lighting_craft_speed_multiplier(), which should directly mimic their craftability.

I modified fine_detail_vision_mod() to return a wider range. The previous behavior was not as useful, mis-documented, and malformed. I do not believe any existing uses of this function will be impacted by this change.

I changed a few crafting functions to refer to specific recipes, due to the above changes' reliance on the skill/difficulty of each recipe.

I've tried to test this in as many ways as I can, but some quirks in the crafting interface and recipe dictionary make it slow going without being able to use the debug menu to learn recipes. Further playtesting is required before this patch can be merged.

closes #14285
closes #14287

[sparr]

the above recipes are just sliding plates in and out of pockets in the vest. seems very easy to do with your eyes closed.

[sparr]

I don't think any existing lua code uses this function. I considered removing it, but decided to make it work.

[sparr]

this overload allows the lua binding to crafting_allowed() to work with a string parameter. I wasn't able to figure out how/if it would be possible to inform the lua interface about the recipe type.

[Coolthulhu]

Make that string a reference

[sparr]

@Coolthulhu thanks, I missed that. I've been trying to be better about that. Having fixed it... I hate one-character changes to player.h that require me to recompile the whole damn game :/

[sparr]

.count() is the appropriate way to check for membership in a std::set, oddly enough.

[BevapDin]

Why not add two proper bools to the class? That would not only be faster, but also safer regarding spelling mistakes (one might accidentally check for a "BLINDEASY" flag).

[Coolthulhu]

If we're going that way, it could be a float instead. That way it could be more granular than just BLIND_EASY/HARD.

[sparr]

Adding two bools or a float makes this too single-purpose. I anticipate adding more flags to recipes in the future. An array of strings is how flags are handled in every other object type we deal with. @kevingranade said he was "leaning toward a flag".

[BevapDin]

Adding two bools or a float makes this too single-purpose.

The flags only have one purpose to start with. Why make it needlessly generic? If someone later needs generic flags (not named at compile time), they can implement it at that point.

I anticipate adding more flags to recipes in the future.

Fine. But those flag most likely require changes to the code anyway, adding another member and the code to load it from JSON doesn't matter much.

An array of strings is how flags are handled in every other object type we deal with.

The project used to store raw enums in the saves, that wasn't a good approach, but it was commonly used, so I'm leaning towards storing flags as set of strings being cargo cult. Besides: there are several instances where it's done differently: monster type flags use a bitset, which is essentially a vector of bools. Several other types have bool members that act as flags.

he was "leaning toward a flag".

A member bool flag; is a flag.

[sparr]

the monster type flags as a bitset is only an internal represenation. they are still an array of strings in the monster definition (the json). they were promoted to a bitset for efficiency reasons, iirc.

Please don't respond to my short quote from kevin, got read the actual linked discussion where it is in context.

[kevingranade]

The reason to use a bitset instead of a set of strings is if the code doing the lookup is performance critical, but this requires the code to maintain a mapping of strings to numeric values, so it should only be done if necessary.

[Coolthulhu]

I added two new flags to recipes, BLIND_EASY and BLIND_HARD. Either indicates that it is possible to craft an item in the dark (or blind). Both require a skill level of difficulty+2.

If both require the same skill level and only differ by crafting speed, they aren't really different.

[sparr]

@Coolthulhu Do you think perhaps _EASY should have a lower skill penalty in addition to not having a speed penalty?

[Coolthulhu]

Speed penalty should be there, but skill penalty seems quite tough early on (when it matters the most).
I'd expect an untrained survivor to be able to sharpen sticks in the darkness just fine.

I'd change it to just speed penalty for easy, but speed and skill penalty for hard.
That way boiling water or wrapping rags around hands in the darkness wouldn't waste resources due to repeated failure, just take more time.

[sparr]

The very first filter I applied when choosing recipes was to exclude anything that requires a tool ("tools" or "qualities" in the json). That means no stick sharpening or water boiling. That might have been too strong of a filter, but kevin suggested we start small and expand the whitelist later.

Just a speed penalty for easy seems... not like what I had in mind. There are definitely things that I'm good at that I can do just as fast with my eyes closed.

Maybe easy and hard both have a speed penalty, but the penalty falls off with extra skill faster for easy? so at skill+0 they are both 25% speed, at skill+2 easy is 100% speed and hard is 50% speed, and you have to get all the way to skill+6 for hard to be 100% speed.

[sparr]

I've modified what BLIND_EASY and BLIND_HARD mean.

BLIND_EASY allows crafting in the dark with no extra skill. There's a speed penalty, down to 25% speed in pitch black, with no extra skill. +2 skill eliminates the penalty.

BLIND_HARD requires 2 extra skill to craft in the dark. There's a speed penalty, down to 25% speed in pitch black, with the minimum 2 extra skill. +8 skill eliminates the penalty.

@Coolthulhu how does that sound?

[Coolthulhu]

8 - exceeds, but /6.0f?
Am I missing something?

[sparr]

check the if(). that code path only happens if exceeds is >=2

[sparr]

maybe I should move that line out to a helper, since it's used in both conditions with different constants. would be easier to document/explain there.

[Coolthulhu]

Looks OK, but I still think it would be much better to split it into per-item values.
There could be a skill penalty (int) and speed penalty (float). Those could be scaled with darkness as the currently hardcoded values scale. Items uncraftable in darkness could be marked by having skill penalty == -1

That way there could be no penalty for stuff like cooking meat, boiling water on hotplate (it just sits there, no action required) or for turning bandana into a blindfold.

[sparr]

I think providing an int and a float gives too much flexibility and too little consistency. I don't want to see a thousand different combinations of values there for different items added by different people.

This goes with a discussion I was having with @kevingranade recently about auditing and simplifying skill checks in general. There are way too many distinct kinds of skill/stat checks in the game already.

[sparr]

@BevapDin #14350 is another immediate place where flags might be used.

[BevapDin]

There seems to be a slight confusion: I'm not against flags, I'm against implementing them as big old chunk of std::set<std::string> when it's not necessary at all.

What exactly do you gain by storing them as set of strings instead of dedicated bool members? Using members has advantages:

• avoids name conflicts: somebody may add a flag that is already (unknown to them) used somewhere.
• spellchecking at compile time (and auto completion by IDE),
• faster code,
• easier usage: foo.flag vs foo.has_flag("flag")
• overview of used flags directly in the class,
• documentation of all available flags, perhaps via Doxygen.

For example: somebody may implement a new item type feature and may choose to use the flag "CHARGE" to trigger this feature. But that flag is already used by the charger gun, so charger guns automatically have the new feature and all items with the new feature are automatically chargers guns. With generic flag sets, the conflict can only be detected at run time by carefully testing the item. With bool members, one would get this at compile time, or even earlier while adding the new member.

---

I'm also not opposed to loading this from a JSON array (and not from several JSON members), in fact this might even be better as it allows checking of the provided flags:

for( auto &f : jobj.get_flags( "flags" ) ) {
    if( f == "a" ) {
        obj.a = true;
    } else if( f == "b" ) {
        ...
    } else {
        jobj.throw_error( "unknown flag: " + f );
    }
}

Adding new flags only requires adding two new lines here:

    } else if( f == "new_flag" ) {
        obj.new_flag = true;

and a declaration in the class.

---

Neither this PR, nor the linked one requires a generic std::set to store the flags, both can do just fine with bool members. Adding a new flag usually requires one to add code anyway, so adding another member and loading it shouldn't be to much overhead.

---

Btw. and totally unrelated: you need to document the new flags and their meaning in doc/JSON_INFO.md.

[sparr]

What exactly do you gain by storing them as set of strings instead of dedicated bool members?

Less pollution of the struct.

The ability to add new flags without modifying the C++ code at all. We don't currently have much in the way of complex lua mods, but hopefully we will some day.

Consistency with all of the other data types that store sets of flags. AFAIK they all started as a set of strings, and that's why the json get_tags function exists. Unless I'm mistaken, the way flags have worked in this project is they start as a set of strings, then string mapped to ints with an enum, then a bitfield, as the list of flags gets longer and/or the performance of the functions becomes more important.

[kevingranade]

Why are you changing the styling?

[sparr]

because I can't figure out a reasonable process to astyle just one block of code. when I run astyle on a block, the rules work differently than running it on the whole file, and when I run it on the whole file it takes forever to cherry pick my block for committing.

[kevingranade]

Either figure out a way to astyle or internalize the rules. I'm not merging unstyled code, and I'm not cleaning the PRs up any more.

[Malkeus]

What's happening with this pr? I'd love to be able to sensibly craft some
things in the dark (and maybe give some not so sensible things the flag) :)
On Dec 14, 2015 10:51 AM, "sparr" notifications@github.com wrote:

What exactly do you gain by storing them as set of strings instead of
dedicated bool members?

Less pollution of the struct.

The ability to add new flags without modifying the C++ code at all. We
don't currently have much in the way of complex lua mods, but hopefully we
will some day.

Consistency with all of the other data types that store sets of flags.
AFAIK they all started as a set of strings, and that's why the json
get_tags function exists. Unless I'm mistaken, the way flags have worked in
this project is they start as a set of strings, then string mapped to ints
with an enum, then a bitfield, as the list of flags gets longer and/or the
performance of the functions becomes more important.

—
Reply to this email directly or view it on GitHub
#14416 (comment)
.

[kevingranade]

Mostly just me being on vacation, I'm looking into merging it now.

[Malkeus]

"1 check failed" That looks ominous...

Enjoy your vacation, don't worry about us, man.

[kevingranade]

It was just some merge conflicts :)