[WIP][CR] Add constants for easily changing the size of the reality bubble at compile time. #23376

mrkybe · Apr 5, 2018

They should really be documented better, but I don't have enough of an understanding of the various parts to do it.

kevingranade · Apr 5, 2018

kevingranade requested changes Apr 5, 2018

View changes

A bunch of these aren't distances, and some if the distances shouldn't scale with map size.

kevingranade · Apr 5, 2018

src/activity_handlers.cpp

@@ -890,7 +890,7 @@ void activity_handlers::firstaid_finish( player_activity *act, player *p )
 static void rod_fish( player *p, int sSkillLevel, int fishChance )
 {
   if( sSkillLevel > fishChance ) {
-        std::vector<monster *> fishables = g->get_fishable(60); //get the nearby fish list.
+        std::vector<monster *> fishables = g->get_fishable(DISTANCE_FROM_CENTER); //get the nearby fish list.


This is range at which you can attract fish to a lure, 60 already seems a bit overboard...

Renamed to fishing_distance.

kevingranade · Apr 5, 2018

src/computer.cpp

@@ -512,10 +512,10 @@ void computer::activate_function( computer_action action )
    break;

    case COMPACT_MAP_SEWER: {
-        g->u.moves -= 30;


This is the cost of reading the map, not distance related.

kevingranade · Apr 5, 2018

src/map.cpp

@@ -5695,18 +5695,26 @@ const visibility_variables &map::get_visibility_variables_cache() const {

 lit_level map::apparent_light_at( const tripoint &p, const visibility_variables &cache ) const {
    const int dist = rl_dist( g->u.pos(), p );
+
+    // Being outside of the world bubble overrides Clairvoyance.


This shouldn't be necessary, wasn't the fix for this in handle_mouseview?

Finally getting back to this PR. So apparently this function isn't only called from handle_mouseview, which is why I ended up handling it here instead of in handle_mouseview.

It is used heavily in map::update_visibility_cache, which is called on the entire bubble and only bubble (ie. it doesn't ever cross boundaries of the bubble).
update_visibility_cache is not cheap and apparent_light_at is the most expensive part of it.

map::inbounds isn't an expensive check though. I'm not sure how else to do it.

Extract the check to print_all_tile_info, which is the only place where this check matters.

Ah, I see. Done.

kevingranade · Apr 5, 2018

src/map.cpp

    const auto &map_cache = get_cache_ref(p.z);
    bool obstructed = map_cache.seen_cache[p.x][p.y] <= LIGHT_TRANSPARENCY_SOLID + 0.1;
    const float apparent_light = map_cache.seen_cache[p.x][p.y] * map_cache.lm[p.x][p.y];

    // Unimpaired range is an override to strictly limit vision range based on various conditions,
    // but the player can still see light sources.
-    if( dist > g->u.unimpaired_range() ) {


Unnecessary change.

kevingranade · Apr 5, 2018

src/monattack.cpp

@@ -4512,7 +4512,7 @@ bool mattack::grenadier(monster *const z)
    if (z->attitude_to( *target ) == Creature::A_FRIENDLY) {
        return false;
    }
-    int ret = grenade_helper(z, target, 30, 60, grenades);
+    int ret = grenade_helper(z, target, 30, DISTANCE_FROM_CENTER, grenades);


I suspect this is a range, not a position, it doesn't necessarily need updating.

Upon further examination, this was a moves cost. Returned back to '60'.

kevingranade · Apr 5, 2018

src/player.cpp

@@ -2756,8 +2756,8 @@ float player::active_light() const

    lumination = ( float )maxlum;

-    if( lumination < 60 && has_active_bionic( bio_flashlight ) ) {
-        lumination = 60;


This is a lighting level, not a distance.

kevingranade · Apr 5, 2018

src/sounds.cpp

@@ -175,7 +175,7 @@ int get_signal_for_hordes( const centroid &centr )
    //Volume in  tiles. Signal for hordes in submaps
    //modify vol using weather vol.Weather can reduce monster hearing
    const int vol = centr.volume - weather_data( g->weather ).sound_attn;
-    const int min_vol_cap = 60; //Hordes can't hear volume lower than this
+    const int min_vol_cap = DISTANCE_FROM_CENTER; //Hordes can't hear volume lower than this


Not map related.

kevingranade · Apr 5, 2018

src/weather_data.cpp

@@ -42,7 +42,7 @@ weather_datum const weather_data( weather_type const type )
     * Weather types data definition.
     * Name, color in UI, ranged penalty, sight penalty,
     * light modifier, sound attenuation, warn player?
-     * Note light modifier assumes baseline of DAYLIGHT_LEVEL at 60


Again, light level, not distance.

BevapDin · Apr 5, 2018

Please don't add macros, add constants as static constexpr.

Coolthulhu · Apr 14, 2018

Coolthulhu reviewed Apr 14, 2018

View changes

Coolthulhu · Apr 14, 2018

src/game_constants.h

+#define DISTANCE_FROM_CENTER 60
+
+/** Was used based off lighting but I'm not sure?*/
+#define LIGHT_SIXTY_F 60.0f


This won't work right. You'd need to alter air transparency instead, otherwise you'll get a ton of side effects.

BevapDin · Apr 14, 2018

BevapDin reviewed Apr 14, 2018

View changes

BevapDin · Apr 14, 2018

src/game_constants.h

+#define DISTANCE_FROM_CENTER 60
+
+/** Was used based off lighting but I'm not sure?*/
+#define LIGHT_SIXTY_F 60.0f


If you put the (current) value of the constant into its name, you might as well use the value directly. As soon as somebody changes the value, the name will be misleading.

Regarding two constants for the same value (int and float): define just one of them. And if you really need the other one (note: the compiler will convert the values as it needs it anyway), you should define it with a reference to the first one, e.g.

constexpr int DIST = 60; constexpr float DIST_FLT = DIST;

But it's better to use the integer constant all the time, the compiler will automatically cast it at compile time when needed.

Changed to using constexpr. My goal was to replace hardcoded values with constants while being certain that it wouldn't have any side-effects. I wanted to avoid side effects related to how integer division works vs floats, so I kept the types the same.

Get rid of the int variant please.

BevapDin · Apr 14, 2018

BevapDin reviewed Apr 14, 2018

View changes

BevapDin · Apr 14, 2018

src/map.cpp

@@ -5695,17 +5695,24 @@ const visibility_variables &map::get_visibility_variables_cache() const {

 lit_level map::apparent_light_at( const tripoint &p, const visibility_variables &cache ) const {
    const int dist = rl_dist( g->u.pos(), p );
+
+    // Being outside of the world bubble overrides Clairvoyance.
+    if(p.x < 0 || p.y < 0) {


We have map::inbounds for this, which works with tripoint directly, checks the z component as well and checks for the upper bound as well.

It's a bad idea to have this check here, though.
apparent_light_at is used a lot and is one of the most expensive functions, according to profiler.

kevingranade · Apr 20, 2018

kevingranade reviewed Apr 20, 2018

View changes

kevingranade · Apr 20, 2018

src/game.cpp

@@ -7329,7 +7329,13 @@ void game::print_all_tile_info( const tripoint &lp, const catacurses::window &w_
                                const int last_line, bool draw_terrain_indicators,
                                const visibility_variables &cache )
 {
-    auto visibility = m.get_visibility( m.apparent_light_at( lp, cache ), cache );
+    visibility_type visibility;


how about:

visibility = VIS_HIDDEN if( g->map_ptr->inbounds( lp ) ) { visibility = m.get_visibility( m.apparent_light_at( lp, cache ), cache ); }

Just a styling thing (including the spaces around parentheses).

kevingranade · Apr 20, 2018

kevingranade reviewed Apr 20, 2018

View changes

kevingranade · Apr 20, 2018

src/game_constants.h

+constexpr int player_view_distance_int = player_view_distance;
+
+/** Distance to search for fishables from trap/fishing pole/etc */
+constexpr int fishing_distance = 60;


Please hoist this to a common shared header like iuse_actor.h, no need to make this visible to the entire game.

kevingranade · Apr 20, 2018

kevingranade reviewed Apr 20, 2018

View changes

kevingranade · Apr 20, 2018

src/game_constants.h

+constexpr int fishing_distance = 60;
+
+/** Light level to use for starting a fire */
+constexpr float min_light_level_for_firestarter = 60.0f;


Also move to iuse_actor.h, or move them both to iuse_constants.h, just not here.

kevingranade · Apr 20, 2018

kevingranade reviewed Apr 20, 2018

View changes

kevingranade · Apr 20, 2018

src/game_constants.h

@@ -88,6 +88,18 @@
 /** Maximum range at which ranged attacks can be executed */
 #define RANGE_HARD_CAP 60

+/** Player view distance FLOAT */
+constexpr float player_view_distance = 60.0f;


Please hoist to a header file shared by lightmap.cpp and player.cpp, map.h would work.

ZhilkinSerg · Jun 16, 2018

Feel free to reopen.

mrkybe added some commits Apr 5, 2018

reality bubble 2x size

3106747

remove unused constants, fix styling

c7028b2

default settings for constants

Loading status checks…

0e05f7c

mrkybe changed the title ~~Add constants for easily changing the size of the reality bubble at compile time.~~ [CR} Add constants for easily changing the size of the reality bubble at compile time. Apr 5, 2018

mrkybe changed the title ~~[CR} Add constants for easily changing the size of the reality bubble at compile time.~~ [CR] Add constants for easily changing the size of the reality bubble at compile time. Apr 5, 2018

revert incorrect changes

Loading status checks…

654d82c

mrkybe changed the title ~~[CR] Add constants for easily changing the size of the reality bubble at compile time.~~ [WIP][CR] Add constants for easily changing the size of the reality bubble at compile time. Apr 5, 2018

ZhilkinSerg added Code: Performance [C++] Code: Astyle / Optimization labels Apr 5, 2018

mrkybe added some commits Apr 17, 2018

Address requested changes

Loading status checks…

988a3b8

Use inbounds check instead.

Loading status checks…

358cf24

Move bounds check to print_all_tile_info.

Loading status checks…

10abb9e

Requested changes.

Loading status checks…

1f4fa33

ZhilkinSerg added the (S5 - OnHold / Stalled) label Jun 16, 2018

ZhilkinSerg closed this Jun 16, 2018

mrkybe referenced this pull request Sep 11, 2018
Merged
Refactor item temperature calculations #25509

CleverRaven/Cataclysm-DDA

Join GitHub today

[WIP][CR] Add constants for easily changing the size of the reality bubble at compile time. #23376

Conversation

mrkybe commented Apr 5, 2018 • edited

mrkybe added some commits Apr 5, 2018

mrkybe changed the title Add constants for easily changing the size of the reality bubble at compile time. [CR} Add constants for easily changing the size of the reality bubble at compile time. Apr 5, 2018

mrkybe changed the title [CR} Add constants for easily changing the size of the reality bubble at compile time. [CR] Add constants for easily changing the size of the reality bubble at compile time. Apr 5, 2018

kevingranade requested changes Apr 5, 2018 View changes

kevingranade left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mrkybe changed the title [CR] Add constants for easily changing the size of the reality bubble at compile time. [WIP][CR] Add constants for easily changing the size of the reality bubble at compile time. Apr 5, 2018

ZhilkinSerg added Code: Performance [C++] Code: Astyle / Optimization labels Apr 5, 2018

This comment has been minimized.

BevapDin commented Apr 5, 2018

Coolthulhu reviewed Apr 14, 2018 View changes

This comment has been minimized.

BevapDin reviewed Apr 14, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Apr 14, 2018 View changes

This comment has been minimized.

This comment has been minimized.

mrkybe added some commits Apr 17, 2018

kevingranade reviewed Apr 20, 2018 View changes

This comment has been minimized.

kevingranade reviewed Apr 20, 2018 View changes

This comment has been minimized.

kevingranade reviewed Apr 20, 2018 View changes

This comment has been minimized.

kevingranade reviewed Apr 20, 2018 View changes

This comment has been minimized.

ZhilkinSerg added the (S5 - OnHold / Stalled) label Jun 16, 2018

This comment has been minimized.

ZhilkinSerg commented Jun 16, 2018

ZhilkinSerg closed this Jun 16, 2018

mrkybe referenced this pull request Sep 11, 2018

mrkybe commented Apr 5, 2018 •

edited

kevingranade requested changes Apr 5, 2018

View changes

Coolthulhu reviewed Apr 14, 2018

View changes

BevapDin reviewed Apr 14, 2018

View changes

BevapDin reviewed Apr 14, 2018

View changes

kevingranade reviewed Apr 20, 2018

View changes

kevingranade reviewed Apr 20, 2018

View changes

kevingranade reviewed Apr 20, 2018

View changes

kevingranade reviewed Apr 20, 2018

View changes