[Cursor Review] Implement piercing enchantment#30
Conversation
|
@cursor review |
📝 WalkthroughWalkthroughAdds a Piercing enchantment and pierce-capable projectile behavior, refactors arrow spawn parameters into an exported ArrowSpawnConfig used by the entity registry and weapons, and enforces mutual enchantment compatibility when applying enchantments to item stacks. ChangesArrow Piercing Enchantment and Mechanics
Sequence Diagram(s)sequenceDiagram
participant Projectile as ProjectileBehaviour
participant Trace as CollisionTrace
participant Entity as TargetEntity
participant HitCB as HitCallback
participant Closure as ClosureCheck
Projectile->>Trace: tickMovement with ignores filter
Trace-->>Entity: detect entity collision
Entity->>Projectile: append to collidedEntities
Projectile->>Entity: apply Damage (if Damage >= 0)
Projectile->>HitCB: invoke Hit callback (if set)
HitCB->>Closure: return to projectile tick
alt len(collidedEntities) > PiercingLevel
Closure->>Projectile: Close projectile
else
Closure->>Projectile: Keep velocity / continue flying
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/entity/projectile.go (1)
315-334:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't discard the rest of the tick after a pierced hit.
When
PiercingLevel > 0, the trace still stops at the first entity andendis truncated tohit.Position(), but the remaining segment is never processed. That makes piercing arrows lose travel distance on every hit and can skip a second entity or block that was still on the same tick path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/entity/projectile.go` around lines 315 - 334, The trace currently stops and truncates end to hit.Position() even when lt.conf.PiercingLevel > 0, causing pierced projectiles to lose remaining travel and skip subsequent collisions; update the logic in the trace block (where trace.Perform is called and hit/end/vel are handled) to, when hit is an entity and lt.conf.PiercingLevel > 0, continue processing the remaining segment from hit.Position() to the original intended end (or loop repeatedly) instead of immediately truncating and returning: perform additional trace.Perform calls on the remaining ray(s) (starting from hit.Position() plus a small epsilon to avoid immediate re-hit), decrement or account for piercing consumption per hit, accumulate the final end and any subsequent hit(s) (or the last non-hit end) and only set the Movement return values (pos: end, dpos, hit) after all segments are processed so pierced hits do not lose travel distance; keep using the existing symbols pos, end, hit, vel, trace.Perform, lt.conf.PiercingLevel and Movement to locate and implement the looped/continued-trace fix.
🧹 Nitpick comments (1)
server/world/chunk/light.go (1)
91-107: ⚡ Quick winSkip neighbour propagation once the light level reaches 1.
After
a.setLight(...),n.level <= 1cannot enqueue any neighbours because the minimum filter cost is already 1. Returning early here avoids six extra hot-path calls per terminal node.♻️ Proposed change
func (a *lightArea) propagate(queue *lightQueue) { n, ok := queue.pop() if !ok { return } if a.light(n.pos, n.lt) >= n.level { return } a.setLight(n.pos, n.lt, n.level) + if n.level <= 1 { + return + } x, y, z := n.pos[0], n.pos[1], n.pos[2] a.propagateNeighbour(queue, n.lt, n.level, x+1, y, z) a.propagateNeighbour(queue, n.lt, n.level, x-1, y, z) a.propagateNeighbour(queue, n.lt, n.level, x, y+1, z)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/world/chunk/light.go` around lines 91 - 107, In propagate (method lightArea.propagate) after the call to a.setLight(n.pos, n.lt, n.level), check if n.level <= 1 and return early to avoid calling a.propagateNeighbour six times for terminal nodes; this prevents attempting to enqueue neighbours when the minimum filter cost is 1 and eliminates the extra hot-path calls for those nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/entity/register.go`:
- Around line 50-53: Make ArrowSpawnConfig handling zero-value safe in the Arrow
factory: guard the type assertion on arrow.Tip and the call to arrow.Owner.H()
so they don't panic when omitted—use a safe type-assertion for tip (e.g., tip,
ok := arrow.Tip.(potion.Potion); if !ok use zero-value potion.Potion{}) and
check arrow.Owner for nil (or presence of H) before calling H(), then assign
conf.Damage, conf.Potion and conf.Owner using those safe values in Arrow (the
Arrow function that sets conf from arrow and arrowConf).
---
Outside diff comments:
In `@server/entity/projectile.go`:
- Around line 315-334: The trace currently stops and truncates end to
hit.Position() even when lt.conf.PiercingLevel > 0, causing pierced projectiles
to lose remaining travel and skip subsequent collisions; update the logic in the
trace block (where trace.Perform is called and hit/end/vel are handled) to, when
hit is an entity and lt.conf.PiercingLevel > 0, continue processing the
remaining segment from hit.Position() to the original intended end (or loop
repeatedly) instead of immediately truncating and returning: perform additional
trace.Perform calls on the remaining ray(s) (starting from hit.Position() plus a
small epsilon to avoid immediate re-hit), decrement or account for piercing
consumption per hit, accumulate the final end and any subsequent hit(s) (or the
last non-hit end) and only set the Movement return values (pos: end, dpos, hit)
after all segments are processed so pierced hits do not lose travel distance;
keep using the existing symbols pos, end, hit, vel, trace.Perform,
lt.conf.PiercingLevel and Movement to locate and implement the
looped/continued-trace fix.
---
Nitpick comments:
In `@server/world/chunk/light.go`:
- Around line 91-107: In propagate (method lightArea.propagate) after the call
to a.setLight(n.pos, n.lt, n.level), check if n.level <= 1 and return early to
avoid calling a.propagateNeighbour six times for terminal nodes; this prevents
attempting to enqueue neighbours when the minimum filter cost is 1 and
eliminates the extra hot-path calls for those nodes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cf25738-003d-4d0c-ada9-f000c64785e2
📒 Files selected for processing (10)
server/entity/projectile.goserver/entity/register.goserver/item/bow.goserver/item/crossbow.goserver/item/enchantment/multishot.goserver/item/enchantment/piercing.goserver/item/enchantment/register.goserver/world/chunk/light.goserver/world/chunk/light_area.goserver/world/entity.go
| Arrow: func(opts world.EntitySpawnOpts, arrow world.ArrowSpawnConfig) *world.EntityHandle { | ||
| tip := arrow.Tip.(potion.Potion) | ||
| conf := arrowConf | ||
| conf.Damage, conf.Potion, conf.Owner = damage, tip.(potion.Potion), owner.H() | ||
| conf.KnockBackForceAddend = float64(punchLevel) * enchantment.Punch.KnockBackMultiplier() | ||
| conf.DisablePickup = disallowPickup | ||
| if obtainArrowOnPickup { | ||
| conf.PickupItem = item.NewStack(item.Arrow{Tip: tip.(potion.Potion)}, 1) | ||
| conf.Damage, conf.Potion, conf.Owner = arrow.Damage, tip, arrow.Owner.H() |
There was a problem hiding this comment.
Make ArrowSpawnConfig zero-value safe here.
arrow.Tip.(potion.Potion) and arrow.Owner.H() will panic if a caller omits the tip or spawns an ownerless arrow. The new struct API makes both omissions easy.
💡 Suggested fix
Arrow: func(opts world.EntitySpawnOpts, arrow world.ArrowSpawnConfig) *world.EntityHandle {
- tip := arrow.Tip.(potion.Potion)
+ var tip potion.Potion
+ if t, ok := arrow.Tip.(potion.Potion); ok {
+ tip = t
+ }
conf := arrowConf
- conf.Damage, conf.Potion, conf.Owner = arrow.Damage, tip, arrow.Owner.H()
+ conf.Damage, conf.Potion = arrow.Damage, tip
+ if arrow.Owner != nil {
+ conf.Owner = arrow.Owner.H()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Arrow: func(opts world.EntitySpawnOpts, arrow world.ArrowSpawnConfig) *world.EntityHandle { | |
| tip := arrow.Tip.(potion.Potion) | |
| conf := arrowConf | |
| conf.Damage, conf.Potion, conf.Owner = damage, tip.(potion.Potion), owner.H() | |
| conf.KnockBackForceAddend = float64(punchLevel) * enchantment.Punch.KnockBackMultiplier() | |
| conf.DisablePickup = disallowPickup | |
| if obtainArrowOnPickup { | |
| conf.PickupItem = item.NewStack(item.Arrow{Tip: tip.(potion.Potion)}, 1) | |
| conf.Damage, conf.Potion, conf.Owner = arrow.Damage, tip, arrow.Owner.H() | |
| Arrow: func(opts world.EntitySpawnOpts, arrow world.ArrowSpawnConfig) *world.EntityHandle { | |
| var tip potion.Potion | |
| if t, ok := arrow.Tip.(potion.Potion); ok { | |
| tip = t | |
| } | |
| conf := arrowConf | |
| conf.Damage, conf.Potion = arrow.Damage, tip | |
| if arrow.Owner != nil { | |
| conf.Owner = arrow.Owner.H() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/entity/register.go` around lines 50 - 53, Make ArrowSpawnConfig
handling zero-value safe in the Arrow factory: guard the type assertion on
arrow.Tip and the call to arrow.Owner.H() so they don't panic when omitted—use a
safe type-assertion for tip (e.g., tip, ok := arrow.Tip.(potion.Potion); if !ok
use zero-value potion.Potion{}) and check arrow.Owner for nil (or presence of H)
before calling H(), then assign conf.Damage, conf.Potion and conf.Owner using
those safe values in Arrow (the Arrow function that sets conf from arrow and
arrowConf).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0cf3d5a. Configure here.
| if !compatible { | ||
| // Enchantment is not compatible with another enchantment on the item. | ||
| continue | ||
| } |
There was a problem hiding this comment.
WithItem silently drops incompatible enchantments due to new check
Medium Severity
The new enchantment compatibility check in WithEnchantments causes WithItem (line 374) to silently drop incompatible enchantments when copying them. WithItem calls WithEnchantments(s.Enchantments()...), and since Enchantments() iterates a map in non-deterministic order, which of two incompatible enchantments gets dropped is random. Items loaded from NBT via WithForcedEnchantments can legitimately hold incompatible enchantments; those are now randomly pruned whenever WithItem is called — for example in Crossbow.ReleaseCharge. The copy in WithItem likely needs to use WithForcedEnchantments to preserve existing enchantments faithfully.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0cf3d5a. Configure here.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/entity/projectile.go (1)
179-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep side effects for harmless projectiles.
Line 180 now skips
hitEntity()wheneverDamage < 0, which also drops potion/fire application.ProjectileBehaviourConfig.Damageonly promises “not hurt” / “not knocked back”, so this broadens the contract for any projectile that is meant to be harmless but still apply on-hit effects. CallhitEntity()unconditionally here and gate only the damage/knockback path insidehitEntity().Suggested direction
- if lt.conf.Damage >= 0 { - lt.hitEntity(l, e, vel) - } + lt.hitEntity(l, e, vel)// Then, inside hitEntity(), guard Hurt/KnockBack on lt.conf.Damage >= 0, // but keep the potion/fire application path intact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/entity/projectile.go` around lines 179 - 183, The current code skips lt.hitEntity(l, e, vel) whenever lt.conf.Damage < 0, which prevents non-damaging projectiles from applying potion/fire effects; instead call lt.hitEntity(...) unconditionally for Living targets and move the Damage >= 0 guard into hitEntity so only the Hurt/KnockBack paths are gated by lt.conf.Damage, while leaving potion/fire application and other side-effects intact; update hitEntity (and any helpers it calls) to check lt.conf.Damage >= 0 before performing damage/knockback but always execute effect-application logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/entity/projectile.go`:
- Around line 179-183: The current code skips lt.hitEntity(l, e, vel) whenever
lt.conf.Damage < 0, which prevents non-damaging projectiles from applying
potion/fire effects; instead call lt.hitEntity(...) unconditionally for Living
targets and move the Damage >= 0 guard into hitEntity so only the Hurt/KnockBack
paths are gated by lt.conf.Damage, while leaving potion/fire application and
other side-effects intact; update hitEntity (and any helpers it calls) to check
lt.conf.Damage >= 0 before performing damage/knockback but always execute
effect-application logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06adb743-4d27-4eaf-ab69-0b3e6c913158
📒 Files selected for processing (2)
server/entity/projectile.goserver/world/entity.go
2 participants
Cursor review mirror of df-mc#1246
Original: df-mc#1246
Note
Medium Risk
Modifies core projectile collision/damage flow and enchantment application rules, which can affect combat balance and entity interactions. Risk is moderate due to new state tracking for pierced entities and API signature changes for arrow spawning.
Overview
Adds the crossbow Piercing enchantment (registered as ID
34) and makes it mutually incompatible withMultishot.Updates projectile handling so arrows can pass through multiple entities based on a new
PiercingLevel, tracking already-hit entities to avoid repeat hits and only closing the projectile after exceeding the allowed pierces.Refactors arrow spawning to use a single
world.ArrowSpawnConfigstruct (replacing the long parameter list) and threads this throughEntityRegistryConfig,entity/register.go,Bow, andCrossbow(including reading the piercing enchant level when firing). Also tightensStack.WithEnchantmentsto enforce both-direction enchantment compatibility against existing enchants.Reviewed by Cursor Bugbot for commit 6960176. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Balance & Improvements