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 disappearing backpack during trade #3750

Merged
merged 2 commits into from
Nov 3, 2021
Merged

fix disappearing backpack during trade #3750

merged 2 commits into from
Nov 3, 2021

Conversation

Zbizu
Copy link
Contributor

@Zbizu Zbizu commented Oct 19, 2021

Changes Proposed

prevent a trade that would result in one of trade containers disappearing in situation when both players try to trade equipped backpack

expected result: the player who accepts the trade should see "there is not enough room" error

Issues addressed: #3742

Notes

needs testing

@EPuncker EPuncker added the needs-testing Needs testing with screenshots label Oct 19, 2021
@EPuncker EPuncker linked an issue Oct 19, 2021 that may be closed by this pull request
2 tasks
@ramon-bernardo
Copy link
Contributor

That solves it, but shouldn't we find bug in internalMoveItem?

@Zbizu
Copy link
Contributor Author

Zbizu commented Oct 19, 2021

That solves it, but shouldn't we find bug in internalMoveItem?

Adding item to container normally terminates the trade (or at least it should). In this situation trade was already completing so it couldn't terminate. The item moves by cloning and deleting itself, but the other item was added before the container deleted itself so the item was lost. I believe there won't be any more problems after very specific situation which is trade got resolved.

@EPuncker EPuncker merged commit 0d1a8fd into otland:master Nov 3, 2021
@Zbizu Zbizu deleted the patch-3 branch November 16, 2021 22:38
Znote pushed a commit to Znote/forgottenserver that referenced this pull request Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix needs-testing Needs testing with screenshots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yet another trade bug
3 participants