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

Using zone as firewood source while handling item activity #33372

Conversation

pierredavidbelanger
Copy link
Contributor

Summary

SUMMARY: Features "Use zone for designating a firewood source"

Purpose of change

I face the same problem described in issue #32979 (regarding the requirement of light). And I liked the proposal of using zone instead to designate firewood source.

Describe the solution

While handling item activity, when trying to fuel fire (try_fuel_fire in activity_item_handling.cpp), instead of searching for a tile with a special trap id, I change the code to use zone_manager::get_near instead, with the already defined LOOT_WOOD zone id.

Describe alternatives you've considered

I have not considered other alternative, but I am willing to.

Additional context

Its been a while since I have done C++, I may have done something terribly wrong here, I am sorry for that :)

I did not clean up references linked to the old way of doing this (ie: the trap id tr_firewood_source), I will do this if you like this PR.

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones labels Aug 19, 2019
if( g->m.has_items( tile ) &&
g->m.accessible_items( tile ) &&
g->m.clear_path( center, tile, PICKUP_RANGE, 1, 100 ) ) {
best_tile = tile;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can return directly from here.

Btw. best_tile is not really fitting anyway as it is not comparing different suitable tiles, it only returns the first encountered suitable tile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, you are right.

But, excuse my C++ newbe ignorance, but then how do I return en empty cata::optional<tripoint> at the end of the function without declaring a variable of that type first ?

Copy link
Contributor Author

@pierredavidbelanger pierredavidbelanger Aug 20, 2019

Choose a reason for hiding this comment

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

CLIon is suggesting me to simply return cata::optional<tripoint>(); 😯

Copy link
Contributor

Choose a reason for hiding this comment

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

Or simply return {};, which returns a default-constructed object.

@jbytheway
Copy link
Contributor

I don't think it's appropriate to use the existing LOOT_WOOD zone for this. It should be a new zone type. If players want to use all their looted wood for firewood then they can easily put the two zones on top of one another, but if they don't we shouldn't force that upon them.

@jbytheway
Copy link
Contributor

I did not clean up references linked to the old way of doing this (ie: the trap id tr_firewood_source), I will do this if you like this PR.

The way you've written this, it looks like the old way no longer works. If that's the case then references to it should certainly be removed (at least it should no longer appear in the construction menu to confuse players). Alternatively, you could add the new system while also retaining the old system, to allow a transition period.

@pierredavidbelanger
Copy link
Contributor Author

@jbytheway

I don't think it's appropriate to use the existing LOOT_WOOD zone for this. It should be a new zone type

Yeah! I had the same thought while playing a little with this feature. I accidentally burn some plank I did not wanted to burn. I will do this change.

it looks like the old way no longer works.

Indeed, I replaced the "search".

Alternatively, you could add the new system while also retaining the old system, to allow a transition period.

But should we retaining the old system ? This will not be hard at all to leave everything from the old system in place.

@pierredavidbelanger
Copy link
Contributor Author

My last push implements @jbytheway ideas:

  1. new zone for only this purpose: id SOURCE_FIREWOOD, label Source: Firewood

Source for firewood or other flammable materials in this zone may be used to automatically refuel fires. This will be done to maintain light during long-running tasks that require it such as crafting or reading, but not (for example) if you are simply waiting nearby.

  1. retains the old system in place, so if no SOURCE_FIREWOOD zone are defined nearby, we fallback on the marked spot (trap) code

@pierredavidbelanger pierredavidbelanger changed the title initial impl of using zone as firewood source while handling item activity [WIP] Using zone as firewood source while handling item activity Aug 22, 2019
@jbytheway
Copy link
Contributor

I believe that since that documentation for the firewood source was written, things have changed such that the waiting activity now does trigger fire maintenance. See #31364. So that aspect of your description should probably be changed.

@pierredavidbelanger
Copy link
Contributor Author

@jbytheway you are right, waiting does trigger the refuel fire code. I changed the zone description accordingly.

Source for firewood or other flammable materials in this zone may be used to automatically refuel fires. This will be done to maintain light during long-running tasks such as crafting, reading or waiting.

Since I do not want to mix too many thing in this PR, I did NOT change the Mark firewood source construction description, from where I copied pasted the text, this will need to be updated in an other PR.

@pierredavidbelanger pierredavidbelanger force-pushed the feature-zone-as-firewood-source branch from c7dc4cd to 105a601 Compare August 24, 2019 04:08
@pierredavidbelanger pierredavidbelanger changed the title [WIP] Using zone as firewood source while handling item activity Using zone as firewood source while handling item activity Aug 24, 2019
@ZhilkinSerg ZhilkinSerg self-assigned this Aug 24, 2019
@ZhilkinSerg ZhilkinSerg merged commit 4b22a02 into CleverRaven:master Aug 28, 2019
@ZhilkinSerg ZhilkinSerg removed their assignment Aug 28, 2019
misterprimus pushed a commit to misterprimus/Cataclysm-DDA that referenced this pull request Sep 21, 2019
…en#33372)

* initial impl of using zone as firewood source while handling item activity

* fix my C++ newbeness

* astyle

* new SOURCE_FIREWOOD zone (named "Source: Firewood") + make the old way of marking firewood source still usable

* SOURCE_FIREWOOD zone description now state that waiting *does* trigger the refuel fire code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants