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

fix loading of traps #74928

Merged
merged 1 commit into from
Jul 7, 2024
Merged

fix loading of traps #74928

merged 1 commit into from
Jul 7, 2024

Conversation

PatrikLundell
Copy link
Contributor

Summary

None

Purpose of change

Fix #74926, i.e. failure to read saves with traps

Describe the solution

Check whether the optional string value is null before saving it on load.

Describe alternatives you've considered

Leave it to someone who actually knows how it's supposed to work.

Testing

Loaded the bugged save successfully.

Additional context

Note that I don't know how or why there is an optional string there, and I haven't tested any situation where there actually is one (I don't even know if any such strings exist). Thus, this should be reviewed by someone who actually knows what this is supposed to do, ideally the same person who made the changes that causes the saves to break in the first place.

@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 BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jul 6, 2024
@RenechCDDA
Copy link
Member

Note that I don't know how or why there is an optional string there, and I haven't tested any situation where there actually is one (I don't even know if any such strings exist). Thus, this should be reviewed by someone who actually knows what this is supposed to do, ideally the same person who made the changes that causes the saves to break in the first place.

Oops hi that's me.

Didn't realize the failures were trap-related or else I'd have known without checking this PR...

I don't see anything wrong with this approach if it is keeping the chunks from being corrupted. Worst case scenario this breaks the traps which use that data but I specifically gave them error handling to prevent crashing if the item data was missing.

@PatrikLundell
Copy link
Contributor Author

There is code checking for missing data, but the optional thingie either produces a null entry or a string, so there will never be empty data there. Thus, that check does nothing with saves made with current code.
However, if this is something that was added, then it ought to handle the case that an older save doesn't have the data.

I don't know if this check is to handle legacy saves, a failed attempt to handle the null case, or both, so that check remains for now: at worst it's redundant code.

What I really would need help with is to find a trap that does provide this string so the modified code can be tested to see it does what it ought to do in those cases (since the bug report save provides a null entry case, so that's been tested).

@RenechCDDA
Copy link
Member

The code was bundled in #73867, I think they only added traps of this type to XE.

@PatrikLundell
Copy link
Contributor Author

Thanks. Then I think this PR is ready for review, as I won't try to figure out how XE works and think the code will work fine.

@Maleclypse
Copy link
Member

Ok I've compiled and tested the XE traps with this . They worked. I discovered prototype cyborgs are ROBOT species lol I'll be opening a PR to fix that. I guess since they predate cyborg species.

@Maleclypse Maleclypse merged commit 973f6a9 into CleverRaven:master Jul 7, 2024
22 of 28 checks passed
@PatrikLundell PatrikLundell deleted the fix_trap branch July 7, 2024 17:45
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.

Save game fails to load areas with constructed firewood source
3 participants