refactor: replace lazy ItemStack suppliers with direct fields, preserve icon metadata - #1774
Open
Travja wants to merge 3 commits into
Open
refactor: replace lazy ItemStack suppliers with direct fields, preserve icon metadata#1774Travja wants to merge 3 commits into
Travja wants to merge 3 commits into
Conversation
…ve icon metadata Split out of #1677. FabledClass, Skill, and FabledAttribute cached their icon/indicator as a Supplier<ItemStack> that was invoked on every access. Replace it with a plain ItemStack field, and make the icon-producing methods (getToolIcon, getIndicator, getToolIndicator) preserve the source icon's display name, lore, damage value, and custom model data instead of dropping them. Data.java is intentionally left untouched here since its Codex item-type lookup removal in the original PR conflicts with the custom item ID preservation added in 1bca114.
Open
- SkillIconMetadataTest: getToolIndicator() and the per-player getIndicator() both preserve the source indicator's damage and custom model data. - FabledAttributeIconMetadataTest: getToolIcon() and getIcon(PlayerData) both preserve the source icon's damage and custom model data. - FabledClassIconTest: getIcon() returns a stable cached ItemStack across calls (no longer re-invoking a Supplier each time), and getToolIcon() still preserves the source icon's lore.
CI caught three real issues in the tests added for this PR: - getToolIndicator()/FabledAttribute#getToolIcon() were incorrectly asserted to preserve damage/custom model data - they don't; only getIndicator(PlayerSkill, boolean) and getIcon(PlayerData) do. Corrected those tests to assert what the methods actually guarantee. - getIndicator_playerSkillOverload test NPE'd because Skill's constructors never populate incompatibleSkills (only load() does), and getIndicator() calls isCompatible() which iterates it. getSkill() now round-trips the source indicator through Data.serializeIcon/parseIcon via a load() call so incompatibleSkills gets initialized while the indicator's damage/custom model data survive the round-trip.
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
2 participants
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.
Split out of #1677 (piece 1 of 8 — see that PR for the full breakdown).
What
FabledClass,Skill, andFabledAttributecached their icon/indicatorItemStackbehind aSupplier<ItemStack>that was re-invoked on every access. This replaces the supplier with a plainItemStackfield, and makes the icon-producing methods (getToolIcon,getIndicator,getToolIndicator) preserve the source icon's display name, lore, damage value, and custom model data — previously these were dropped when building the GUI-facing copy.Why split out separately
This is a self-contained refactor of icon handling with no dependency on the other pieces of #1677 (Divinity integration, flag system, etc).
Note
Data.java's icon parsing/serialization is intentionally not touched here. The original PR simplified it by removing Codex item-type lookups, butdevhas since gained custom item ID preservation for unresolved icons (e.g. Nexo items) in1bca114, and applying that removal would regress that feature. If the Codex-lookup removal is still wanted, it needs to be reconciled with that commit rather than reverting it.Testing
Could not build locally in this environment (private Maven repo
repo.travja.devisn't reachable), so this needs CI/manual verification — in particular, that class/skill/attribute icons still display correctly in GUIs (menus, tool icons) with lore, damage, and custom model data intact after this change.Generated by Claude Code