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

Make copy of item to be thrown - fix throwing bugs #36296

Merged
merged 4 commits into from Dec 22, 2019
Merged

Make copy of item to be thrown - fix throwing bugs #36296

merged 4 commits into from Dec 22, 2019

Conversation

ghost
Copy link

@ghost ghost commented Dec 20, 2019

Summary

SUMMARY: Bugfixes "Make copy of item to be thrown - fix throwing bugs"

Purpose of change

Fixes #36271
Fixes #36239
Fixes #36293

Describe the solution

#36106 changed how throwing works, unfortunately throwing did some sneaky stuff with copies that broke after that PR.

the throw function made a copy, and manipulated charges on that, and deleted the original.
The new function created a reference to the original, and kept the code that set the ( should be copy ) charges to 1, and then deleted it if it was the last charge or not stackable.

What ive done is create a copy again, send that to the ranged.cpp throwing function, and set the copies charges to 1, and subtract the originals charges appropiately. and delete the original if need be. ( but only after a copy has been sent to ranged.cpp )

Describe alternatives you've considered

N/A

Testing

new character, spawn 10 rocks, throw them all, charges reduce properly, thrown drops are the right stack size, and thre last rock is thrown and is deposited on the map, and no rocks left in inventory.

Additional context

N/A

src/avatar_action.cpp Outdated Show resolved Hide resolved
@ChucklesTheBeard
Copy link
Contributor

ChucklesTheBeard commented Dec 21, 2019

Does not close #36271 - Throwing directly from inventory without manually wielding first still segfaults.

fresh crash.log

src/avatar_action.cpp Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Dec 22, 2019

Does not close #36271 - Throwing directly from inventory without manually wielding first still segfaults.
fresh crash.log

Im throwing from inventory with this PR applied, and its not segfaulting.... you applied this PR yes? and what are the exact repro steps?

@ChucklesTheBeard
Copy link
Contributor

ChucklesTheBeard commented Dec 22, 2019

Steps to reproduce are the same. I thought I may had screwed up the merge or build flags somehow so I went ahead and re-ran by doing:

$ git checkout f1c09f9e76
$ make clean
$ make -j12
$ ./cataclysm --userdir ~/<doesn't matter>

g++ version: g++ (GCC) 9.2.0
kernel version: 5.4.5-arch1-1
arch: x86_64

  1. Create new world.
  2. Start new game: wilderness scenario / field location.
  3. Wish for 1 rock.
  4. [t]hrow
  5. [m]rock (1)
    SIGSEGV

crash.log

This save skips steps 1-3. Are you unable to reproduce the crash with that save?

I'm unable to reliably reproduce the issue in the default evacuee start, but wilderness+field starts reproduce the issue reliably.

edit: re-running with 78568ae, standby...

@ghost
Copy link
Author

ghost commented Dec 22, 2019

I could've got very lucky with the invalidation and it accessed same memory, but I threw rocks over and over and over again and no crash... but yeah with latest commit that shouldnt be a risk anymore ( hopefully )

How weird that default start dosnt repro, but wilderness does....

@ghost
Copy link
Author

ghost commented Dec 22, 2019

Yeah after latest commit, your attached save, throwing works ok, no crash, for me.

@ChucklesTheBeard
Copy link
Contributor

Yep, after 78568ae, it no longer crashes! 🎉

@kevingranade kevingranade merged commit f2f2bbf into CleverRaven:master Dec 22, 2019
@ghost ghost deleted the throw_fix branch January 17, 2020 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants