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

Add config toggle for fortune on ores #1887

Open
wants to merge 2 commits into
base: 1.20.1
Choose a base branch
from

Conversation

JuiceyBeans
Copy link
Contributor

What

Mfw the mod adds fortune to tools but its useless
Adds a config option for fortune working on ore drops (off by default)

Implementation Details

Added a new config option and an if statement to block loot drop MixinHelper to apply fortune if oreFortuneDrops is true

Outcome

Turning on oreFortuneDrops makes fortune work on ores

@JuiceyBeans JuiceyBeans requested a review from a team as a code owner September 3, 2024 10:38
screret
screret previously approved these changes Sep 3, 2024
@JuiceyBeans
Copy link
Contributor Author

WHOOPS DID NOT MEAN FOR THAT TO REFERENCE AN ISSUE LMAO

@Ghostipedia
Copy link
Member

Ghostipedia commented Sep 3, 2024

Against this being merged.
Fortune should ideally have a chance to drop crushed ores, not full raw. And an equally slimmer chance for bonus 'dust' rolls.

Context ; This was a system discussed as a balance solution vs direct raw ores. which become absurdly difficult to balance around in the presence of over-enchanting. Even packs like GTNH of all things limit Fortune to a Max of 3 and don't allow >3 to work

@JuiceyBeans
Copy link
Contributor Author

JuiceyBeans commented Sep 3, 2024

Meant this as a placeholder for players (and packdevs) who want to use it until the Fortune rework happens eventually:tm:

Better to have something instead of nothing but wasted tools until it happens

@Ghostipedia
Copy link
Member

It makes more sense to directly implement properly than add a temp solution. As temporary solutions can easily become permanent ones.
We can talk over a proper implementation in the discord but until then I'm going to mark this for do not merge

@Ghostipedia Ghostipedia added the Do Not Merge DO NOT MERGE THIS PR YET! label Sep 3, 2024
@JuiceyBeans JuiceyBeans deleted the fortune-toggle branch September 26, 2024 11:05
@JuiceyBeans JuiceyBeans restored the fortune-toggle branch September 26, 2024 11:06
@JuiceyBeans
Copy link
Contributor Author

OOPS

@JuiceyBeans JuiceyBeans reopened this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge DO NOT MERGE THIS PR YET!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants