-
Notifications
You must be signed in to change notification settings - Fork 793
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
Buyback functionality for Griswold and Adria #1069
base: master
Are you sure you want to change the base?
Conversation
Rebased and aligned with the changes in inv.cpp. It works throughout the test scenarios I could think of, but I would like to eliminate some code duplication. |
If you could make a PR that cleans things up and lays the foundation for changes like this with out changing current behavior that would be much appreciated and make this one easier to evaluate. |
Sorry what do you mean changing current behavior? The code duplication is within the code I added on. But I see this all over the file. |
I expected that you choose to have S_SBuyBackEnter() and void S_WBuyBackEnter() because that is how the current code structure expects things to be done. But if you can clean this up in your PR with out touching any existing code then you should probably do so. This PR changes over 2k lines of code so I expect that it relies on some refactoring which could be done separately. 1k change is what I consider a lot for a single PR, unless it's trivially moving things around. |
Never mind, it looks like this PR is actually only 250 lines of new code, the rest is changed white space that shouldn't have been changed. Try running clang-format on the code or get your editor to use the correct line endings. |
This will change again & reduce once I'm done with the cleanup in PR #1524. |
Coming back to this one, in a way I feel that maybe this feature addition isn't best suited for the binary but rather be targeted as a bundled lua mod? I do like the idea, however this can have impacts on gameplay. This feature feels more like turning Griswold into a pawn shop, where you sell your item and then you can go back later when you have some money and buy it back. While it's not a bad feature, I think it has more implications than solving the problem I believe it was created for, which is accidentally selling and item to a vendor that you didn't intend on selling with no way to recover it. I think a proper and better approach that would be suited for the binary would be a way to "favorite" items, which has been talked about. |
They are actually not comparable. Players can choose whether to use or abuse the buyback option. You cannot reasonably opt-in to the zero-durability mechanic without some pesky UX considerations in Multiplayer that are difficult to navigate. On that note, I think there are some pesky UX considerations with regard to favoriting items that you probably haven't examined. In all these cases, how do you prevent the user from accidentally selling their favorite items?
On the other hand, I believe buyback can be more readily compared with stash. They are both as-needed and therefore opt-in by definition. Lastly, it's worth noting that buyback abuse is incredibly situational and depends on the implementation. I feel its impact on gameplay is basically negligible, even compared to the zero-durability feature you mentioned. |
If you clear the buyback list after leaving town, that could meet the need of buying back something sold by accident (assuming you realize it at the time), without allowing Griswold to be treated as a pawn shop. |
Good point |
The way I did implement it back then, there was no save mechanic between games. So if you stop playing and start again, your sold items are gone. Buyback abuse would be pretty limited like that. With stash and the option of item pictures in the shop, the risk of accidentally selling is somewhat mitigated. A pawn shop should be canon: "I gotta pawn some of this stuff" ;) |
Ever accidentally sold anything and wished you could get it back?
I sure did.
This pull requests adds a the menu option "Buy back item" to Adria's and Griswold's stores.