fix(nui): force a fresh reload of an inherited skin before copying from it

[soloturn]

AI-assisted change proposal. Filed by agent driven by @soloturn via GDD.

Fixes #1621 - skin inheritance breaking specifically across a module environment switch (the loading screen).

Root cause

UISkinBuilder.build() copies the base skin's current properties into a derived skin at build time - a one-time snapshot, not a live reference. UISkinFormat's inherit resolution used Assets.get(), which returns whatever's already cached - during an environment switch that can be the base skin's stale pre-switch data, because ModuleAwareAssetTypeManagerImpl#reloadAssets() (gestalt-asset-core) reloads every already-loaded asset once, in whatever order getLoadedAssetUrns() returns, with no dependency ordering between them. If a derived skin's own reload lands before its base's in that same pass, the snapshot it takes is stale. That's what the original report actually hit: a texture moved to a different module, referenced only through an inherited skin property, missing until the affected skins were reloaded again by hand (reloadSkin - since removed from the engine entirely, for what it's worth).

Fix

At the actual point of the stale read: force the base skin to reload from its data producer right before copying from it, resolved the same way (full or partial urn) the subsequent Assets.get() resolves it, so the asset forced fresh here is exactly the one that lookup finds. This guarantees a current copy regardless of where either skin falls in a reload pass.

Deliberately not touching the shared reload-ordering mechanism (gestalt-asset-core, used by every asset type) or UISkin's inheritance/merge architecture (touches every widget in the game) - both would be the more "complete" fix but are foundational changes I'm not making blind without extensive live visual QA across the whole UI. This fixes it at the actual site of the defect for this specific case.

Also logs when inherit can't be resolved at all (a genuinely different failure - the module providing it not being available) - previously failed completely silently, which is what made this issue so hard to track down in the first place.

Test plan

• Compiles clean.
• Traced the mechanism through UISkinFormat → UISkinBuilder.build() → UISkin → AssetType.loadAsset()/reload() to confirm the fix addresses the actual defect (Asset.reload() mutates the cached instance in place, so forcing it before the read guarantees the subsequent snapshot is current).
• No automated regression test added - simulating the actual race (module environment switch, reload-order-dependent staleness) would need a full asset-scanning/module-environment test harness; noting that gap rather than skipping it silently.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 974466c3-8c4e-450d-ad47-d15a1589a4b4

📥 Commits

Reviewing files that changed from the base of the PR and between 338d7dd and 851120a.

📒 Files selected for processing (1)

• engine/src/main/java/org/terasology/engine/rendering/nui/skin/UISkinFormat.java

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Improved UI skin inheritance handling so inherited styles are reloaded correctly.
  • Added clearer warnings when inherited skin properties cannot be resolved, helping identify configuration or module-environment issues.

Walkthrough

UISkinFormat now reloads inherited skin assets through the module-aware asset manager before resolving them. It logs missing inherited properties when resolution fails.

Changes

UI skin inheritance

Layer / File(s)	Summary
Reload inherited skin assets engine/src/main/java/org/terasology/engine/rendering/nui/skin/UISkinFormat.java	The loader resolves inherited skin URIs and reloads the assets through ModuleAwareAssetTypeManager. It logs a warning when inherited properties remain unresolved.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 85112

This localized skin-loading fix is merge-ready after normal checks and review; no actionable merge-blocking risk remains.

Poem

I’m a rabbit with skins to mend,
Reloading inheritance around each bend.
Modules shift, textures appear,
Warnings speak when paths aren’t clear.
Hop, hop—UI skins now draw near!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the primary fix: reloading inherited skins before copying their properties.
Description check	✅ Passed	The description directly explains the cause, fix, scope, and testing status for the UI skin inheritance issue.
Linked Issues check	✅ Passed	The change addresses issue #1621 by refreshing inherited skins before property copying and logging unresolved inheritance.
Out of Scope Changes check	✅ Passed	The changes are limited to UISkinFormat and directly support the linked issue without unrelated scope expansion.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch soloturn-uiskin-inherit-log-1621

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[soloturn]

AI-assisted update. Filed by agent driven by @soloturn via GDD.

Noting two more complete fixes for the same underlying class of bug, deliberately not attempted here (see #5386 for why - foundational-library changes I'm not making blind without live visual QA):

1. Dependency-ordered reload in gestalt-asset-core. ModuleAwareAssetTypeManagerImpl#reloadAssets() reloads every already-loaded asset once, in whatever order getLoadedAssetUrns() returns - no dependency ordering. The general fix: give AssetData an optional getDependencies() (a UISkinData would return its inherit target), and topologically sort before reloading. This is the correct fix for the general hazard - any asset type with cross-references during a bulk reload, not just skins - but it's a change to a shared library used by every asset type in the engine.

2. Live (lazy) skin inheritance instead of snapshot-at-build. UISkinBuilder.build() copies the base skin's properties into a derived skin once, at build time - a value snapshot, not a live reference. The correct fix: have UISkin hold a reference to its base and fall through to it at query time for anything it has no override of, so a later reload of the base is picked up automatically by every skin built from it, no matter when. This is architecturally right but reworks the fine-grained per-(widget, part, mode) merge in a class every widget in the game resolves its style through - real visual-regression risk without launching the game and checking every screen.

The fix actually shipped in #5386 addresses the specific reported symptom (a stale cross-reference at one known choke point) without either of the above. If either is wanted, it's its own separate piece of work with live verification, not bundled here.