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

Handled nowhere parent #75819

Merged
merged 2 commits into from
Aug 26, 2024
Merged

Conversation

PatrikLundell
Copy link
Contributor

Summary

None

Purpose of change

Fix #75792, i.e. item location load failure crashes saving.

Describe the solution

Handle an item_location parent of "nowhere" by making the item_location itself "nowhere", rather than trying to move the item to the garbage location of "nowhere".

Describe alternatives you've considered

Try to figure out if the original error handling is incorrect for other cases and if it's even possible for other cases.
It's safer to assume it can somehow happen for other cases, so the placement of the item on the ground is a valid strategy for those cases.

Testing

Loaded the bug report save, quick saved, saved & quit, loaded the save again.

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Aug 19, 2024
@mqrause
Copy link
Contributor

mqrause commented Aug 19, 2024

I have a feeling this is the wrong approach to fix that issue. Both the load and the save errors come from (de-)serializing an ACT_EAT_MENU activity of which there are multiple in the backlog of the save from the issue. I don't think there should be any, though, so something else broke the save before.

@PatrikLundell
Copy link
Contributor Author

I agree the change only handles the symptoms of an earlier error, and the error message reflects that.
However, I think it's still worthwhile to try to handle errors as gracefully as possible.

That said, it's far superior to fix the original error if it can be located, and while I think both changes should be made if possible, I'd go with fixing a root problem if only one thing is done.

I didn't even try to look at the horrible unreadable one liners the JSON files contain, both because of the mess and because I don't have much hope of finding anything useful.

Auto saving has a history of messing up things, and I wouldn't be shocked if you can get messed up saves if you're eating from the eating menu while it kicks in (which I guess these activities represent). However, testing it might be tricky unless you can somehow trigger auto saving manually while the PC is chewing.

Copy link
Contributor

@mqrause mqrause left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, identifying the real cause is something between hard and impossible. On the user end, a fairly easy fix is to delete the entire backlog. But that's not that good of a solution considering it happens somewhat regularly.

But looking at the deserialization and item_location default values in general, what mostly seems unsafe to me is using tripoint_min. That's simply not a valid tripoint to use anywhere and the ultimate cause of the crash. But if we have no valid tripoint, I think the item_location ultimately has to become nowhere.

src/item_location.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 20, 2024
@dseguin dseguin merged commit 8185064 into CleverRaven:master Aug 26, 2024
26 checks passed
@PatrikLundell PatrikLundell deleted the item_location branch August 26, 2024 08:09
Knut-Aage-Hofseth pushed a commit to Knut-Aage-Hofseth/Cataclysm-DDA that referenced this pull request Aug 29, 2024
* Handled nowhere parent

* Update src/item_location.cpp

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

---------

Co-authored-by: mqrause <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

segfault on save
3 participants