feat: ProxySpell - #1100
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Direct targeted casts can bypass proxy redirection.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds ProxySpell, allowing spells and entity damage targeting a designated proxy to redirect to the caster.
Changes:
- Tracks proxy relationships and lifecycle.
- Redirects spell targeting, impacts, and entity damage.
- Applies redirect effects and charge handling.
File summaries
| File | Summary |
|---|---|
core/src/main/java/com/nisovin/magicspells/spells/buff/ProxySpell.java |
Implements proxy tracking and redirection. Moderate issue: Subspell DIRECT casts can bypass onSpellTarget, allowing spells to affect the proxy instead of the caster. |
Review details
Suppressed comments (5)
core/src/main/java/com/nisovin/magicspells/spells/buff/ProxySpell.java:96
- This listener only receives
EntityDamageByEntityEvent, so damage that is not caused by an entity—such as fall, fire, drowning, suffocation, or void damage—bypasses the proxy entirely. That contradicts the feature's promised redirection of any damage; handle the broaderEntityDamageEventand preserve the original entity damager when one exists.
public void onEntityDamage(EntityDamageByEntityEvent event) {
if (!(event.getEntity() instanceof LivingEntity target) || !target.isValid()) return;
core/src/main/java/com/nisovin/magicspells/spells/buff/ProxySpell.java:90
- The retargeted
subDatais only used for the effects and is never written back to theSpellPreImpactEvent, so every consumer still receives the original target. Moreover,setRedirected(true)makes projectile implementations take their reflection path (for example, invert the spell data), which can send the impact back toward the original caster rather than applying it to the proxy owner. Persist the retargeted data in the event (adding the corresponding event API if necessary) and use the normal impact path for proxy delivery.
SpellData subData = new SpellData(event.getCaster(), proxyTarget, event.getPower());
playRedirectEffects(target, proxyTarget, subData);
event.setRedirected(true);
core/src/main/java/com/nisovin/magicspells/spells/buff/ProxySpell.java:106
redirectingis populated beforeplayRedirectEffects, but thetry/finallystarts afterward. If a configured effect throws, the UUID is never removed; later damage then hits the early return at line 100 and bypasses proxying. MoveplayRedirectEffectsinside thetryso the marker is always cleared.
playRedirectEffects(target, proxyTarget, subData);
event.setCancelled(true);
try {
core/src/main/java/com/nisovin/magicspells/spells/buff/ProxySpell.java:107
damage(double, Entity)creates a new native damage event and discards the original event'sDamageCause/DamageSource. For example, aMagicSpellsEntityDamageByEntityEventwithMAGICis applied to the proxy owner as an entity attack, so cause/type-filtered listeners and protections observe the wrong damage. Forward using the original source/cause instead of this overload.
proxyTarget.damage(event.getDamage(), event.getDamager());
core/src/main/java/com/nisovin/magicspells/spells/buff/ProxySpell.java:99
- This treats every
EntityDamageByEntityEventas real damage, but the core also creates the same event type for non-damaging permission/modifier checks (Spell.createFakeDamageEvent, used for example by FlamewalkSpell before its target event). A fake check against a proxy will enter here, damage the proxy's caster, and be cancelled, causing the original spell to be skipped. Synthetic damage checks must be distinguishable and ignored before forwarding damage.
public void onEntityDamage(EntityDamageByEntityEvent event) {
if (!(event.getEntity() instanceof LivingEntity target) || !target.isValid()) return;
LivingEntity proxyTarget = getProxyTarget(target);
if (proxyTarget == null) return;
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public void onSpellTarget(SpellTargetEvent event) { | ||
| LivingEntity target = event.getTarget(); | ||
| if (target == null || !target.isValid()) return; | ||
|
|
||
| LivingEntity proxyTarget = getProxyTarget(target); | ||
| if (proxyTarget == null) return; | ||
|
|
||
| SpellData subData = event.getSpellData().target(proxyTarget); | ||
| playRedirectEffects(target, proxyTarget, subData); | ||
|
|
||
| event.setTarget(proxyTarget); |
Allows the caster to designate an entity as a "proxy". Any spell / damage will be redirected to the caster treating that entity as if it were the caster themself.