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

Misc bugfixes from mainline #167

Merged
merged 8 commits into from
Oct 31, 2020
Merged

Misc bugfixes from mainline #167

merged 8 commits into from
Oct 31, 2020

Conversation

olanti-p
Copy link
Contributor

@olanti-p olanti-p commented Oct 30, 2020

Summary

SUMMARY: Bugfixes "Port various bugfixes from DDA"

Purpose of change

Fix bugs by not inventing the bicycle

Describe the solution

Cherry-pick:
CleverRaven#39918
CleverRaven#40302
CleverRaven#40288
CleverRaven#40313
CleverRaven#40296
CleverRaven#40381
CleverRaven#40534

Describe alternatives you've considered

Adding more/less fixes in this PR, but 7 looks like a nice number.

Testing

There were no substantial conflicts, and no mentions of any regressions, so the fixes should work as is.
Just to be sure, got an NPC to undergo tentacle dog attack.
Also, favorited a bunch of items - letters hjkl were skipped.

Additional context

I'm not opposed to leaving any of these out if needed.

Aloxaf and others added 8 commits October 30, 2020 23:25
* Use radius, not diameter

* Update src/vehicle_move.cpp

Co-authored-by: ZhilkinSerg <[email protected]>

Co-authored-by: Kevin Granade <[email protected]>
Co-authored-by: ZhilkinSerg <[email protected]>
The function `are_requirements_nearby` accesses `player::backlog::front()`, which causes UB when the `backlog` list is empty.

This is usually checked by the caller, but there is at least one way to reach this function without that check:

```
#2  0x000000000083dfaa in string_id<activity_type>::operator== (rhs=..., this=0xdf1b460) at src/player_activity.h:93
#3  are_requirements_nearby (loot_spots=std::vector of length 21, capacity 32 = {...}, needed_things=..., p=..., activity_to_restore=..., in_loot_zones=true, src_loc=...)
    at src/activity_item_handling.cpp:1113
#4  0x0000000000843a46 in generic_multi_activity_check_requirement (p=..., act_id=..., act_info=..., src=..., src_loc=...,
    src_set=std::unordered_set with 1 element = {...}, check_only=false) at src/activity_item_handling.cpp:2676
#5  0x0000000000852813 in generic_multi_activity_handler (act=..., p=..., check_only=check_only@entry=false) at src/activity_item_handling.cpp:2885
#6  0x0000000000800896 in activity_handlers::multiple_butcher_do_turn (act=<optimized out>, p=<optimized out>) at src/activity_handlers.cpp:3822
#7  0x000000000082b248 in std::_Function_handler<void (player_activity*, player*), void (*)(player_activity*, player*)>::_M_invoke(std::_Any_data const&, player_activity*&&, player*&&) (__functor=..., __args#0=<optimized out>, __args#1=<optimized out>) at /usr/include/c++/8/bits/std_function.h:88
#8  0x00000000008630e3 in std::function<void (player_activity*, player*)>::operator()(player_activity*, player*) const (this=<optimized out>, __args#0=<optimized out>,
    __args#0@entry=0xdf1b330, __args#1=<optimized out>, __args#1@entry=0xdf1ae50) at /usr/include/c++/8/bits/std_function.h:260
#9  0x0000000000860306 in activity_type::call_do_turn (this=0x2c3c930, act=act@entry=0xdf1b330, p=p@entry=0xdf1ae50) at src/activity_type.cpp:118
#10 0x00000000014a968b in player_activity::do_turn (this=this@entry=0xdf1b330, p=...) at src/player_activity.cpp:237
#11 0x00000000013092a4 in npc::do_player_activity (this=this@entry=0xdf1ae50) at src/npcmove.cpp:3299
#12 0x0000000001322a07 in npc::execute_action (this=this@entry=0xdf1ae50, action=<optimized out>, action@entry=npc_player_activity) at src/npcmove.cpp:1237
#13 0x000000000132690a in npc::move (this=this@entry=0xdf1ae50) at src/npcmove.cpp:907
```

This adds a simple check within the function.
`map_cursor` is used in `item_location`, which in turn is used by the `player_activity`, which in turn is used by NPCs. So if you ask an NPC to butcher something, it will move to the corpse and "wait" there (won't move until the butchering is done). This stores are references to the corpse via an `item_location` (and the contained `map_cursor`), which means it stores the **local** coordinates of the corpse. If you move far enough away, so the map shifts, those coordinates are now pointing to a different location.

Consequently, when the butchering finishes, it will try to remove the corpse from the wrong place.

I fixed this with the smallest amount of code changes possible. But it's not a good fix. Better would be to access the `tripoint` via dedicated getters.

This may also fix some other bugs that display a "Tried to remove item from object which did not contain it" message.
@Coolthulhu Coolthulhu merged commit dbc190d into cataclysmbnteam:upload Oct 31, 2020
@Coolthulhu
Copy link
Member

Mass fixes like this look OK, as long as the commit messages are descriptive.
I see you squashed some of them, so it's all fine.

@olanti-p olanti-p deleted the misc-fixes branch October 31, 2020 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants