Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upHot air from fires and direct heat radiation applies to tile temperatures #24956
Conversation
nexusmrsep
added some commits
Aug 18, 2018
This comment has been minimized.
This comment has been minimized.
stk2008
commented
Aug 18, 2018
|
Holy crap this released already I so can't wait to test this. So select display heat radiation and they it overlays heat icon coming from and around fire plus any where else the heat is actually going to. PR this nowwwww I want lol. Thanks |
codemime
reviewed
Aug 18, 2018
|
Copy-pasted chunks of code are simply a no go. Common code should be extracted and reused as needed. |
This comment has been minimized.
This comment has been minimized.
|
Also |
This comment has been minimized.
This comment has been minimized.
|
@codemime you are right, I will rework it so the method will have a common place in the code. |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
reviewed
Aug 19, 2018
|
Some observations, possible improvement |
| for( const auto &intensity_dist : fires ) { | ||
| const int intensity = intensity_dist.first; | ||
| const int distance = intensity_dist.second; | ||
| temp_mod = 6 * intensity * intensity / distance; |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 19, 2018
Contributor
value of temp_mod on exiting this loop is always the value of 6 * fires.back().first * fires.back().first / fires.back().second and effectively ignores all other values. Is this intentional?
This comment has been minimized.
This comment has been minimized.
nexusmrsep
Aug 19, 2018
Author
Contributor
You are right, should be +=. Heat radiation should sum up from multiple sources, thats the sole purpose of this loop.
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 19, 2018
Contributor
Is it supposed to be 6* (intensity^2 / distance)? I think radiant heat would follow an inverse-square intensity change more closely ( 6 * (intensity / distance^2) ), dropping off rapidly with distance. A linear drop isn't an issue though, mostly concerned about the intensity being squared.
| fires.emplace_back( std::make_pair( heat_intensity, fire_dist ) ); | ||
| if( fire_dist <= 1 ) { | ||
| // Extend limbs/lean over a single adjacent fire to warm up | ||
| best_fire = std::max( best_fire, heat_intensity ); |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 19, 2018
Contributor
best_fire is set here, but is not used elsewhere in the temperature calculations.
This comment has been minimized.
This comment has been minimized.
nexusmrsep
Aug 19, 2018
Author
Contributor
that too should be removed, it was needed in original, but not here
| break; | ||
| default: | ||
| break; | ||
| } |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 19, 2018
Contributor
Since the structure is very similar over each of the switch blocks, can make use of a lambda:
auto tile_strength_mod = []( const tripoint &loc, field_id fld, int case_1, int case_2, int case_3 ){
int strength = g->m.get_field_strength( loc, fld );
int cases[3] = { case_1, case_2, case_3 };
return ( strength > 0 && strength < 4 ) ? cases[ strength - 1 ] : 0;
};
Using the lambda:
temp_mod += tile_strength_mod( location, fd_hot_air1, 2, 6, 10 );
temp_mod += tile_strength_mod( location, fd_hot_air2, 6, 16, 20 );
temp_mod += tile_strength_mod( location, fd_hot_air3, 16, 40, 70 );
temp_mod += tile_strength_mod( location, fd_hot_air4, 70, 100, 160 );
This comment has been minimized.
This comment has been minimized.
nexusmrsep
Aug 19, 2018
Author
Contributor
Those are code shenanigans I never suspected i'd be using with my C++ skill level, so I'm thankful for the idea, and of course it is a lot cleaner.
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 19, 2018
Contributor
Actually, looking at how field strength is processed don't even need the conditional. Strength will always be in the range [1..3], so both conditions will always be true.
Can change the return line to return cases[ strength - 1 ];
This comment has been minimized.
This comment has been minimized.
|
Since |
This comment has been minimized.
This comment has been minimized.
|
There may be an odd interaction between |
OrenAudeles
referenced this pull request
Aug 19, 2018
Merged
Cleanup calls to game::get_temperature #24964
This comment has been minimized.
This comment has been minimized.
|
@Rod995 Let me explain some of your observations:
Of course, its visibility is just for testing purposes - imagine how hard it would be to test invisible field. In fact this PR does not have code lines that allow anyone to see hot air. Also multiple 'hot_air' entries you saw was different types of hot air (there are 4 types with 3 lvls of intensity each) - this is original design by original author, and it does its role. And fire has heat radiation too, like lava, but with mild intensity in case of a |
This comment has been minimized.
This comment has been minimized.
Can you elaborate? Long fast-forward with temperature input affected by the fire will be a rare occurrence, and fire affected temperature applies to food, so rot calculations should include it. If not the effect would be that rot could consider food as frozen while it's sitting in a blazing fire. But perhaps it's not what you meant? |
This comment has been minimized.
This comment has been minimized.
|
@nexusmrsep Going to copy over my response from Discord
|
This comment has been minimized.
This comment has been minimized.
tyrael93
commented
Aug 19, 2018
|
Question: I seem to remember one of the main reason fires used to be lag machines in the old versions was because of the long list of added checks for smoke and various other variables, which was fixed by Coolthulhu some years ago. Will this change with all its added hidden calculations make the game lag the game to hell and back like it used to? |
This comment has been minimized.
This comment has been minimized.
|
@tyrael93 short answer: no it shouldn't. If a smoke grenade doesn't slow your machine, this will neither. There are safeguards preventing excessive generation of smoke in large fires, and if it ever becomes a problem they can be tweaked tighter. |
This comment has been minimized.
This comment has been minimized.
SaltatorMortis
commented
Aug 19, 2018
|
Question: Do you plan to use Thermal conductivity as factor for passive cooling? or will closed doors "freeze" the calculation(after the Temperature spread out evenly) for simplicity? |
This comment has been minimized.
This comment has been minimized.
|
@SaltatorMortis short answer: No. I use existing mechanics, that already had hot air coded. It was mostly not used though, but now it serves a purpose. It behaves the same way as smoke both inside and outside houses, so it's rather gas spread mechanics then thermal dissipation, but overall, at this level of simulation its decent enough to use it, without implementing NASA style code that would make your CPU sweat. |
This comment has been minimized.
This comment has been minimized.
|
Might be not related enough to make the change in this PR but could you take a look at Lava it should logically at least spread fire like a raging fire around it but does not seem to do so? |
This comment has been minimized.
This comment has been minimized.
|
@Mecares that might be a topic for another PR, since I guess you don't refer to simple heat radiation. Mind you, this could in fact lead to some nasty fires, but perhaps would be a nice addition. Make a feature request issue on it. |
This comment has been minimized.
This comment has been minimized.
|
@codemime your request is applied. Major reworks done. Also thanks @OrenAudeles for your invaluable help.
|
OrenAudeles
suggested changes
Aug 20, 2018
|
|
| // @param location Location affected by heat sources | ||
| // @param direct forces return of heat intensity (and not temperature modifier) of | ||
| // adjacent hottest heat source | ||
| int get_heat_radiation( const tripoint &location, bool direct ); |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 20, 2018
Contributor
Does not need to be in class game. Only uses the map m member of game. Extract from class game and modify declaration to int get_heat_radiation( const map& m, const tripoint &location, bool direct);
| // adjacent hottest heat source | ||
| int get_heat_radiation( const tripoint &location, bool direct ); | ||
| // Returns temperature modifier from hot air fields of given location | ||
| int get_convection_temperature( const tripoint &location ); |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 20, 2018
Contributor
oes not need to be in class game. Only uses the map m member of game. Extract from class game and modify declaration to int get_convection_temperature( const map& m, const tripoint &location );
| std::vector<std::pair<int, int>> fires; | ||
| fires.reserve( 13 * 13 ); | ||
| int best_fire = 0; | ||
| for( const tripoint &dest : g->m.points_in_radius( location, 6 ) ) { |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 20, 2018
Contributor
Whether function is moved from class game to be a free function, or remains as is, can shorten all instances of g->m.__func__ to just m.__func__
Other lines impacted:
- 1804
- 1807
- 1810
Function is only called currently as g->get_heat_radiation so it already has access to the global-g's map.
| { | ||
| // Heat from hot air (fields) | ||
| int temp_mod = 0; | ||
| const trap &trap_at_pos = g->m.tr_at( location ); |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 20, 2018
Contributor
Whether function is moved from class game to be a free function, or remains as is, can shorten all instances of g->m.__func__ to just m.__func__
Other lines impacted:
- 1840
- 1846
Function is only called currently as g->get_convection_temperature so it already has access to the global-g's map.
This comment has been minimized.
This comment has been minimized.
nexusmrsep
Aug 23, 2018
Author
Contributor
If it's freed from game class it losses access to m which is member of this class so g->m within free functions have to stay.
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 24, 2018
Contributor
If a map-reference is added as a parameter to the free function you can get rid of the g-> part. You are correct that if the parameter isn't added g->m is the only way you will keep access to the map.
| } | ||
| // hot air of a fire/lava | ||
| auto tile_strength_mod = []( const tripoint &loc, field_id fld, int case_1, int case_2, int case_3 ){ | ||
| int strength = g->m.get_field_strength( loc, fld ); |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 20, 2018
Contributor
In addition to changing from g->m.get_field_strength to just m.get_field_strength, if the function is made a free function and its function signature modified to pass in the map may need to modify the lamda to capture the passed in map variable: [&m]( const tripoint &loc, ... ){
| auto tile_strength_mod = []( const tripoint &loc, field_id fld, int case_1, int case_2, int case_3 ){ | ||
| int strength = g->m.get_field_strength( loc, fld ); | ||
| int cases[3] = { case_1, case_2, case_3 }; | ||
| return ( strength > 0 && strength < 4 ) ? cases[ strength - 1 ] : 0; |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 20, 2018
Contributor
Having looked into things more carefully the condition isn't necessary. Can be modified to just return cases[ strength - 1 ];
This comment has been minimized.
This comment has been minimized.
nexusmrsep
Aug 23, 2018
Author
Contributor
...and it sends the game into infinite loop with insane temperatures, stamina loss, and inevitable death. No, just no...
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 24, 2018
Contributor
oh I'm dumb. I was making an assumption and not validating that assumption. If the field does not exist at the location it's not going to be returning a value in range [1, 3]. Sorry for the bad suggestion.
|
|
||
| if( !new_game ) { | ||
| temp_mod += g->get_heat_radiation( location, false ); | ||
| temp_mod += g->get_convection_temperature( location ); |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 20, 2018
Contributor
If game::get_heat_radiation and game::get_convection_temperature are made into free functions change calls to get_heat_radiation( g->m, location, false ); and get_convection_temperature( g->m, location );
| // Hot air from a heat source | ||
| // times 50 to convert to weather.h BODYTEMP scale | ||
| const int fire_warmth = g->get_convection_temperature( pos() ) * 50; | ||
| const int best_fire = g->get_heat_radiation( pos(), true ); |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 20, 2018
Contributor
If game::get_heat_radiation and game::get_convection_temperature are made into free functions change calls to get_heat_radiation( g->m, pos(), true ); and get_convection_temperature( g->m, pos() );
| temp_conv[bp] += 300 * heat_here; | ||
| blister_count += heat_here; | ||
|
|
||
| const int h_radiation = g->get_heat_radiation( pos(), false ); |
This comment has been minimized.
This comment has been minimized.
OrenAudeles
Aug 20, 2018
Contributor
If game::get_heat_radiation is made into a free function change call to get_heat_radiation( g->m, pos(), false );
This comment has been minimized.
This comment has been minimized.
|
@OrenAudeles does making them free functions serve as optimization purpose or is there another reason? Player.cpp also calls game class directly for both of those new functions and not in general for get_temperature(), and I can see that in the future they may be called from elsewhere too. And also: originaly half of them were player.cpp code. Since temperature now trends to be more of a game.cpp thing (and less map.cpp and weather.cpp) this is the reason why I placed it there. |
This comment has been minimized.
This comment has been minimized.
|
It's to keep the interface of |
nexusmrsep
referenced this pull request
Aug 21, 2018
Closed
Items freezing so fast the game is almost unplayable. #25014
nexusmrsep
added some commits
Aug 23, 2018
This comment has been minimized.
This comment has been minimized.
|
@codemime please verify validity of your request again. Your suggestions were applied. If you don't, I'll dismiss your review as obsolete. Due to changes of temperature application I've reworked mechanics for adding blisters from heat radiation sources, and methods of protection from that effect. To my surprise I even found a bug where What happens is that even if you are protected from blisters/fire, you will overheat rapidly (near big fire/lava), losing speed, stamina, and looping into a kind of unconscious/coma state in which time passes and you cannot move while being (probably) still affected by the temperature source. With RM13 i was even able to survive the dying out of the fire and lot of time passed before regaining control was even possible (then some zombies ate me). Problem is - I'm not even sure IF it needs balancing, so I'm pointing this out to the public. This is either intended or partially intended - while I believe its realistic, it might need balancing or might not. Can't make this decision myself. Steps to recreate: /testing environment/
|
ZhilkinSerg
assigned
ZhilkinSerg
and unassigned
ZhilkinSerg
Aug 24, 2018
nexusmrsep
dismissed
codemime’s
stale review
Aug 26, 2018
Obsolete. Requested changes were applied.
nexusmrsep
changed the title
[WIP] Hot air from fires and direct heat radiation applies to tile temperatures
Hot air from fires and direct heat radiation applies to tile temperatures
Aug 26, 2018
OrenAudeles
approved these changes
Aug 27, 2018
|
Shoulda modified my review earlier, sorry. I have no more issues. |
ZhilkinSerg
removed their assignment
Aug 27, 2018
ZhilkinSerg
merged commit 4205ba5
into
CleverRaven:master
Aug 27, 2018
nexusmrsep
deleted the
nexusmrsep:hot_air_to_temperature
branch
Aug 27, 2018
This comment has been minimized.
This comment has been minimized.
|
Question: Can more than one hot air of the same type be on one tile? E.g. 2x Question: Does This would be true even if |
This comment has been minimized.
This comment has been minimized.
|
@Brambor to my knowledge:
I was expecting decay of higher type fields of hot air to lesser types to be there, but its apparently not coded. While it'd be fun, I don't think I'll do it. Perhaps someone else would. |
This comment has been minimized.
This comment has been minimized.
|
If you are accepting my issue, I'm really proposing a possible bug, the solution to it would be that |
This comment has been minimized.
This comment has been minimized.
|
No, i do not recognize this as an issue/bug. Testing did not prove that anything out of ordinary happens because of that. At this level of simulation there is no need to change that, unless someone determined wants to have a try. I'm not determined enough to fix problems that do not add any substantial value to the simulation. |
This comment has been minimized.
This comment has been minimized.
|
I suggest waiting for feedback from various players for several days. |














nexusmrsep commentedAug 18, 2018
•
edited
SUMMARY: Features "Hot air and direct heat radiation from fires affect local temperature and can heat nearby area including interiors."Resolves #24938
What does it do?
This PR:
Conclusion:
It indirectly allows to defrost food or prevent it from freezing by raising adjacent temperature.
Rationale:
You can use a stove in a house to heat up interiors in the winter. Heated room raises temperature of items in that room. You can warm up by the fire, so your items or items stored nearby should too, just by being there.
Implementation
Mainly
copiedreworked parts of the code that ruled over player being affected by proximity of fires and adjusted them to be used byget_temperature()function ingame.cppAlso changed restriction in
field.cppto allow propagation of hot air from small fires, while restricting it when there is too much fire nearby for optimization.After reworking functions for
heat radiationandheat convectionakahot air propagationI had to rework howheat radiationaffects player - blisters, turnout gear, heatsinks, etc. aiming at keeping them close to original balance.Risks
Where possible I did not change values used by that code in other way then converting them to the applied system, so this should be balanced BUT I did not remove anything that affects player and I know that
get_temperature()is also checked in calculations of player-fire player-lava temperature interactions. So that might result in higher impact of those heat sources on the player.EDIT: This change required re-balancing of blister application and some mechanics of applying heat from fire/lava to the player body temperatures, including how heat-sinks behave. It also - as i pointed out in comments below - introduces higher risk of lethal overheating in big fires. Might also lead to much endurable overheating from small fires in hot seasons. Something to be aware about.
Testing
Test were positive for the main function of this PR:
8 in the middle is a small fire in a brazier, ~ is hot air from said fire (made visible for testing purposes) color is intensity from white-yellow-red (1-2-3)
Ambient temperature where character stands is -12C, tile is affected by type 1 intensity 3 hot air, and character is one tile away from small fire in a brazier for direct heat radiation, this brings the temperature to -3C overall.
Same room, left upper corner: out of range of direct heat radiation, inside type 1 intensity 3 hot air, temp from -12C --> -7C
Request a test?
Yes, please. It is done and tested, but I'll remove the [WIP] flag only after someone does some testing too and compare the results.