fix acu teleport damage not aligning with blueprint#7168
fix acu teleport damage not aligning with blueprint#7168Lightningbulb2 wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChanges update teleport-in effect damage typing and increase weapon damage values in four unit blueprints. ChangesTeleport Damage Adjustment
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lua/EffectUtilities.lua (1)
1383-1383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated epsilon value into a named constant.
The literal
0.000000001is duplicated identically across all four faction branches. A shared local constant (e.g.local TELEPORT_IN_AOE_DAMAGE = 0.000000001) with a brief comment explaining why a near-zero (rather than exact0) value is used would improve clarity and avoid drift if this needs tuning again.♻️ Suggested refactor
+-- Effectively-zero damage used to preserve hit-reaction/VFX behavior in DamageArea +-- without dealing meaningful damage (see issue `#7006`). +local TELEPORT_IN_AOE_DAMAGE = 0.000000001 + function PlayTeleportInEffects(unit, effectsBag) ... - DamageArea(unit, unit:GetPosition(), 9, 0.000000001, 'Force', true) + DamageArea(unit, unit:GetPosition(), 9, TELEPORT_IN_AOE_DAMAGE, 'Force', true)Also applies to: 1410-1410, 1447-1447, 1475-1475
🤖 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 `@lua/EffectUtilities.lua` at line 1383, The repeated near-zero damage radius literal in EffectUtilities.lua should be replaced with a shared named constant. Add a local constant near the relevant teleport/area-damage logic and use it in each faction branch where DamageArea is called, so the same value is defined once and stays consistent. Include a short comment by the constant explaining why a near-zero value is used instead of exact zero.
🤖 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.
Nitpick comments:
In `@lua/EffectUtilities.lua`:
- Line 1383: The repeated near-zero damage radius literal in EffectUtilities.lua
should be replaced with a shared named constant. Add a local constant near the
relevant teleport/area-damage logic and use it in each faction branch where
DamageArea is called, so the same value is defined once and stays consistent.
Include a short comment by the constant explaining why a near-zero value is used
instead of exact zero.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 129878dd-2c55-4b1e-b9fa-c0864b59161e
📒 Files selected for processing (5)
lua/EffectUtilities.luaunits/UAL0001/UAL0001_unit.bpunits/UEL0001/UEL0001_unit.bpunits/URL0001/URL0001_unit.bpunits/XSL0001/XSL0001_unit.bp
lL1l1
left a comment
There was a problem hiding this comment.
I think a more broad solution can be done:
- Force type damage should be ignored by units instead. I'm not sure why they aren't, maybe @Garanas knows because he reworked the tree knocking behavior.
- Alternatively all scripted Force damage types should be converted to TreeForce. This would have the least effect on mods. I prefer this solution with what I know.
Context for 1:
Units already handle some tree damage types:
Line 1377 in 1d1530b
This function handles the damage types used by trees for context:
Lines 95 to 137 in a7ccd80
I added option 2: It works perfectly fine using TreeForce so I don't see why not, plus it better indicates what it's actually doing in this case. (Damaging trees, not troops) I kinda thought it was there for some other sort of compatibility reason until I checked the 12 year old PR that added it. |
Acu teleport now does exactly as much damage as it should (according to the blueprint)
fixes #7006
Checklist
- [ ] Changes are annotated, including comments where useful- [ ] Changes are documented in a changelog snippet according to the guidelines.- [ ] Request 2-3 reviewers from the list of reviewers and their areas of knowledge.not necessary, tiny change
Summary by CodeRabbit