seals and charms#763
Conversation
ProfileModel rejected Seals and Charms because those sections were not defined. TTS could classify HoradricSeal/Charm, but read_descr treated them as unsupported non-equipment and returned None. Changed files: src/config/profile_models.py src/item/data/item_type.py src/item/descr/read_descr_tts.py src/item/filter.py tests/config/models_test.py tests/item/read_descr_tts_test.py tests/item/filter/filter_test.py
Charms and Seals shared a generic spellcraft filter model, so charm identity fields and seal slot count could not be represented or matched. Charm set data also was not exposed on parsed Items. Changed files: src/config/profile_models.py: added separate CharmFilterModel and SealFilterModel. src/item/filter.py: added charm set/uniqueAspect matching and seal slotCount matching. src/item/descr/read_descr_tts.py, src/item/models.py, src/dataloader.py: parse/store charm set names using sets.json. Importer/editor files updated to create the correct charm/seal model types. Targeted tests updated for model validation, matching, and charm set parsing.
Root cause:
Seals only supported slotCount; there was no separate field for “boosts this set”. Seal TTS also did not store the detected boosted set on the parsed Item.
Changed files:
src/config/profile_models.py: added SealFilterModel.boostedSet, validated against sets.json.
src/item/models.py: added boosted_set_name.
src/item/descr/read_descr_tts.py: set detection now supports both charms and seals; seals populate boosted_set_name.
src/item/filter.py: seal matching now checks boostedSet alongside existing criteria.
Updated targeted tests in tests/config/models_test.py, tests/item/filter/filter_test.py, tests/item/read_descr_tts_test.py.
Seals:
- CrucibleFury:
boostedSet: berserkers_crucible
affixPool:
- count:
- maximum_fury
cjshrader
left a comment
cjshrader
left a comment
There was a problem hiding this comment.
I know you're still working on it, I see there are some unaddressed comments but you're not done.
cjshrader
left a comment
cjshrader
left a comment
There was a problem hiding this comment.
Please review all of your other test data and make sure it's real. I haven't been able to review everything
…ode that didn't appear to be needed.
chrisoro
left a comment
chrisoro
left a comment
There was a problem hiding this comment.
Code review — 4 additional issues found.
|
|
||
| # 3. Try to match the set name from the clean name | ||
| for set_name in Dataloader().set_list: | ||
| set_clean = set_name.replace("_", " ") |
There was a problem hiding this comment.
Why are you cleaning the set_names in set_list? They are already clean. Just check if your incoming name is in the list, no need to loop through the list.
There was a problem hiding this comment.
src/gui/importer/gui_common.py (line 233): removed the loop that cleaned and scanned every set_list entry in match_charm_to_set_or_unique(). The function now checks the normalized incoming name directly against Dataloader().set_list.
There was a problem hiding this comment.
Still open: current code still loops sorted(dataloader.set_list, key=len, reverse=True). If incoming name is already normalized, use direct name_clean in dataloader.set_list.
There was a problem hiding this comment.
Yeah I suspect this entire method is going to end up removed. I've asked @chrizzocb to take a look at how I handled this in maxroll.py because I think a shared method is just not necessary
There was a problem hiding this comment.
Optimized Set Matching in match_charm_to_set_or_unique:
Added a direct membership check if name_clean in dataloader.set_list: return None, name_clean in
gui_common.py
prior to performing substring search loops.
This avoids sorting and iterating over the entire set_list when the incoming name is already normalized.
Added Unit Tests:
Added a test case in
test_gui_common.py
to verify that pre-normalized set name matching works correctly and fast.
gui_common.py — Removed rarity param from create_seal_charm_filter; set min_count=1 for seals; fixed deduplicate_filters to produce unique keys when multiple groups share the same (xN) suffix. maxroll.py — Removed rarity=rarity from call. d4builds.py — Removed rarity=rarity/rarity=None from 3 calls. mobalytics.py — Removed rarity=None from call.
charm set name was missing
…g of seal affixes. Added unique aspect to seals but we are missing mythic data.
| if ( | ||
| name not in Dataloader().affix_dict | ||
| and name not in Dataloader().charm_affix_dict | ||
| and name not in Dataloader().seal_affix_dict |
There was a problem hiding this comment.
L74-78: 🟡 risk: name_must_exist now accepts seal/charm-specific affix names in a regular item Affixes filter section — validation passes but the affix will never match at runtime because _check_item_filters only matches against affix_dict. Scope validation to the correct dict based on filter context, or document that cross-dict names are intentional.
There was a problem hiding this comment.
I don't know how to address this. The affix class is shared and doesn't have knowledge of what class (affix, charm, seal) is using it. I guess it could become the base of a shared class, so we implement AffixAffix or whatever, CharmAffix, SealAffix and let those decide their dicts? Open to ideas here.
| if item.item_type == ItemType.Tribute: | ||
| return self._check_tribute(item) | ||
|
|
||
| if item.item_type == ItemType.HoradricSeal: |
There was a problem hiding this comment.
L674/682: 🔵 nit: dispatches on raw item.item_type == ItemType.HoradricSeal/Charm — inconsistent with is_sigil() used 6 lines above. Wrap in is_seal_or_charm() and dispatch inside, or add is_seal()/is_charm() helpers.
There was a problem hiding this comment.
This is not correct. is_sigil checks across two types. We need different handling per type. The better example is if item.item_type == ItemType.Tribute which is exactly what we're mirroring here.
adding mythic seals to uniques.py via d4data validation via: uv run python -m src.tools.gen_data d4data
|
|
||
| def get_class_name(input_str: str) -> str: | ||
| input_str = input_str.lower() | ||
| input_str = input_str.strip().lower() |
There was a problem hiding this comment.
Same as above, is this addressing a specific issue? Or is it just adding a few lines? It would already log an error and return unknown if it couldn't find a class name.
|
|
||
| # 3. Try to match the set name from the clean name | ||
| for set_name in Dataloader().set_list: | ||
| set_clean = set_name.replace("_", " ") |
There was a problem hiding this comment.
Yeah I suspect this entire method is going to end up removed. I've asked @chrizzocb to take a look at how I handled this in maxroll.py because I think a shared method is just not necessary
update d4build charm and seals import. https://d4builds.gg/builds/whirlwind-barbarian-endgame/?var=5 problem: seal-no mythic name just a mythic aspect... what should we do ?
fix for "to_combat_skills"
comment resolving change
3 participants
ProfileModel rejected Seals and Charms because those sections were not defined. TTS could classify HoradricSeal/Charm, but read_descr treated them as unsupported non-equipment and returned None. Changed files:
src/config/profile_models.py
src/item/data/item_type.py
src/item/descr/read_descr_tts.py
src/item/filter.py
tests/config/models_test.py
tests/item/read_descr_tts_test.py
tests/item/filter/filter_test.py