Make compared have a copy of the inventory_entry rather than a pointer to fix segfault when using multiple filters. #36139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KorGgenT
added
<Bugfix>
|
I'm getting a segfault. Steps:
Cannot reproduce on Windows with MS VS. |
|
I haven't been able to replicate this problem on MS VS19. The stack trace points to the item disappearing on linux for some reason. I did try a completely different approach of making the entries_unfiltered the master list and entries pointers to the inventory_entry's in that list. Ended up changing a ton of things and completely broke inventory as well as compare. I know KorGgenT is doing heavy work in this area at the moment so am going to pause doing any more work on this PR to see how those changes shake out. |
|
@ishtatann you got it backwards, the stuff i'm working on is low priority compared to this being a release blocker. |
|
Save a copy of the first selection before set_filter is called. Restore the first selection after set_filter finishes. I'm hoping this will fix the crash people were seeing on the linux side. |
…r to fix segfault when using multiple filters.
Restore the selection after set_filter has changed all the entries.
ishtatann
force-pushed
the
ishta-bugfix-34046
branch
from
590c3df to
4b923fe
Compare
|
I can still reproduce the crash with these changes. Built with |
jbytheway
added a commit
to jbytheway/Cataclysm-DDA
that referenced
this pull request
Jan 1, 2020
Fixes CleverRaven#34046. Using the search filter causes all the inventory_entry objects to be recreated, so it's not safe to store a pointer to them. A previous attempt to solve this (CleverRaven#36139) stored copies of the inventory_entry objects, but that was also unsafe. This attempt instead stores just pointers to the underlying items. This seems to prevent the crashes. The behaviour of the window is still not always exactly correct, because using the filter will violate the invariant that the items in the compared vector correspond to the selected entries. I don't see a way to fix that in general -- sometimes the compared item is filtered out, and thus you can't possibly select the entry (because it doesn't exist). We might want to add something that does its best to select the right entries when the filter changes, but this is a sufficiently niche use case that I'm not sure it's worth the complexity. I've settled for just fixing the crash for now.
|
I've posted an alternate attempted fix at #36606. Testing by others welcome. |
jbytheway
added a commit
to jbytheway/Cataclysm-DDA
that referenced
this pull request
Jan 1, 2020
Fixes CleverRaven#34046. Using the search filter causes all the inventory_entry objects to be recreated, so it's not safe to store a pointer to them. A previous attempt to solve this (CleverRaven#36139) stored copies of the inventory_entry objects, but that was also unsafe. This attempt instead stores just pointers to the underlying items. This seems to prevent the crashes. The behaviour of the window is still not always exactly correct, because using the filter will violate the invariant that the items in the compared vector correspond to the selected entries. I don't see a way to fix that in general -- sometimes the compared item is filtered out, and thus you can't possibly select the entry (because it doesn't exist). We might want to add something that does its best to select the right entries when the filter changes, but this is a sufficiently niche use case that I'm not sure it's worth the complexity. I've settled for just fixing the crash for now.
kevingranade
pushed a commit
that referenced
this pull request
Jan 2, 2020
Fixes #34046. Using the search filter causes all the inventory_entry objects to be recreated, so it's not safe to store a pointer to them. A previous attempt to solve this (#36139) stored copies of the inventory_entry objects, but that was also unsafe. This attempt instead stores just pointers to the underlying items. This seems to prevent the crashes. The behaviour of the window is still not always exactly correct, because using the filter will violate the invariant that the items in the compared vector correspond to the selected entries. I don't see a way to fix that in general -- sometimes the compared item is filtered out, and thus you can't possibly select the entry (because it doesn't exist). We might want to add something that does its best to select the right entries when the filter changes, but this is a sufficiently niche use case that I'm not sure it's worth the complexity. I've settled for just fixing the crash for now.
|
Obsoleted by #36606 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY: Bugfixes "Make compared have a copy of the inventory_entry rather than a pointer to fix segfault when using multiple filters"Purpose of change
Fixes #34046
Describe the solution
Converted the compared vector to be a copy of the inventory_entry instead of a pointer.
Describe alternatives you've considered
I considered turning compared into an item* but that turned into something very invasive.
Testing
Verified that comparison works with and without filters after the change.