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

QoL: Add Gameplay Option to Disable the Search Spell #7855

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

Conversation

wkdgmr
Copy link
Contributor

@wkdgmr wkdgmr commented Mar 17, 2025

Overview
This PR addresses the deprecation and uselessness of the "Search" spell when the Show Item Labels option is enabled. With the new Disable Search Spell gameplay option enabled, our changes ensure that:

  • Scrolls, Books, and Staves dropped in the dungeon are prevented from dropping or re-rolled if they would grant the SpellID::Search.
  • Adria’s shop no longer offers items that provide the Search spell.
  • Griswold and Wirt no longer offers items that provide the Search spell.
  • The Monk’s class skill is updated from Search to Infravision.

Rationale
In our changes, we are updating the Monk’s class Skill by replacing the traditional “Search” spell with an Infravision ability. This adjustment more closely aligns with the Monk’s thematic identity as described in Hellfire’s materials—a warrior whose inner focus and spiritual insight allow him to see beyond the ordinary, revealing what lies hidden in the darkness. Moreover, Infravision represents a unique skill that mirrors the special abilities of the other classes, whereas Search is learnable as a generic spell and does not reflect the distinct nature of the Monk’s discipline.

Compatibility & Stability

  • This PR maintains backwards compatibility. When the Disable Search Spell option is disabled, the Monk will lose access to Infravision and regain the Search skill.
  • It ensures that item generation remains stable and does not inadvertently cause morphing or other adverse effects.
  • The changes occur only when the Disable Search Spell option is enabled, leaving standard behavior intact for users who do not opt in.

Summary of Changes

  • Item Generation: When the Disable Search Spell option is active, any droppable item that would generate SpellID::Search is re-rolled via the SetUpBaseItem() or SpawnItem() and scrolls of search are prevented from dropping via the GetItemIndexForDroppableItem() function.
  • Shop Logic: Prevents Vendors from offering items that grant the Search spell when the option is enabled. This is done by adding Checks to WitchItemOk and PremiumItemOkay logic.
  • Player Initialization: Updates the Monk’s class skill to Infravision in a revertible and backwards compatible manner. Logic was added to handle this seamlessly.

These changes aim to bring certain obsolete/useless Hellfire elements in line with modern QoL as an optional toggle.

@wkdgmr
Copy link
Contributor Author

wkdgmr commented Mar 17, 2025

I received some feedback on the item re rolling. My addition of idx re roll in spawnitem is not good, will commit a change to this soon. Need to address Scrolls and their associated spells all being base items and need to re roll this at the idx level

@wkdgmr wkdgmr force-pushed the disableSearchInfraMonkOptions branch from bf11f47 to 07037ad Compare March 17, 2025 20:16
@wkdgmr
Copy link
Contributor Author

wkdgmr commented Mar 17, 2025

Okay, I am comfortable where this is at now functionally. There is another PR that as submitted after this one effecting the spellbook skill logic that I may have to make some changes for but I would be willing to do that if needed

@wkdgmr
Copy link
Contributor Author

wkdgmr commented Mar 17, 2025

There were some line changes made by my document formatter to pass the clang check, apparently it removed some brackets. This seems to be a good thing, but I was just formatting at that point to pass clang check that wasn't intentional.

Also there are a few areas of my code that could be deduplicated/slightly refactored to be a bit cleaner, I will push a commit for that soon

@wkdgmr
Copy link
Contributor Author

wkdgmr commented Mar 17, 2025

The PR is now functionally and technically sound as well as formatted correctly. We may need to force this as a forced on/off by game creator type of thing looking into that

@yuripourre
Copy link
Collaborator

yuripourre commented Mar 18, 2025

@wkdgmr first of all I agree with the motivation but I know this change is very subjective.

The title is misleading, you are not introducing only the ability to disable the Search, you are actually adding a toggle to replace it by infravision and I think that's where this PR starts entering in mod territory.

I am totally fine with a PR that adds an option that only disables the search skill. But replace it by infravision requires a broader discussion and I don't think it would be well received.

As a suggestion, for the infravision part, I think you could try to do this as a mod using lua script.

@wkdgmr
Copy link
Contributor Author

wkdgmr commented Mar 18, 2025

@wkdgmr first of all I agree with the motivation but I know this change is very subjective.

The title is misleading, you are not introducing only the ability to disable the Search, you are actually adding a toggle to replace it by infravision and I think that's where this PR starts entering in mod territory.

I am totally fine with a PR that adds an option that only disables the search skill. But replace it by infravision requires a broader discussion and I don't think it would be well received.

As a suggestion, for the infravision part, I think you could try to do this as a mod using lua script.

Hi Yurri,

To clarify, we would only be giving the Monk access to Infravision as a class skill when Search is disabled. Search Items will either be skipped, rerolled, or just not dropped at all. Your response made it seem like you thought I am replacing Search wholesale with Infravision, which is not the case apologies if it came across that way.

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