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

Buyback functionality for Griswold and Adria #1069

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

joewis
Copy link
Contributor

@joewis joewis commented Feb 28, 2021

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.

@AJenbo AJenbo added this to the 1.3.0 milestone Feb 28, 2021
Source/stores.cpp Outdated Show resolved Hide resolved
Source/stores.cpp Outdated Show resolved Hide resolved
@julealgon julealgon added the enhancement New feature or request label Apr 4, 2021
@joewis
Copy link
Contributor Author

joewis commented Apr 9, 2021

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.
void S_SBuyBackEnter() and void S_WBuyBackEnter() are completely identical except that I can't figure out how to figure out if I'm in the Griswold or Adria, which is needed for menu navigation only.

@AJenbo
Copy link
Member

AJenbo commented Apr 9, 2021

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.

@joewis
Copy link
Contributor Author

joewis commented Apr 9, 2021

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.

@AJenbo
Copy link
Member

AJenbo commented Apr 9, 2021

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.

@AJenbo
Copy link
Member

AJenbo commented Apr 9, 2021

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.

@joewis
Copy link
Contributor Author

joewis commented Apr 14, 2021

This will change again & reduce once I'm done with the cleanup in PR #1524.

@AJenbo AJenbo modified the milestones: 1.3.0, 1.4.0 Sep 6, 2021
@AJenbo AJenbo modified the milestones: 1.4.0, 1.5.0 Dec 17, 2021
@AJenbo AJenbo modified the milestones: 1.5.0, 1.6.0 Apr 8, 2023
@kphoenix137
Copy link
Collaborator

kphoenix137 commented Mar 13, 2024

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.
The gameplay implications from this can be compared to the PR that prevents items from being fully destroyed when hitting 0 durability, giving the chance to the player to repair them and make them usable again.

@StephenCWills
Copy link
Member

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.
The gameplay implications from this can be compared to the PR that prevents items from being fully destroyed when hitting 0 durability, giving the chance to the player to repair them and make them usable again.

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?

  • What if I drop a favorited item on the ground and pick it back up?
  • What if I hand over a favorited item to my buddy, he uses it for a while, then gives it back?
  • What if I put a favorited item in stash and transfer it to another character?
  • What if I simply thought my item was favorited, but it wasn't?

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.

@DakkJaniels
Copy link
Contributor

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.

@kphoenix137
Copy link
Collaborator

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

@joewis
Copy link
Contributor Author

joewis commented Mar 13, 2024

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" ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants