Refactor and Unify Vendor Item Generation for Premium and Boy Items

[kphoenix137]

Overview

This PR refactors the vendor item generation logic in Diablo/Hellfire to simplify and consolidate common validation and generation routines for both premium items and boy items. The changes aim to improve code clarity, reduce duplication, and ensure consistency across different item types.

Key Changes

• Helper Function Extraction:

  • GeneratePremiumItem / GenerateBoyItem:
    • Encapsulate RNG advancement, seeding, attribute assignment, and bonus application into dedicated helper functions.
    • Streamlines the main SpawnOnePremium and SpawnBoy functions.
  • IsVendorItemValid:
    • A unified validation function that applies item value and stat requirements.
    • Supports a "strict" phase with additional checks (including hero class restrictions) in Hellfire mode.
  • IsItemTypeAllowedForClass:
    • Shared function that checks whether a given ItemType is allowed for a specific hero class.
    • Replaces hacky modifications (using INT_MAX) with an explicit boolean check.

• Refactored SpawnOnePremium and SpawnBoy:

  • Simplified the item generation loops by leveraging the helper functions.
  • Ensures consistency between premium and boy item validation logic.
  • Implements configurable thresholds:
    • For premium items: strict mode with 150 tries and a total of 300 tries.
    • For boy items: strict mode with 250 tries and a total of 500 tries.
  • Maintains early exit logic to prevent regeneration if an item already meets criteria.
  • New Behavior:
    • If a suitable item cannot be generated within the maximum number of attempts, the item data is cleared, ensuring that no invalid or unintended item is sold.

Benefits

• Increased Readability:
  The separation of concerns makes the code easier to understand and maintain.
• Reduced Duplication:
  Shared helper functions ensure that validations and generation steps are consistent across different vendor scenarios.
• Ease of Future Enhancements:
  Changes to the validation logic or generation parameters can be made in one place, affecting all vendor item generation functions.
• Improved Consistency:
  The use of explicit item type restrictions via IsItemTypeAllowedForClass avoids reliance on “magic” values and makes disqualification logic clear and explicit.
• Robustness:
  Clearing the item data after exceeding maximum attempts prevents the game from using an unsuitable item and avoids potential downstream issues.

Conclusion

This refactor centralizes the logic for item generation and validation, making the codebase more maintainable and robust. By introducing clear helper functions and explicitly handling the scenario where no suitable item is generated (by clearing the item data), we improve both readability and reliability. Feedback and further improvements are welcome.

[Copilot]

Pull Request Overview

This PR refactors and unifies the vendor item generation logic for premium and boy items, consolidating common routines into helper functions to improve code clarity and consistency.

• Extracts helper functions for RNG, seeding, attribute assignment, bonus application, and validation for both premium and boy items.
• Consolidates validation logic and introduces explicit checks (such as IsItemTypeAllowedForClass) along with configurable thresholds to ensure only valid vendor items are generated.
• Clears item data if a suitable vendor item isn’t generated within the maximum number of attempts to prevent downstream issues.

Comments suppressed due to low confidence (1)

Source/items.cpp:1960

• Returning true by default for unhandled hero classes in IsItemTypeAllowedForClass may unintentionally allow disallowed item types. Verify if this behavior is intended or if an explicit handling of unknown hero classes is more appropriate.

default:
		return true;

[StephenCWills]

If a suitable item cannot be generated within the maximum number of attempts, the item data is cleared, ensuring that no invalid or unintended item is sold.

Does this not also ensure that the store itself will not function properly?

EDIT: Seems like maybe not. I see several places in stores.cpp where it's checking item.isEmpty().