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

Cleanup Store code #7424

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

Conversation

kphoenix137
Copy link
Collaborator

No description provided.

@kphoenix137
Copy link
Collaborator Author

I think I'll submit a PR separately that changes the variable names found in stores.cpp so this PR is much easier to review.

@AJenbo
Copy link
Member

AJenbo commented Sep 19, 2024

I think you should, this one already has conflicts despite being the latest

@AJenbo
Copy link
Member

AJenbo commented Sep 20, 2024

Do your self a favor and merge master rather then rebase on it with the commits you have.

@kphoenix137 kphoenix137 force-pushed the stores-cleanup branch 3 times, most recently from 95cd85b to 3e5daee Compare September 26, 2024 21:15
@kphoenix137 kphoenix137 marked this pull request as ready for review October 10, 2024 02:33
@kphoenix137 kphoenix137 mentioned this pull request Oct 11, 2024
Comment on lines +873 to +881
for (int i = 0; i < giNumberOfSmithPremiumItems; ++i) {
LoadAndValidateItemData(file, Blacksmith.items[i]);
}

// Remove any empty/null items from the vector after loading
Blacksmith.items.erase(std::remove_if(Blacksmith.items.begin(), Blacksmith.items.end(), [](const Item &item) {
return item._itype == ItemType::None;
}),
Blacksmith.items.end());
Copy link
Member

Choose a reason for hiding this comment

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

Could there potentially be items that where not nulled and which where previously truncated by PremiumItemCount? Maybe we should just load the given number and then fast forward the point (giNumberOfSmithPremiumItems - PremiumItemCount) * sizeof(Item)

@@ -6,69 +6,95 @@ using namespace devilution;

namespace {

TEST(Stores, AddStoreHoldRepair_magic)
// Helper function to reset the playerItems vector before each test
Copy link
Member

Choose a reason for hiding this comment

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

Use doc blocks for function comments:

/**
 * @brief Helper function to reset the playerItems vector before each test.
 */

Comment on lines +43 to +44
// Call the filtering function to remove non-repairable items
Test_FilterRepairableItems();
Copy link
Member

Choose a reason for hiding this comment

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

This tests that the Test_FilterRepairableItems() function works, but we would want to know if the game code works, not if the tests work...

void InitStores();

/** Spawns items sold by vendors, including premium items sold by Griswold and Wirt. */
/* Spawns items sold by vendors, including premium items sold by Griswold and Wirt. */
Copy link
Member

Choose a reason for hiding this comment

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

Use short form doc block for single line function comments.

Suggested change
/* Spawns items sold by vendors, including premium items sold by Griswold and Wirt. */
/** Spawns items sold by vendors, including premium items sold by Griswold and Wirt. */


/** Clears premium items sold by Griswold and Wirt. */
/* Clears premium items sold by Griswold and Wirt. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* Clears premium items sold by Griswold and Wirt. */
/** Clears premium items sold by Griswold and Wirt. */

Comment on lines +83 to +85
Item *itemPtr; // Pointer to the original item
ItemLocation location; // Location in the player's inventory (Inventory, Belt, or Body)
int index; // Index in the corresponding array
Copy link
Member

Choose a reason for hiding this comment

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

Please use doc blocks instead of inline comments so that the documentation is picked up properly by tools.

}

void SpawnHealer(int lvl)
{
constexpr size_t PinnedItemCount = 2;
constexpr std::array<_item_indexes, PinnedItemCount + 1> PinnedItemTypes = { IDI_HEAL, IDI_FULLHEAL, IDI_RESURRECT };
const auto itemCount = static_cast<size_t>(RandomIntBetween(10, gbIsHellfire ? 19 : 17));
const size_t itemCount = static_cast<size_t>(RandomIntBetween(10, gbIsHellfire ? NumHealerItemsHf : NumHealerItems));
Copy link
Member

Choose a reason for hiding this comment

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

You can omit size_t here since there is a cast where the type is explicit.

}

SortVendor(WitchItems + PinnedItemCount);
// Sort the vendor's inventory, keeping pinned items in place
Copy link
Member

Choose a reason for hiding this comment

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

This should be a comment on SortVendor()


if (CurrentItemIndex == 0) {
HasScrollbar = false;
// FIXME: Put in anonymous namespace
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to address the FIXME parts of the code before we merge it.

Comment on lines +117 to +123
const std::string SmithMenuHeader = "Welcome to the\n\nBlacksmith's shop";

const StoreMenuOption SmithMenuOptions[] = {
{ TalkID::Gossip, fmt::format("Talk to {:s}", Blacksmith.name) },
{ TalkID::BasicBuy, "Buy basic items" },
{ TalkID::Buy, "Buy premium items" },
{ TalkID::Sell, "Sell items" },
Copy link
Member

Choose a reason for hiding this comment

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

All of these string needs to be exposed for translation, since they are mostly static global you should wrap them in the fake translation function N_(), if you don't they will all be stripped from the language next time someone want to update a translation.


const std::string HealerMenuHeader = "Welcome to the\n\nHealer's home";

const StoreMenuOption HealerMenuOptions[] = {
Copy link
Member

@AJenbo AJenbo Nov 23, 2024

Choose a reason for hiding this comment

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

Having each store statically hard coded doesn't feel like a good direction. It would probably be better to populate a single structure with the relevant store, that would also make it feasible to make custom stores at runtime in mods.

This would also let you avoid the issue of having to wrap static strings in the fake translation function since you can perform the translation where the string is defined.


void SetActiveStore(TalkID talkId)
{
OldActiveStore = ActiveStore;
Copy link
Member

Choose a reason for hiding this comment

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

Old as in Previous?

Comment on lines +259 to +262
if (itemCount <= ItemLineSpace)
return false;

return true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (itemCount <= ItemLineSpace)
return false;
return true;
return itemCount > ItemLineSpace;

{
int itemCount = GetItemCount(ActiveStore);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int itemCount = GetItemCount(ActiveStore);
const int itemCount = GetItemCount(ActiveStore);

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.

2 participants