[Gear] Fix Bite of Zul'jan review findings#1
Open
NN0323 wants to merge 1 commit into
Open
Conversation
Address code review of the Bite of Zul'jan set (Venomfang weapons + Zul'jin's Guillotine Technique trinket + 2pc): - Venomfang Burst now fires per instance. The async debuff's expire_callback only fires when the whole debuff clears, so overlapping applications produced a single burst instead of one per instance. Drive the burst from a stack-change callback on each lost stack (the final stack clears via expire(), which also invokes it). - Guillotine ricochet no longer drifts onto the primary. secondary_targets_only() excludes ricochet->target, but a single-target action's execute_on_target() does not invalidate the target cache (set_target only does so when n_targets() != 0), so an add/death cache refresh could re-admit the primary, causing a double Guillotine + Venomfang. Re-point at the primary and invalidate the cache each cast. - Venomfang debuff duration reads the refreshed DoT's remains() instead of recomputing the refresh duration, which overshot by a tick on the first application. - composite_ta_multiplier zeroes tick damage when no instance debuff is active, matching root_rot. - Gate the Venomfang weapon driver (1291718) behind 12.1.0 for consistency with its Bite of Zul'jan set siblings (trinket + 2pc driver). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Fixes from code review of the Bite of Zul'jan set (Venomfang weapons + Zul'jin's Guillotine Technique trinket + 2pc). All changes are in
engine/player/unique_gear_midnight.cpp.Critical
Venomfang Burst fired once instead of per instance. The async debuff used
set_expire_callback, which only fires when the buff fully clears —decrement()callsexpire()only on the last stack, while intermediate expiries just decrement. With N overlapping applications the burst fired once (N→…→0), not N times, defeating the purpose of the ASYNCHRONOUS rework. Now driven byset_stack_change_callbackfiring on each lost stack (the final stack still clears viaexpire(), which also invokes the callback). Matches the async idiom used byreavers_mark(DH,sc_demon_hunter.cpp:9362) and the paladin templar buffs.Important
Guillotine ricochet could drift onto the primary target.
secondary_targets_only()excludesricochet->target, but the ricochet is single-target, soexecute_on_target()repoints its target to the last bounce victim without invalidating the target cache (set_targetonly invalidates whenn_targets() != 0). On any add/death the cache is rebuilt by thetarget_non_sleeping_listcallback, now excluding the drifted target — re-admitting the primary and yielding a double Guillotine + double Venomfang on it. Fixed by re-pointing at the primary and forcing a cache recompute each cast. (font_of_venomous_rageis safe with the same helper only because its splatter is AoE, whereexecute_on_targetdoes invalidate the cache.)Minor
remains()instead of recomputingcalculate_dot_refresh_durationon the already-refreshed dot, which (forDOT_REFRESH_DURATION=time_to_next_full_tick + triggered_duration) overshot by ~1 tick on the first application (≈7s vs the 6s DoT). Also makes the per-instance burst fire on DoT expiry, matching the comment.composite_ta_multiplier: zero tick damage when no instance debuff is active, matchingroot_rot(defensive; the DoT and debuff could otherwise desync by a tick).1291718) behind 12.1.0, consistent with its Bite of Zul'jan set siblings (the trinket1291728and 2pc driver1291726are already gated).Deliberately not changed
The review suggested asserting on
find_action("venomfang")in the 2pc path. I kept the existingif ( venomfang )guard instead:set_bonus=overrides can force the 2pc without equipping the weapons, where the action is legitimately null — an assert would crash those sims. This matches the file's idiom for cross-item set dependencies (e.g.torments_dualityuses a softfind_special_effectcheck, not an assert).Verification
Syntax-checked the translation unit with MSVC (
cl /Zs /std:c++17 /utf-8, exit 0). All touched APIs (set_stack_change_callback(buff_t*,int,int),dot_t::remains(),set_target, publictarget_cache) were verified against the engine headers and existing usages. No sim was run; the right end-to-end check is the review's suggested multi-target sim watching Venomfang Burst frequency, which is what surfaces the Critical fix.🤖 Generated with Claude Code