[Demonology] Let cast-time on-use buff trinkets fire around Summon Demonic Tyrant - #11807
Open
bclinnin wants to merge 1 commit into
Open
[Demonology] Let cast-time on-use buff trinkets fire around Summon Demonic Tyrant#11807bclinnin wants to merge 1 commit into
bclinnin wants to merge 1 commit into
Conversation
…monic Tyrant Demonology's buff-trinket line is unsatisfiable for any on-use buff trinket with a cast time. For such a trinket the second clause requires Tyrant to be inactive: (!pet.demonic_tyrant.active&trinket.N.cast_time>0|!trinket.N.cast_time>0) while the third requires it to be active, the remaining arms needing either no Tyrant talent or trinket_priority pointing at the other slot: (pet.demonic_tyrant.active|!talent.summon_demonic_tyrant|variable.trinket_priority=...) With Tyrant talented and trinket_priority on the trinket's own slot the conjunction can never be true, so the line only fires through its trailing |variable.trinket_N_buff_duration>=fight_remains -- once, in the last few seconds of the fight. Measured with Stormbound Emblem of Dazar equipped: usage is 1.0 per fight at 300s, 600s and 900s max_time, i.e. independent of fight length despite a 120s cooldown; and removing the fight_remains clause makes the trinket never fire at all. Add the matching cast-time arm to the third clause so such a trinket may fire while Tyrant is nearly ready. Instant trinkets are unaffected -- the new arm is gated behind cast_time>0. uptime 1.40% -> 19.19% (1.0 -> 3.0 uses/fight, 4.2s -> 19.3s each) Soul Harvester 1/2/5 tgt +1.51% / +2.23% / +2.89% Diabolist 1/2/5 tgt +1.10% / +1.38% / +2.36% ilvl 320 / 350 +1.34% / +1.74% DungeonSlice +0.98% emblem in trinket2 +1.54% Regression, target_error=0.05: no emblem equipped +0.02%, and Vile Vial of Volatile Venom (an instant on-use buff trinket) +0.02% -- both noise. The <=4 threshold was swept against .ready / <=2 / <=6 / <=8 / ungated on both hero trees; it is first on Soul Harvester and within 0.004% of first on Diabolist. Ungated and .ready are the two worst on both.
bclinnin
force-pushed
the
warlock-stormbound-emblem
branch
from
August 26, 2026 19:52
d87f6a9 to
9759a97
Compare
bclinnin
marked this pull request as ready for review
August 26, 2026 21:04
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.
Demonology's buff-trinket line is unsatisfiable for any on-use buff trinket that has a cast time. The second clause requires Tyrant to be inactive:
and the third requires it to be active, its remaining arms needing either no Tyrant talent or
trinket_prioritypointing at the other slot:With Tyrant talented and
trinket_priorityon the trinket's own slot, the conjunction can never be true. The line only ever fires through its trailing|variable.trinket_N_buff_duration>=fight_remains— once, in the last few seconds of the fight.Two measurements with Stormbound Emblem of Dazar equipped, on the unmodified APL:
max_time300 / 600 / 900fight_remainsclause removedThe change
Add the matching cast-time arm to the third clause, so such a trinket may fire while Tyrant is nearly ready. Instant trinkets are untouched — the new arm sits behind
cast_time>0.Two lines,
+2 −2. With Stormbound equipped:use_itemon cooldown, no rotation)Worth +0.98% to +2.89% across hero trees, target counts, item levels and fight styles, and inert for instant trinkets and for players without a cast-time buff trinket. Details below.
Full results —
target_error=0.05Regressions — this edits a line every Demonology trinket flows through, so both cases are measured rather than argued:
Both are noise, confirming instant trinkets take the unchanged path.
Note on methodology: the shipped profile embeds its own APL snapshot, so simming it directly does not exercise
warlock.cpp. All runs above use an APL-stripped copy, verified equivalent to the embedded APL within 0.09%.Why
remains<=4Swept against
.ready/<=2/<=6/<=8/ ungated on both hero trees,target_error=0.05:<=4<=2<=6<=8.readyThe trees disagree on the optimum, and
<=4is the only value strong on both. Ungated and.readyare the two worst on each, so the gate is doing real work.Scope: which trinkets this actually affects
The bug needs both
has_use_buffandcast_time>0. Across the MID2 trinket pool, exactly one item qualifies today:has_use_buffcast_time>0So in practice this currently rescues one trinket. It is written as a logic fix rather than a named-item exception because the defect is in the condition, and any future cast-time on-use buff trinket would hit it the same way.
An earlier revision of this PR instead added a named
use_itemline for the emblem. That worked, but it routed around the broken condition rather than fixing it, and sat +9 −6 with a hard-coded item name; the repair here beats it in every test case above.Note on
has_use_buffFor the record, since it came up while investigating:
trinket.X.has_use_buffresolves correctly for this trinket.item_buff_exists_expr_tfalls back to a name lookup —— so the buff being created directly rather than through
create_all_stat_buffs()(and therefore missing fromspecial_effect_t::buff_list) does not affect the expression. There is no engine bug here; the problem is entirely in the APL condition.