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

Fixed remove items from stash on create market offer #474

Merged
merged 20 commits into from
Oct 5, 2022

Conversation

dudantas
Copy link
Contributor

@dudantas dudantas commented Aug 1, 2022

Resolves #470

Created the Player::requestLockerItems function to avoid code repetition, it can be used in ProtocolGame and Game class improved the way locker items were read and stored

Game::getMarketItemList function has been replaced by Player::requestLockerItems
Small modification in ProtocolGame::sendMarketEnter function to avoid code repetition, it will read Player::requestLockerItems function

Added some error logs for market functions so that we can identify where the problem was, in future tests. And if there is a problem during the processing of the offer, it will not be created, making it impossible to have an item or money clone.

Added some logs (debug) to make it easier to find problems in the future

Copy link

@CleberMoreira CleberMoreira left a comment

Choose a reason for hiding this comment

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

Tested with low and very high values like 20k+ items at once and it was smooth! 110% approved.

@gerotib
Copy link
Contributor

gerotib commented Aug 2, 2022

When we have certain item both in depot and stash, and create market offer, it will remove the amount from both depot and stash. It should prioritize either depot or stash, so no double amount of items is being removed.

@dudantas
Copy link
Contributor Author

dudantas commented Aug 2, 2022

When we have certain item both in depot and stash, and create market offer, it will remove the amount from both depot and stash. It should prioritize either depot or stash, so no double amount of items is being removed.

Ok I'll check.

@dudantas
Copy link
Contributor Author

@gerotib the bugs have been corrected. Or the only thing that was left was related to item count. When I have more than 65k items stored and try to raise an offer, it won't create. I'm investigating.

@dudantas dudantas force-pushed the fix-remove-stash-item-create-market-offer branch 3 times, most recently from fd846c2 to b12f4d2 Compare August 17, 2022 14:58
@sonarcloud
Copy link

sonarcloud bot commented Aug 17, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@github-actions
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions bot added the Stale No activity label Sep 17, 2022
Created the Player::requestLockerItems function to avoid code repetition, it can be used in ProtocolGame and Game
improved the way locker items were read and stored

Game::getMarketItemList function has been replaced by Player::requestLockerItems
Small modification in ProtocolGame::sendMarketEnter function to avoid code repetition, it will read Player::requestLockerItems function

Added some logs (debug) to make it easier to find problems in the future
@dudantas
Copy link
Contributor Author

@gerotib and @CleberMoreira can test again, please?

@github-actions github-actions bot removed the Stale No activity label Sep 26, 2022
@guispiller
Copy link
Contributor

Found a small issue there, this is how to reproduce:
1 - put 94k of the same item on the stash (example rope belt)
2 - create a sell offer of 64k rope belts. (you'll still have 30k on stash)
3 - cancel your sell offer of rope belts. (you'll have 64k on mailbox and 30k on stash)
4 - create new 64k sell offer on market. All items will be removed from stash and from mailbox (you lose 30k items)

Also found another bug, but i think it's on main too:
if you press to stow items from mailbox, it's checking and stowing items from player's bp instead of mailbox.

@dudantas dudantas force-pushed the fix-remove-stash-item-create-market-offer branch from 09fa0cd to 7f6cb33 Compare September 29, 2022 22:42
Extracted remove items in own function
Added some checks for avoid item cloning/duplication
@dudantas dudantas force-pushed the fix-remove-stash-item-create-market-offer branch from 7f6cb33 to 89a8d66 Compare September 29, 2022 23:07
@guispiller
Copy link
Contributor

After the last commits, the bugs I commented were solved and also the stow function of mailbox were implemented.

I just noticed that, when you right click on a stackable item inside the mailbox, and select "stow all items of this type", the function is searching items to stow at the players backpack, not at the mailbox.

@dudantas dudantas force-pushed the fix-remove-stash-item-create-market-offer branch from 48b01cd to 823c002 Compare October 3, 2022 23:35
@dudantas
Copy link
Contributor Author

dudantas commented Oct 3, 2022

After the last commits, the bugs I commented were solved and also the stow function of mailbox were implemented.

I just noticed that, when you right click on a stackable item inside the mailbox, and select "stow all items of this type", the function is searching items to stow at the players backpack, not at the mailbox.

Thanks for reporting the issues. Are fixed!

@sonarcloud
Copy link

sonarcloud bot commented Oct 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 13 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@dudantas dudantas changed the title Fix remove items from stash on create market offer Fixed remove items from stash on create market offer Oct 5, 2022
@dudantas dudantas merged commit 4e49d5b into main Oct 5, 2022
@dudantas dudantas deleted the fix-remove-stash-item-create-market-offer branch October 5, 2022 01:02
@gerotib
Copy link
Contributor

gerotib commented Oct 5, 2022

@dudantas I have found an issue in removeOfferItems function, item is not being removed while creating market offer if we have some stackable items in inbox. It takes the count from the stackable ones inside inbox instead of actual item id we try to sell. Its possible to clone items this way.
https://gfycat.com/smallhighbonobo

@dudantas
Copy link
Contributor Author

dudantas commented Oct 5, 2022

@dudantas I have found an issue in removeOfferItems function, item is not being removed while creating market offer if we have some stackable items in inbox. It takes the count from the stackable ones inside inbox instead of actual item id we try to sell. Its possible to clone items this way. https://gfycat.com/smallhighbonobo

@gerotib It's been ten days since I asked to test again. Now, I've merged this pr. I'll test it later and let you know. You can open an issue?

@gerotib
Copy link
Contributor

gerotib commented Oct 5, 2022

@dudantas sure, I just got back and didn't see this earlier

luan pushed a commit that referenced this pull request Jul 11, 2023
#474)

Resolves #470

• Created the Player::requestLockerItems function to avoid code repetition, it can be used in ProtocolGame and Game class improved the way locker items were read and stored
• Game::getMarketItemList function has been replaced by Player::requestLockerItems
• Small modification in ProtocolGame::sendMarketEnter function to avoid code repetition, it will read Player::requestLockerItems function

•Added some error logs for market functions so that we can identify where the problem was, in future tests. And if there is a problem during the processing of the offer, it will not be created, making it impossible to have an item or money clone.

• Added some logs (debug) to make it easier to find problems in the future
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: PRs Done
Development

Successfully merging this pull request may close these issues.

Market offer is not being created while selecting item from stash
5 participants