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.

Sign up for GitHub

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

Jump to bottom

[RDY]Automatic fire refuelling #24077

Merged
merged 2 commits into from
Jul 8, 2018
Merged

[RDY]Automatic fire refuelling #24077

merged 2 commits into from
Jul 8, 2018

Conversation

Coolthulhu
Copy link
Contributor

Long requested feature that got more urgent ~recently, because I implemented a mechanic that makes dying fires more annoying.

Use case goes like this:

  • Player has a small fire adjacent
  • Player has a pile of burnable items - wood pile - adjacent on ONE spot (went for simplicity, the PR grew big fast)
  • Player marks the wood pile using construction menu. Kinda ugly, but I had no better idea that wouldn't get huge fast. It will be easy to change that in the future, though.
  • Player starts a long action that requires light (manually marked in json) - reading or crafting
  • The fire ages to 10 minutes or so
  • The character automatically moves enough items from wood pile to fire pile to keep the fire going. Items on top of the wood pile are picked first, liquids are skipped, unburnable items are skipped.
  • Time passes
  • The fire gets too big because of low granularity of fuel items
  • The character moves first flammable, non-liquid item from fire pile to wood pile, to slow down the rate of burning

I successfully tested it in the following cases:

  • Reading adjacent to a free-standing fire. The fire didn't explode and didn't go out.
  • Reading adjacent to fireplace. The fireplace wasn't over-fueled and it went out shortly after.
  • Reading adjacent to over-fueled free-standing fire. The fire was quickly un-fueled and didn't go super-critical (ie. stayed at intensity 1).

Additionally:

  • Made fire burning more deterministically. It got too chaotic and the player can't easily see if the fire is about to turn into a fireball or go out. May break compatibility with some mods that used burn_data::chance_in_volume.
  • Made fire code use units::volume instead of legacy cups

[CR] because my C++ may be a bit rusty, and because of that weird designation thing.

@ZhilkinSerg ZhilkinSerg added <Enhancement / Feature> New features, or enhancements on existing [JSON] Changes (can be) made in JSON Crafting / Construction / Recipes Includes: Uncrafting / Disassembling [C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. labels Jun 24, 2018
@RadHazard
Copy link
Contributor

Does this work with crafting that requires fire (e.g. cooking)? I know that it currently doesn't interrupt crafting if the fire goes out halfway through, but if this system works then it seems like it would be reasonable to change it so that it does.

@Coolthulhu
Copy link
Contributor Author

Does this work with crafting that requires fire (e.g. cooking)?

Currently all crafting triggers it, even if you don't need the light because you have other sources.

@OzoneH3
Copy link
Member

OzoneH3 commented Jun 26, 2018

Works great. Maybe up the fire/stockpile range from adjacent to crafting range?

@Coolthulhu
Copy link
Contributor Author

Maybe up the fire/stockpile range from adjacent to crafting range?

Yeah, would be better than hardcoded 1 range limit.

@kevingranade any objections? The closest analogy I recall would be the vehicle crane thing, which is limited to 1 tile.

@kevingranade
Copy link
Member

If the player is free to move during the activity, then using crafting range is reasonable. I also don't anticipate perf issues for scanning since it only happens intermittently during crafting.

@Coolthulhu Coolthulhu changed the title [CR]Automatic fire refuelling [RDY]Automatic fire refuelling Jun 28, 2018
@Coolthulhu
Copy link
Contributor Author

Does "Jenkins, rebuild" re-trigger Travis too?

@kevingranade
Copy link
Member

kevingranade commented Jun 29, 2018 via email

@Coolthulhu
Copy link
Contributor Author

Nope. Requires developer privs

@Night-Pryanik
Copy link
Contributor

I was able to restart this travis build.

@ZhilkinSerg ZhilkinSerg self-assigned this Jun 30, 2018
ZhilkinSerg
ZhilkinSerg reviewed Jun 30, 2018
View reviewed changes
src/player_activity.cpp
@@ -96,6 +96,11 @@ std::string player_activity::get_str_value( size_t index, std::string def ) cons

void player_activity::do_turn( player &p )
{
// Should happen before activity or it may fail du to 0 moves
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

du -> due

@ZhilkinSerg
Copy link
Contributor

I cannot do thorough testing right now, but works fine at first glance.

kevingranade
kevingranade approved these changes Jul 7, 2018
View reviewed changes
Copy link
Member

@kevingranade kevingranade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've merged this and tested it locally, but I seem to have forgotten the push it, so feel free to just resolve the conflict in item.h and push it.

src/activity_item_handling.cpp
best_fire = pt;
best_fire_age = fire_age;
}
// If a contained fire exists, ignore any other fires
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should find the best contained fire, instead it finds the first contained fire, I'm fine to leave it as a bugfix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to change it after the range increase, though it's next to trivial.
I'll leave it as is for now since it's tested already. It's pretty unlikely that a player will have two contained fires anyway.

@Coolthulhu
Copy link
Contributor Author

I'm not authorized because I'm not a part of CleverRaven at the moment.
If you add me, I'll push the fix.

@ZhilkinSerg ZhilkinSerg merged commit 47a80bc into CleverRaven:master Jul 8, 2018
@ZhilkinSerg ZhilkinSerg removed their assignment Jul 8, 2018
@kevingranade kevingranade mentioned this pull request Aug 17, 2018
Closed
@Brambor
Copy link
Contributor

Brambor commented Aug 18, 2018 •
edited
Loading

Does not work with charcoal. Maybe it has something to do with that charcoal stacks in form charcoal (50) whereas two-by-four and more flamable items stack in two-by-four 50x.

I would expect charcoal to be valid item for keeping the fire going.

@Brambor
Copy link
Contributor

Brambor commented Aug 18, 2018

Oh and it is not clear how to stop refueling the fire as if you read by the window and the PC keeps throwing more and more wood into the fire.

@ZhilkinSerg ZhilkinSerg mentioned this pull request Jan 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZhilkinSerg ZhilkinSerg ZhilkinSerg left review comments

@kevingranade kevingranade kevingranade approved these changes

Assignees
No one assigned
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Crafting / Construction / Recipes Includes: Uncrafting / Disassembling <Enhancement / Feature> New features, or enhancements on existing Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. [JSON] Changes (can be) made in JSON
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Coolthulhu @RadHazard @OzoneH3 @kevingranade @Night-Pryanik @ZhilkinSerg @Brambor