Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framework to allow burnt items to drop byproducts on destruction #23600

Merged
merged 20 commits into from May 9, 2018

Conversation

@NotFuji
Copy link
Contributor

commented Apr 24, 2018

closes #22983

Items destroyed by fire drop items; included example is wooden items leaving wood ash when burned.

Number of products depends on the weight of the burned item, the fuel efficiency for the reaction, and the amount of the item that is made of flammable material. (Since how much of each material is present in an item isn't defined, it's assumed that each is in equal parts by weight. IE, a tool made of wood and steel is assumed to be 50% wood and 50% steel.

Modular as to handle items made from different materials, including multiple.

@smolbird

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

Byproduct as a property material definitions can cite in JSON might be more useful overall, but interesting idea anyway.

@@ -881,6 +881,35 @@ bool map::process_fields_in_submap( submap *const current_submap,
bool destroyed = fuel->burn( frd, can_spread);

if( destroyed ) {
//Check what the item is made of and spawn appropriate byproducts

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

Please move the new code into a separate function. I don't want to scroll horizontal while reading code.

@@ -881,6 +881,35 @@ bool map::process_fields_in_submap( submap *const current_submap,
bool destroyed = fuel->burn( frd, can_spread);

if( destroyed ) {
//Check what the item is made of and spawn appropriate byproducts
std::vector<material_id> all_mats = fuel->made_of();
if( all_mats.size() > 0 )

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

Please use empty() when you check a container for emptieness.

std::vector<material_id> all_mats = fuel->made_of();
if( all_mats.size() > 0 )
{
units::mass fuel_weight = fuel->type->weight;

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

Why is this using the type weight instead of the item weight item::weigth. Those are not necessarily the same same (e.g. for items with dynamic weight (corpses)). The function has a parameter to exclude the weight of its contents.

//Efficiency as decimal proportion. Value from: https://en.wikipedia.org/wiki/Wood_ash
const double eff = 0.018;
itype_id by = itype_id( "ash" );
int by_n = floor( eff * ( by_weight / item( by ).type->weight ) );

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

You don't need a full item instance to get the item type, you can use item::find_item instead.

units::mass by_weight = fuel_weight / all_mats.size();

std::vector<itype_id> all_by;
std::vector<int> all_by_n;

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

Why two separate vectors when you apparently need them synchronized (element N in one belongs to element N in the other). Use a single vector containing std::pair<itype_id, int> instead.

Actually, why the separate step of gathering type and count and later creating the item? You can create the item directly inside the if branch below.

And when you call an emplace function on a container, the parameters are forwarded to the constructor of the contained type. You don't need to create the instance yourself. In other words: write emplace_back( "item_type", turn, charges ) and the container will create an item for you.

@NotFuji

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

Added requested changes, and moved byproduct definitions to JSON

NotFuji added some commits Apr 26, 2018

Added products to more materials
Existing scrap items where available.  Metals aren't flammable, but destroyed items that are partially metal would have their metal parts voided from existance otherwise.
@@ -461,6 +461,30 @@ field_id field_from_ident(const std::string &field_ident)
return fd_null;
}

void map::create_burnproducts( const tripoint p, item &fuel ) {
std::vector<material_id> all_mats = fuel.made_of();

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

Please use 4 space characters instead of a tab character.

@@ -461,6 +461,30 @@ field_id field_from_ident(const std::string &field_ident)
return fd_null;
}

void map::create_burnproducts( const tripoint p, item &fuel ) {

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

Does the function really need a reference to a non-const item?

This comment has been minimized.

Copy link
@smolbird

smolbird Apr 26, 2018

Contributor

Allowing for materials to yield different burn by-products. Main use would be for items that should melt rather than burn. Future use might be for tin and other metals that fire could melt. Having wooden-handled items return a blade, scrap metal, etc would be viable too.

@@ -461,6 +461,30 @@ field_id field_from_ident(const std::string &field_ident)
return fd_null;
}

void map::create_burnproducts( const tripoint p, item &fuel ) {
std::vector<material_id> all_mats = fuel.made_of();
if( !all_mats.empty() )

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

Instead of adding a level of indentation to the whole function body, you can return from the function when the condition is not true - e.g. if( all_mats.empty() ) { return; } or you can continue in a loop.

Our (style)[doc/CODE_STYLE.md] requires the opening bracket to be on the same line as the if clause.

units::mass by_weight = fuel_weight / all_mats.size();
for( auto &mat : all_mats ) {
if( !mat->burn_products().empty() ) {
mat_burn_products all_bp = mat->burn_products();

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

burn_products is called twice (here and above), calling it before the if clause and using all_bp in the condition would be more efficient. But you don't need this variable (and the emptiness check) at all. It's perfectly valid to iterate over an empty container, and it's valid to iterator over a function result:

for( auto &bp : mat->burn_products() ) {
src/map.h Outdated
/*
* Spawns byproducts from items destroyed in fire.
* Byproducts are declared in JSON as:
* "burn_products": [ [ "X", float efficiency ], [ "Y", float efficiency ] ]

This comment has been minimized.

Copy link
@BevapDin

BevapDin Apr 26, 2018

Contributor

This information is quite useful, but it does not belong here. People that want to edit JSON files won't search here. And when the format is actually changed, the loading code in "material.cpp" is updated, but most likely not the comment on this function. It might be more suitable as comment for material_type::_burn_products.

NotFuji added some commits Apr 27, 2018

Increased granularity of ash
Otherwise a anything smaller than a two by four won't yield any ash whatsoever.
@@ -97,6 +97,12 @@ void material_type::load( JsonObject &jsobj, const std::string & )
_burn_data[ intensity ] = mbd;
}
}
auto bp_array = jsobj.get_array( "burn_products" );
while( bp_array.has_more( ) )

This comment has been minimized.

Copy link
@kevingranade

kevingranade May 3, 2018

Member

This is the remaining astyle regression.

@@ -461,6 +461,24 @@ field_id field_from_ident(const std::string &field_ident)
return fd_null;
}

void map::create_burnproducts( const tripoint p, const item &fuel ) {
std::vector<material_id> all_mats = fuel.made_of();
if ( all_mats.empty() ) { return; }

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu May 5, 2018

Contributor

Style: should be on 3 lines (if(){, body, }).

float eff = bp.second;
int n = floor( eff * ( by_weight / item::find_type( id )->weight ) );

if( n <= 0 ) continue;

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu May 5, 2018

Contributor

Style: don't omit braces

@@ -78,6 +82,7 @@ class material_type
}

const mat_burn_data &burn_data( size_t intensity ) const;
const mat_burn_products burn_products() const;

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu May 5, 2018

Contributor

You could make it a reference, to avoid copying the result.

@@ -689,7 +696,8 @@
{ "fuel": 1, "smoke": 2, "burn": 1, "chance": 3 },
{ "fuel": 1, "smoke": 3, "burn": 2 },
{ "fuel": 1, "smoke": 5, "burn": 5 }
]
],
"burn_products": [ [ "plastic_chunk", 1 ] ]

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu May 5, 2018

Contributor

Plastic is flammable too

@@ -520,7 +525,8 @@
"chip_resist": 8,
"dmg_adj": [ "marked", "dented", "smashed", "shattered" ],
"bash_dmg_verb": "dented",
"cut_dmg_verb": "scratched"
"cut_dmg_verb": "scratched",
"burn_products": [ [ "lead", 1 ] ]

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu May 5, 2018

Contributor

You've got a tab here

@@ -467,7 +471,8 @@
"repaired_with": "scrap",
"dmg_adj": [ "marked", "dented", "smashed", "shattered" ],
"bash_dmg_verb": "dented",
"cut_dmg_verb": "scratched"
"cut_dmg_verb": "scratched",
"burn_products": [ [ "scrap", 1 ] ]

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu May 5, 2018

Contributor

Tab

@@ -461,6 +461,28 @@ field_id field_from_ident(const std::string &field_ident)
return fd_null;
}

void map::create_burnproducts( const tripoint p, const item &fuel ) {
std::vector<material_id> all_mats = fuel.made_of();
if ( all_mats.empty() ) {

This comment has been minimized.

Copy link
@Night-Pryanik

Night-Pryanik May 5, 2018

Member

Should be like if( all_mats.empty() ) {. Also excess space after curly bracket.

@NotFuji NotFuji referenced this pull request May 7, 2018

Closed

Rebalanced Fire #23677

@kevingranade kevingranade merged commit 0c8b413 into CleverRaven:master May 9, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 23.616%
Details
gorgon-ghprb Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.