[WIP] Refactor firestarter handling

[ZhilkinSerg]

#21034 is stalled, so I'm reviving it.

What was changed:

• added FIRESTARTER flag;
• removed hard-coded ids from player::has_fire and player::use_fire;
• fixes #22797;
• fixes #21005;
• fixes #22944.

[codemime]

Both "FIRE" and "LIGHT_240" are presumably dead flags too.

[ZhilkinSerg]

I'm not sure about LIGHT_X - see https://github.com/CleverRaven/Cataclysm-DDA/blob/master/src/item_factory.cpp#L165-L168.

[ZhilkinSerg]

FIRE flag is dead for items - I've removed it in #22699

[ZhilkinSerg]

On the other hand, FIRE flag is dead only in current master, but in this PR it defines item which is already burning, so it can be used to ignite something without spending any additional charges.

[codemime]

I'm not sure about LIGHT_X

You're right, it's still used (though a bit awkwardly). Please, disregard my comment concerning it.

in this PR it defines item which is already burning, so it can be used to ignite something without spending any additional charges.

Which means they're mutually exclusive and mustn't appear together in one item.

[ZhilkinSerg]

Which means they're mutually exclusive and mustn't appear together in one item.

Thanks, I've remove FIRESTARTER flag from those items which already have FIRE flag.

[codemime]

The copy-pasted snippet can be eliminated by introducing a function which accepts quantity and returns a suitable firestarter (or the null item if none was found). Also, std::any() would be cleaner for this purpose.

[ZhilkinSerg]

There are two functions has_fire and use_fire which are only called directly by has_charges and use_charges functions.

Do you suggest combining these two functions to a single one bool player::use_or_has_fire( const int quantity, const bool ) to get rid of duplicate code and make existing functions just wrappers of a new one?

Edited: I don't know if it will be possible to combine these two functions as has_-functions are const and use_-functions are non-const.

---

What's with std::any()? I'm not familiar with it. Is it some construction which can return multiple types from same function?

[codemime]

Do you suggest combining these two functions to a single one bool

Not exactly, I had something like this in mind:

itype player::find_firestarter( int quantity ) const
{
    const auto firestarters = all_items_with_flag( "FIRESTARTER" );
    const auto it = std::find_if( firestarters.begin(), firestarters.end(), [quantity]( const item *elem ) {
	return elem->has_charges( quantity );
    } ); 
    return it == firestarters.end() ? ret_null : it->typeId();
}

    ...

    const itype firestarter = find_firestarter( quantity );
    if( firestarter != "null" ) {	// should become a proper id eventually.
        use_charges( firestarter, quantity );
    }
    ...

What's with std::any()? I'm not familiar with it. Is it some construction which can return multiple types from same function?

It returns iterators and generally useful as a replacement for for loops followed by conditions and return statements.

[ZhilkinSerg]

It returns iterators and generally useful as a replacement for for loops followed by conditions and return statements.

So, is it this - http://www.cplusplus.com/reference/algorithm/any_of/ ?

[ZhilkinSerg]

itype player::find_firestarter( int quantity ) const

I guess we should return item, not itype?

[codemime]

So, is it this

Yes, it is. I mistyped the name, so thanks.

I guess we should return item, not itype?

No, we probably shouldn't. The whole point of passing itype is that more than one instance of item can be used to acquire the needed amount of charges.

[ZhilkinSerg]

No, we probably shouldn't. The whole point of passing itype is that more than one instance of item can be used to acquire the needed amount of charges.

So item is certain instance of some item and itype will return item of some type (possibly even multiple items)?

[codemime]

What about "bio_laser" and "bio_tools"? I don't see them being handled anymore (actually, I'm not sure if the later bionic even exists).

[Leland]

Take a look at #21005 – will probably close it as well

[ZhilkinSerg]

Take a look at #21005 – will probably close it as well

Yes, that would be fixed too.

[Night-Pryanik]

#22944 should be fixed too.

[kevingranade]

You might be doing this already, if so cool.
This needs to respect the differences between firestarters, and the differences between firestarting jobs.
I.e. you have open flame (lighters, torch, nearby fire, flamethrower), sparks (flint and steel), and heat source (bow drill, magnifying lens) then you have lighting jobs, like fixed and time insensitive (campfire, kiln), dynamic and time critical (lighting fuses), and maybe fast but very easy (propane torch, oil soaked torch).

Some of these don't work together (flint and steel or bow drill can't reasonably light a dynamite fuse), some might be iffy (flint and steel and kiln).

[BevapDin]

There is no need to check for suitable items first, just iterate over all suitable items directly:

    ...
    return true;
}
for( item *const i : all_items_with_flag(...) ) {
    if( ... ) {
        return true;
    }
}
return false;

If there are no suitable items, the loop will iterate over an empty vector, which means it will be skipped anyway.

[ZhilkinSerg]

Temporary closing this.

[Night-Pryanik]

@ZhilkinSerg is there any chance of reviving this?

[ZhilkinSerg]

Most of the changes from current PR were already implemented in #25276. There is little to be done except for expanding as outlined in #22703 (comment).

[Night-Pryanik]

Oh, my bad. I thought the thing wasn't merged at all.