fix(loot): roll binomial_with_bonus_count without fortune 🤖🤖🤖#2546
Open
DemonFiend wants to merge 1 commit into
Open
fix(loot): roll binomial_with_bonus_count without fortune 🤖🤖🤖#2546DemonFiend wants to merge 1 commit into
DemonFiend wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes loot bonus application so binomial_with_bonus_count rolls its base trials even when no Fortune enchantment is present (restoring correct crop seed drop variance).
Changes:
- Removed the outer
enchantment_level > 0gate so bonus formulas execute at level 0. - Added explanatory comments describing the intended behavior at level 0 and why gating was incorrect.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+130
to
+137
| let n = enchantment_level + *extra; | ||
| let mut extra_items = 0; | ||
| for _ in 0..n { | ||
| if rand::rng().random::<f32>() < *probability { | ||
| extra_items += 1; | ||
| } | ||
| stack.item_count = stack.item_count.saturating_add(extra_items as u8); | ||
| } | ||
| stack.item_count = stack.item_count.saturating_add(extra_items as u8); |
Comment on lines
+132
to
136
| for _ in 0..n { | ||
| if rand::rng().random::<f32>() < *probability { | ||
| extra_items += 1; | ||
| } | ||
| stack.item_count = stack.item_count.saturating_add(extra_items as u8); | ||
| } |
| parameters | ||
| { | ||
| let extra = | ||
| rand::rng().random_range(0..=(enchantment_level * *bonus_multiplier)); |
| } | ||
| } | ||
| "minecraft:ore_drops" if enchantment_level > 0 => { | ||
| let multiplier = rand::rng().random_range(0..=(enchantment_level + 1)); |
DemonFiend
force-pushed
the
fix/binomial-bonus-without-fortune
branch
from
July 25, 2026 19:54
508f974 to
70d328a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
pumpkin/src/world/loot.rs:150
uniform_bonus_countcomputes ani32extraand then casts withas u8, which truncates/wraps for values > 255, producing incorrect bonuses at high levels. Also,enchantment_level * bonus_multipliercan overflowi32. Usesaturating_mul, clamp to non-negative, and convert tou8with a saturating/clamping conversion like in the binomial branch.
let extra = rng.random_range(0..=(enchantment_level * *bonus_multiplier));
stack.item_count = stack.item_count.saturating_add(extra as u8);
pumpkin/src/world/loot.rs:156
ore_dropscastsmultiplierwithas u8, which truncates/wraps ifmultiplierever exceeds 255 (andenchantment_level + 1can overflowi32). Since this PR is already hardening overflow behavior in the binomial branch, consider clamping here as well for consistency and correctness at high levels.
let multiplier = rng.random_range(0..=(enchantment_level + 1));
if multiplier > 0 {
stack.item_count = stack.item_count.saturating_mul(multiplier as u8);
| if let Some(LootFunctionBonusParameter::Probability { extra, probability }) = | ||
| parameters | ||
| { | ||
| let n = enchantment_level + *extra; |
Comment on lines
+118
to
+125
| // These formulas must run even without the enchantment. `binomial_with_bonus_count` rolls | ||
| // `enchantment_level + extra` trials, so at level 0 it still rolls `extra` trials — this is what | ||
| // gives e.g. wheat/beetroot seeds their base 1 + binomial(3, p) = 1-4 by hand. Gating the whole | ||
| // block on `enchantment_level > 0` dropped that base roll (crops always yielded 1 seed). | ||
| // `uniform_bonus_count` computes `0..=(level * mult)`, which is a no-op at level 0, and | ||
| // `ore_drops` self-gates on its own match arm, so neither regresses. | ||
| let mut rng = rand::rng(); | ||
| for stack in stacks { |
apply_bonus gated its whole body on enchantment_level > 0, so binomial_with_bonus_count never rolled its base `extra` trials without fortune - crops always dropped exactly 1 seed instead of 1 + binomial(3, p) = 1-4. Removed the outer guard; uniform_bonus_count computes 0..=(level*mult), a no-op at level 0, and ore_drops self-gates on its match arm, so neither regresses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
DemonFiend
force-pushed
the
fix/binomial-bonus-without-fortune
branch
from
July 25, 2026 20:32
70d328a to
f63f7c4
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
pumpkin/src/world/loot.rs:153
uniform_bonus_countcan overflow/truncate in two places:(enchantment_level * *bonus_multiplier)can overflowi32(panic in debug, wrap in release), andextra as u8will wrap for values > 255. Using saturating math plus a checked conversion avoids surprising behavior with malformed/high enchantment levels.
if let Some(LootFunctionBonusParameter::Multiplier { bonus_multiplier }) =
parameters
{
let extra = rng.random_range(0..=(enchantment_level * *bonus_multiplier));
stack.item_count = stack.item_count.saturating_add(extra as u8);
pumpkin/src/world/loot.rs:160
ore_dropsconvertsmultipliertou8viaas, which wraps for values > 255. Also,enchantment_level + 1can overflowi32in debug builds. A saturating add plus checked conversion keeps behavior consistent with the new saturating logic in the binomial branch.
"minecraft:ore_drops" if enchantment_level > 0 => {
let multiplier = rng.random_range(0..=(enchantment_level + 1));
if multiplier > 0 {
stack.item_count = stack.item_count.saturating_mul(multiplier as u8);
}
| // Clamp to a non-negative count: a negative `n` would make `0..n` empty and | ||
| // silently skip every trial (reintroducing the "always base count" bug), and | ||
| // `saturating_add` avoids overflow at implausibly high enchantment levels. | ||
| let n = enchantment_level.saturating_add(*extra).max(0); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
apply_bonusgated its entire body onenchantment_level > 0, sobinomial_with_bonus_countnever rolled its baseextratrials when no fortune was present. Crops that drop via that formula (wheat/beetroot seeds) always yielded exactly 1 seed instead of1 + binomial(extra, p)(e.g. wheat = 1–4).Removed the outer
enchantment_level > 0guard so the formulas always apply.uniform_bonus_countcomputes0..=(level * multiplier), a no-op at level 0, andore_dropsself-gates on its own match arm, so neither regresses.Testing
Broke ~10 mature wheat by hand: seed drops now vary (1–4) instead of always 1; carrots/potatoes (which use
SetCount/uniform and were unaffected) still drop 2–5.cargo clippy --all-targets(debug + release),cargo fmt --check, tests, and typos all pass.