From 1287c328e05cb099ba8a2a99ad34f1ac1fc25ba0 Mon Sep 17 00:00:00 2001 From: grom Date: Sat, 17 Oct 2026 13:41:47 -0600 Subject: [PATCH] creature_anim: the loot sparkle's switch starts where its CVar does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from play: no sparkle on a mining vein, while lootable mobs sparkled fine. That split is the diagnosis — only the GameObject arm reads the switch. `LootSparkleMode` derived `Default`, so it booted `false` while its row registered `"1"`. A boot fires `CvarChanged` only for rows `config.toml` actually carries, and writing `"1"` over a value already `"1"` is a no-op that fires nothing either, so nothing could ever tell the resource its real default — not the boot, and not the player toggling the CVar. The feature was inert for everyone who had never written that row. `vplates.rs`'s `VPlateMode::default` carries this warning in as many words, from `NameplateOcclusion` shipping inert the same way, and this is the same mistake one feature later. So: a hand-written `Default`, and the twin of that module's test, reading the registered row rather than restating it. Swept the rest — of the 32 resources a `CvarChanged` observer writes, 6 derive `Default` and every one of their rows registers `"0"`, so this was the only one. The doc row kept two stale claims and both are corrected: the test has had two arms (the activate bit *or* `GAMEOBJECT_TYPE_CHEST`) since it was widened, and `FxStage` is immaterial here — `Particles\LootFX.m2`, the `"HARDCODED Loot Art"` row's own model, holds one 2400 ms sequence with `flags = 0`, and bit 0 clear loops, so `FxAnimLife::arm` repeats it whatever the stage says. Still unverified in world: whether veins, herbs and chests sparkle, and whether anything sparkles that should not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../src/creature_anim/spell_visual.rs | 19 +++++++++++++++--- .../src/creature_anim/spell_visual/tests.rs | 20 +++++++++++++++++++ docs/ADDON_COMPAT.md | 6 ++++-- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/crates/benilla-app/src/creature_anim/spell_visual.rs b/crates/benilla-app/src/creature_anim/spell_visual.rs index b4da4964..6ffa1f8c 100644 --- a/crates/benilla-app/src/creature_anim/spell_visual.rs +++ b/crates/benilla-app/src/creature_anim/spell_visual.rs @@ -1086,11 +1086,24 @@ pub(crate) fn arm_aura_state_fx( /// just a corpse. The mod's own spelling; registered in `cvars.rs`. pub(crate) const CVAR_LOOT_SPARKLE: &str = "LootSparkle"; -/// [`CVAR_LOOT_SPARKLE`]'s applied value, seeded by the boot-time `CvarChanged` observer as -/// `vplates.rs`'s `VPlateMode` is. -#[derive(Resource, Default)] +/// [`CVAR_LOOT_SPARKLE`]'s applied value. +/// +/// **The default must equal the registered row's**, because a boot emits `CvarChanged` only for +/// rows `config.toml` actually carries, and a write of `"1"` over a `"1"` is a no-op that emits +/// nothing either — so nothing would ever tell a derived `false` that the CVar reads on. That is +/// exactly how this feature shipped inert: the row registered `"1"`, the resource derived +/// `false`, and no vein, herb or chest sparkled for anyone who had never touched the CVar. +/// `vplates.rs`'s `VPlateMode::default` carries the same warning for the same reason; +/// [`tests::the_loot_sparkle_default_agrees_with_its_registered_cvar`] is the check. +#[derive(Resource)] pub(crate) struct LootSparkleMode(pub(crate) bool); +impl Default for LootSparkleMode { + fn default() -> Self { + Self(true) + } +} + /// Apply a change of the switch. pub(crate) fn on_loot_sparkle_cvar( ev: On, diff --git a/crates/benilla-app/src/creature_anim/spell_visual/tests.rs b/crates/benilla-app/src/creature_anim/spell_visual/tests.rs index 964116fc..fff41aae 100644 --- a/crates/benilla-app/src/creature_anim/spell_visual/tests.rs +++ b/crates/benilla-app/src/creature_anim/spell_visual/tests.rs @@ -1653,6 +1653,26 @@ fn an_anim_only_state_kit_still_arms_on_the_aura_add_edge() { ); } +/// The switch's applied default must equal the row it reads, or the feature ships inert. +/// +/// A boot fires `CvarChanged` only for rows `config.toml` carries, and re-writing a row with the +/// value it already holds is a no-op that fires nothing either — so a derived `false` beside a +/// registered `"1"` is never corrected, by the boot or by the player. That is how the sparkle +/// shipped off: the decision below was right, the switch feeding it was never turned on. Twin of +/// `vplates.rs`'s `the_occlusion_default_agrees_with_its_registered_cvar`. +#[test] +fn the_loot_sparkle_default_agrees_with_its_registered_cvar() { + let registered = crate::cvars::registered_pairs() + .find(|(n, _)| n.eq_ignore_ascii_case(super::CVAR_LOOT_SPARKLE)) + .map(|(_, d)| d) + .expect("LootSparkle is registered"); + assert_eq!( + super::LootSparkleMode::default().0, + registered != "0", + "the applied default must equal the registered CVar's ({registered:?})" + ); +} + /// SuperWoW's `LootSparkle`: 1.12 sparkles a lootable corpse and nothing else, so the GameObject /// arm is a deviation that has to be switchable, and the switch off has to answer exactly 1.12. /// diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index 44cf5cb2..2181346c 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -41,7 +41,7 @@ nothing behind it. | `ChatBubbleRange` | done | `chat_bubble.rs`'s `BubbleConfig::range_sq`, replacing the reference's hardcoded 20 yd. Default `20` (today's exact value) — no shipped default published for this one either, within the mod's documented 10-200 yd range | | `NameplateMotion`, `NameplateVerticalSpacing` | todo | needs locating whatever (if anything) already staggers overlapping nameplates, not investigated | | `FoV` | todo, held back | the wiki's only confirmed default (`"1.57"`) doesn't obviously match benilla's existing hardcoded `benilla-world/src/view.rs`'s `CAM_FOVY = FRAC_PI_4` (0.785 rad, 45°) — 1.57 rad is 90°, double. Without confirming whether SuperWoW's `FoV` cvar and `CAM_FOVY` share the same angle convention (vertical half vs. full, radians interpretation) and whether 1.12's true default really is 45°, defaulting to the wiki's literal `1.57` risks a jarring, wrong-looking lens change rather than exposing the existing value as a tunable knob. Left alone rather than guess | -| `LootSparkle` | done, CVar `LootSparkle`, default `1` | `creature_anim/spell_visual.rs`'s `arm_loot_fx` grew a **GameObject** arm beside its unit and corpse ones, hanging the same `"HARDCODED Loot Art"` row (`0x13`) at the same tag. 1.12 sparkles a lootable corpse and nothing else — a chest, a herb, a vein or a quest object is found by eye — and sparkling those arrived in **2.3.0** ("interactable game objects sparkle and display their names"), which is what the mod backports: upstream describes it as "the sparkling effect on lootable treasure". The test is the server's own per-viewer `GO_DYNFLAG_LO_ACTIVATE` in `GAMEOBJECT_DYN_FLAGS` (`GameObject::ActivateToQuest`), already parsed and already documented on its field reader as "the sparkle", so what lights up is exactly what this character can currently use rather than every door and sign in the zone — and the rising and falling edges of that bit are the whole trigger, which is also how the corpse arm works. Defaulted **on**: upstream publishes no default, and the sparkle is present on this install with SuperWoW loaded, which is what "as if its default config loaded" means here; `SetCVar("LootSparkle", "0")` is stock 1.12. `FxStage` is `OneShot` for an object as for a corpse — neither has a sequence of ours to loop, where a unit's loot art loops its first (`0x600640`) | +| `LootSparkle` | done, CVar `LootSparkle`, default `1` | `creature_anim/spell_visual.rs`'s `arm_loot_fx` grew a **GameObject** arm beside its unit and corpse ones, hanging the same `"HARDCODED Loot Art"` row (`0x13`) at the same tag. 1.12 sparkles a lootable corpse and nothing else — a chest, a herb, a vein or a quest object is found by eye — and sparkling those arrived in **2.3.0** ("interactable game objects sparkle and display their names"), which is what the mod backports: upstream describes it as "the sparkling effect on lootable treasure". The test takes **two** arms, because "lootable treasure" is neither one alone: the server's own per-viewer `GO_DYNFLAG_LO_ACTIVATE` in `GAMEOBJECT_DYN_FLAGS` (`GameObject::ActivateToQuest`), documented on its field reader as "the sparkle" — but vmangos raises it through `ActivateToQuest`, so on a chest it means *quest* loot and keying on it alone would light up nothing else — **or** `GAMEOBJECT_TYPE_CHEST`, which is exactly chests, herbs and mining nodes, treasure by construction and a type the client reads without the server's help, excluding `GO_FLAG_IN_USE`/`NO_INTERACT` so an emptied or disabled chest stops. Both arms are a judgement about what the mod does, not a trace of it: if only quest objects light up the first is doing all the work, if doors and signs do the second is too wide. Defaulted **on**: upstream publishes no default, and the sparkle is present on this install with SuperWoW loaded, which is what "as if its default config loaded" means here; `SetCVar("LootSparkle", "0")` is stock 1.12. `FxStage` is immaterial for this effect and the row no longer claims otherwise — `Particles\LootFX.m2` (the `"HARDCODED Loot Art"` row's own model, extracted from this install) holds **one** 2400 ms sequence with `flags = 0`, and bit 0 clear is a looping sequence, so `FxAnimLife::arm` repeats it whatever the stage says | | Healing text, uncap sounds, selection-circle style, outline/shadow text, force-show-auras, XP text | todo | not investigated | | Macro limit 255→510 | done | `script/macros.rs`'s `MAX_MACRO_BODY`; `ui_script/manifest.rs`'s `install_macro_letters_patch` re-patches the stock FrameXML's `SetMaxLetters`/char-count string at boot, since both come from the player's own install | | Targeting-circle spells (Blizzard, Flamestrike) no longer refuse movement | done | `cast_target::is_ground_targeted` (`spell/cast_target.rs`) exempts them in `validator::cast_moving_refusal` (`spell/validator.rs`); the real cast-time interrupt-on-move rule (`inflight.rs`) is untouched | @@ -205,7 +205,7 @@ in this file on the strength of the namreeb research. pepo's v2 has its own | `ItemMixin:ContinueOnItemLoad` — the wait now **asks** | done — a one-line correction to a function already built | `compat/classic_api/item_mixin.rs`. Found while closing `HEARTHSTONE_ITEM_ID` above and worth its own row because it is a second, independent way for the same pfQuest feature to stay off. Upstream's `Item:CreateFromItemID(id) … :ContinueOnItemLoad(cb)` is how a caller *requests* an item's data — `RequestLoadItemDataByID` is the same namespace's bare spelling of the same act — and this port implemented only the waiting half: a `C_Timer` poll on `IsItemDataCached`, because 1.12 has no `GET_ITEM_INFO_RECEIVED` edge. But `C_Item.IsItemDataCachedByID` records no ask *on purpose* (its own registration says so: a caller polling "is it here yet?" should not trigger the query), so the poll only ever resolved for ids something else happened to read, and ran its full 600-tick bound to no effect otherwise. Concretely: a character whose Hearthstone sits in the bank has no bag slot reading template 6948, so nothing asks, so pfQuest's locale check never completes and the addon is off for that character even with the global present. The fix is one read through the `*ByID` family, whose shared `template()` helper puts a miss into `Model::item_stat_asks` for `ui_items/feed.rs` to drain — one source of truth for item data, unchanged, and no new plumbing. Once is enough: the app's pending set keeps an unresolved id across frames. The test drives the whole arm — ask recorded, nothing fired while the answer is absent, callback on the first tick after `set_item_template` — and fails on the first assertion with the line removed | | `C_Map.GetAreas` — **this is why pfQuest drew map icons but no minimap icons** | done — one member, and it is the whole of the minimap half | `compat/classic_api/map.rs`, fed by `ui_world_map::seed_area_names` over the `AreaTableRes` the map catalog is already built from; supersedes the `GetAreas` half of the `C_Map.GetAreas`, `C_Map.GetAreaTriggerInfo` todo row above, which still stands for the trigger member. **The asymmetry was the whole clue and it is not in the minimap code at all** — pfQuest's two passes resolve the player's zone *differently*. `UpdateNodes` (world map) takes `GetMapZones()`'s name and, on a miss, `customids[GetMapInfo()]`, a table built at file scope from `C_Map.GetMapAreaIDs` (`map.lua:632-660`), which has shipped here since that member landed. `UpdateMinimap` has no such fallback: one line, `pfMap:GetMapIDByName(GetRealZoneText())` (`map.lua:1365`), over `pfDB.zones.revloc` — and this install's pfQuest has **deleted** its shipped name table (`db//zones.lua` is a five-line stub saying the names now come from `C_Map.GetAreas`), so with the member absent `pfDB.zones.loc` was empty, `revloc` was nil, the name lookup answered nil, and the pin loop's own gate `if data[mapID] and minimap_sizes[mapID] …` (`map.lua:1383`) indexed by nil and skipped. **Silent by construction**: `data[nil]` is a legal read, so there is no raise, no log line and no missing-global the harness survey can rank — the name is only ever a table key here, the same blind spot `HEARTHSTONE_ITEM_ID` sat in. Measured with the read-back probe before and after (`--probe pfQuest --eval`): `pfDB.zones.loc` 0 entries → 1,480, `revloc` nil → built, and `pfMap:GetMapIDByName(GetRealZoneText())` nil → **12**, the same id the world-map pass already reached through `GetMapAreaIDs`, which is why the two passes now agree on the key pfQuest's node tables are built under. Two further silent losses close with it: `GetMapNameByID` (the browser's and the tracker's zone labels, `UNKNOWN` for every row) and `GetZoneIndex`, which walks the keys of `pfDB.zones.loc` as the zones to ask `C_Map.GetMapOverlays` for (`database.lua:1104`) — with no keys the subzone index was empty and `SearchZoneID` returned early for every subzone-placed objective. **The shape is pfQuest's own generator, not a guess**: `toolbox/extractor.lua:2084-2104` writes `pfDB["zones"][locale][id] = name` straight out of `SELECT * FROM AreaTable ORDER BY id ASC`, so the answer is the **whole** table (1,480 rows here, 1,400 of them subzone rows with no `WorldMapArea` map — the catalog keeps only the ~80 that have one) keyed by id, ascending. What it cannot fix, said plainly: the caller's reverse `name → id` index keeps the *first* id it meets for a name (`database.lua:285-287`) and 70 of the 1,480 names are carried by more than one row (`Westfall` by both 40 and 206, both top-level), so those names resolve by Lua's own table order. That ambiguity is the shipped table's too and nothing in the DBC says which row a name means; picking one here would be this file inventing a fact. **One correction rode along, and it was forced**: `GetMapOverlays`'s `tiles` array is now resolved on **first read** through a per-VM row metatable instead of being built eagerly (`overlay_row_meta`). A cell is measured by *decoding* its file (`benilla-assets::sprite_dimensions`), and until this member landed nothing ever walked every zone's overlays — `GetZoneIndex` now does, and it reads only `areaID` and the four `hitRect*`. Measured on this install: **1,019 tile files across 707 overlay rows, 1.5 s**, which eager tiles would have spent inside pfQuest's `OnUpdate` at the first quest search. pfUI's reveal is the only reader of `tiles` and is itself gated on `C.appearance.worldmap.mapreveal`, so a player with reveal off now pays none of it; a read of `overlay.tiles` answers the same array it always did and is cached on the row after the first. Also seated in the survey fixture (`addon_harness/mod.rs`), beside the race/class tables and the zone globals and for the same reason: an in-world session with an empty `AreaTable` is a state only the harness reaches, and without it the harness is blind to this whole fault class. Tests: `pfquests_minimap_gate_opens_on_the_zone_the_area_name_table_names` transcribes `database.lua:283-289`, `map.lua:583-595`, the `minimap_sizes` metatable and the `map.lua:1383` gate, and asserts the gate is **shut** on an empty table and open on a fed one; `the_whole_area_name_column_reaches_the_vm_including_the_unmapped_rows` fails a feed narrowed to mapped rows; `an_overlay_rows_tiles_are_measured_on_first_read_and_cached_after` counts probe calls and fails an eager build. **One interaction left standing, with the reason.** pfQuest-octo knows about the duplicate names and dedups them — `overwrites.lua:922-963` deletes 16 "phantom" ids whose names collide with a functional zone's, its own comment naming the exact mechanism ("GetMapIDByName scans the locale table with pairs() (non-deterministic order), so it may return a phantom ID instead of the functional one, causing quests and minimap nodes to not appear inside those dungeons"). That deletion lands on `pfDB.zones.-turtle`, which `patchtable.lua:92` merges into `pfDB.zones.` — and then `patchtable.lua:150` calls `pfDatabase:Reload()`, which **re-points** `pfDB.zones.loc` at this call's table and drops the merged copy entirely. So the dedup no longer applies, and those seven dungeon names resolve by table order again. Left alone on purpose: which id a name "means" there is a fact about *pfQuest's database* (which rows carry NPC and object references — the fork says so in as many words), not about `AreaTable.dbc`, and the client has no way to know it. It is also not a regression in either direction — an empty `loc` resolved **every** name to nil, so all seven were already dead — and the custom zones the deleted `-turtle` names existed to supply are in this install's own `AreaTable.dbc` anyway (Dragonmaw Retreat 5600/5601, Hateforge Quarry 5098/5103, Gilneas City, Frostmane Hollow, The Rock of Desolation), which is the whole reason the fork deleted its shipped tables for this call | | 3D unit portraits on an addon's own `PlayerModel` pane (`CreateFrame("PlayerModel", …)` + `:SetUnit`) | todo — **genuinely not built, not a bug**; the 2D path beside it works | What pfUI asks for: `api/unitframes.lua:1455-1456` creates one pane per unit frame (`pfPortraitModel