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

Consolidated buy and sell functions of Smith, Witch and Healer #1524

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joewis
Copy link
Contributor

@joewis joewis commented Apr 14, 2021

Introducing S_Scroll, S_StartBuy, S_BuyEnter, S_BuItem function and switch between the respective Item Arrays rather than duplicating the functions for each store. Removed the duplicate functions.
Wirt is not included as his item is not stored in an array (yet).

This approach works for sell functionality as well, I leave that for a separate commit.

Source/stores.cpp Outdated Show resolved Hide resolved
AJenbo
AJenbo previously requested changes Apr 14, 2021
Copy link
Member

@AJenbo AJenbo left a comment

Choose a reason for hiding this comment

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

A good first step in cleaning up this file. But please apply the correct code style.

@joewis
Copy link
Contributor Author

joewis commented Apr 15, 2021

Submitted for sell functions.
I'll apply clang-format at the end - it appears to cause lots of whitespace changes.

@glebm
Copy link
Collaborator

glebm commented Apr 15, 2021

@joewis Please only apply clang-format to change lines btw

glebm
glebm previously requested changes Apr 15, 2021
Source/stores.cpp Outdated Show resolved Hide resolved
@joewis joewis changed the title Consolidated buy functions of Smith, Witch and Healer Consolidated buy and sell functions of Smith, Witch and Healer Apr 15, 2021
@joewis joewis requested review from glebm and AJenbo April 15, 2021 05:37
@joewis
Copy link
Contributor Author

joewis commented Apr 15, 2021

Patching Wirt into the same store logic proves more tricky than expected, of all reasons, because the item in the buy dialogue is offset by 1 item. I leave him be for now.

Source/stores.cpp Outdated Show resolved Hide resolved
@AJenbo AJenbo added this to the 1.3.0 milestone May 2, 2021
@AJenbo AJenbo modified the milestones: 1.3.0, 1.4.0 Oct 7, 2021
Source/stores.cpp Outdated Show resolved Hide resolved
Source/items.cpp Outdated Show resolved Hide resolved
@AJenbo
Copy link
Member

AJenbo commented Dec 12, 2021

@obligaron I fixed some issues when rebasing and cleaned up some related code with how stores are refreshed. I actually think it fixes a vanilla bug where after drinking enough exiles to use an item it would still appear as red in the store, but haven't confirmed if that was the case.

I may have introduced some new issues related to the line number logic since stextsel = would be 14/16/18 for different stores that visually appears the same. I don't quite understand why and I think it doesn't really matter as that part should just be discarded in the next rewrite of this code.

I'll try and also get the boy shop merged with the other vendors so that it's all combined.

Copy link
Contributor

@obligaron obligaron left a comment

Choose a reason for hiding this comment

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

Didn't finished reviewing yet but wanted to share the first comments.
Before this pr I didn't looked at the store logic. But now I see why the old code needs refactoring. 😟
Good to see that someone works on this. 👍🙂

Source/stores.cpp Show resolved Hide resolved
Source/stores.cpp Outdated Show resolved Hide resolved
Source/stores.cpp Outdated Show resolved Hide resolved
Source/stores.cpp Outdated Show resolved Hide resolved
@joewis
Copy link
Contributor Author

joewis commented Dec 13, 2021

Adding to the list: Cain now crashes with segfault when attempting to identify items.

@AJenbo AJenbo removed this from the 1.5.0 milestone Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants