CleverRaven/Cataclysm-DDA — pull request #9749 by BevapDin (2014-10-30 17:04:04+00:00)
Title: Replace pointless and dangerous access to global objects

While testing the changes to the defense mode, I had some crashes, here is the backtrace:
```
#0 ...
#4 0x084d8c86 in submap::delete_signage (this=0x0, x=8, y=10) at src/mapdata.h:408
#5 0x084c4ae5 in map::delete_signage (this=0xb70bb0ec, x=8, y=10) at src/map.cpp:2585
#6 0x084be962 in map::bash (this=0xbfbc3028, x=8, y=10, str=999, silent=true, destroy=true, bashing_vehicle=0x0) at src/map.cpp:1887
#7 0x084bfdff in map::destroy_furn (this=0xbfbc3028, x=8, y=10, silent=true) at src/map.cpp:2075
#8 0x084bac08 in map::make_rubble (this=0xbfbc3028, x=8, y=10, rubble_type=4, items=true, floor_type=2, overwrite=false) at src/map.cpp:1479
#9 0x08340b2b in map::add_extra (this=0xbfbc3028, type=mx_helicopter) at src/mapgen.cpp:12895
#10 0x082a49f1 in map::generate (this=0xbfbc3028, x=90, y=88, z=0, turn=7200) at src/mapgen.cpp:96
#11 0x086c2ddf in defense_game::init_map (this=0x8f85f18) at src/defense.cpp:287
#12 0x086c1c9b in defense_game::init (this=0x8f85f18) at src/defense.cpp:91
#13 0x0827c763 in game::opening_screen (this=0xb70bb008) at src/main_menu.cpp:596
#14 0x085f0a5e in main (argc=0, argv=0xbfc18268) at src/main.cpp:287
```
See the change from this=0xbfbc3028 (the tinymap) to this=0xb70bb0ec (`g->m`) - that's when it crashes because at this point the game map `g->m` is not yet initialized. Instead of calling **this**->delete_signage, it calls **g->m**.delete_signage - a completely different object with a complete different local coordinate system.

I found more of those pointless references to the global `g->m` or `g->u` objects, where the local object (or `this`) are just as good. And actually they are better. I known that npc are very limited in what they do right now and probably won't call many of the changed function, but when calling `some_player->activate_mutation(...)`, I expect this to *not* affect the player (unless `some_player == &g->u`), I expect it to affect `some_player`.

Similar: I have a character whose memorial says "Put out the fire." several times although the character was never burning. It were several npc that the character accidentally set on fire and *they* put out the fire, but the code in creature.cpp add the memorial log to `g->u` instead of `this` creature.

It's even worse in iexamine.cpp. There are calls on `m` (the parameter of the iexamine functions), and two *pixels* below there are calls on `g->m`. What is that supposed to mean? Why are you changing the map you got as parameter *and* the global map?

This is not just a style issue, as the backtrace above show, this can lead to crashes and subtile bugs that are very hard to reproduce (if at all). The crash above happened only because a map extra with rubble has spawned, how likely is that? When starting a normal game, this is even more unlikely: defense mode generates the initially used submaps itself, during that time `g->m` is completely uninitialized. The normal game mode lets the map generate the submaps as needed, and because the iteration starts at grid(0,0), those submaps are generated and than available in `g->m`. In other words, when the above extra had been placed on the second or later submaps, the `map::delete_signage (this=0xb70bb0ec, x=8, y=10)` would work fine because the submap that contains (8,10) had already been loaded.

Also fixed some things in mapgen:
Don't place the monster via `g->add_zombie` - the generated map might be far away from the player (generate to be destroyed by a nuke, or to place a mission item etc.).
Same reasoning for the other change, g->levz is not necessarily what you want to test there.