Repository navigation
Conversation
Implements the optional font_src of decentraland/protocol#487 without an external service: a scene content file, a URL or a Google Fonts family (via Fontsource) is downloaded and built into TextMeshPro and UI Toolkit font assets at runtime; a source that fails keeps the built-in font. The bindings are the pinned @dcl/protocol plus the four font_src lines.
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Lint did not finish ( All Unity tests passed ✅
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Slowest tests
Full report: run summary · results + editor logs: editmode · playmode 🏁 Bare-metal benchmark finished — run #36880301713. Full reportPR #10118, run #36880301713 Overall: 🟢 GPU average improved on Apple M1 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
On demand — comment |
decentraland-bot
added
the
ext-contribution
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: feat: font_src on TextShape, UiText, UiInput and UiDropdown
STEP 2 — Root-cause check
PASS. This is a feature implementation (protocol font_src field on four SDK components), not a bug fix. The diff directly implements the requirement: scenes can specify custom fonts via bundled TTF files or Google Fonts family names resolved through Fontsource. Unsupported or failed sources fall back to the built-in font.
STEP 3 — Design & integration
PASS. Mandatory owner search completed for all new long-lived units:
| New unit | Lifecycle it manages | Existing owner searched | Conclusion |
|---|---|---|---|
FontsLoadingPlugin |
FontsCache, FontFileStore, RuntimeFontAssetFactory |
TexturesLoadingPlugin, TextShapePlugin (peers) |
Follows the plugin pattern exactly — reads from container, wires systems. No lifecycle duplication. |
LoadFontSystem |
Font download & asset creation | LoadTextureSystem, LoadAudioClipSystem (peers in StreamableLoadingGroup) |
Extends LoadSystemBase<FontData, GetFontIntention> — the canonical loading pattern. Correctly registered in AssetsDeferredLoadingSystem. |
FontFileStore |
Temp font files on disk (FreeType re-reads at glyph rasterization) | No existing font-file owner | New responsibility with no prior owner. Ref-counted Lease pattern is correct for files that must outlive the TMP_FontAsset that reads them. |
SceneFontRequest |
Per-entity font promise lifecycle | Per-entity asset requests (textures on backgrounds, etc.) | Embedded struct in each component — the standard per-entity async-asset pattern. Not a lifecycle duplication. |
FontData |
Ref-counted cache entry | TextureData, AudioClipData (peers) |
Extends StreamableRefCountData — the standard cache-entry type. |
RuntimeFontAssetFactory |
None (stateless factory) | N/A | Produces TMP/UIToolkit font assets from file paths. No lifecycle ownership. |
Teardown/consumption trace:
FontFileStore.Lease: Created inStoreAsync→ disposed viaReleaseAfterDestructionAsync(waits one frame forObject.Destroyto complete, then disposes leases on thread pool). Properly paired.- Font promises (
AssetPromise): Created inSceneFontRequest.Update→ released inSceneFontRequest.Release(called from all 4 release systems +FinalizeComponents). Properly paired. TMP_FontAsset/FontAsset: Created inRuntimeFontAssetFactory.Create→ destroyed inFontFamilyAssets.Destroy→ called fromFontData.DestroyObject. Properly paired.FontsCache: Created in plugin constructor, registered withCacheCleaner, disposed inFontsLoadingPlugin.Dispose. Properly paired.
Cross-cutting change — ReleaseReferenceComponentsSystem: The PR adds respectDeferredDeletion so entities with DeferDeletion = true skip pool-return during normal Update. This is a behavioral change affecting ALL pooled reference types, but it is correct: previously, reference components could be returned to the pool while the entity's deletion was still deferred, creating a race with release systems that also check DeferDeletion. FinalizeComponents still processes everything unconditionally. The change is tested (ReleaseReferenceComponentsSystemShould).
STEP 4 — Member audit
All new public members audited. SceneFontRequest.Update/TryConsume/Release are consumed by exactly the 4 component integration points each. FontFamilyAssets.TextMeshProFont/UIToolkitFont are consumed by their respective component systems. FontFileStore.LooksLikeTrueTypeFont/MAX_FILE_BYTES are consumed by LoadFontSystem.DownloadAsync. No single-use-should-merge, absent≠false, or redundant-guard issues found.
STEP 5 — Line-level findings
See inline comments below.
STEP 6 — Complexity assessment
COMPLEX. Touches ECS systems, components, component structural operations, the StreamableLoading pipeline, async/UniTask patterns, plugin registration, assembly definitions, object pooling, and resource cleanup paths.
STEP 7 — QA assessment
QA_REQUIRED: YES. Runtime changes affecting what the user sees (font rendering on TextShape, UiText, UiInput, UiDropdown). The PR includes a deployed test scene on zone with detailed QA instructions.
STEP 8 — Non-blocking warnings
No Main.unity changes detected. ✅
Merge gates
R25 — Outstanding comments: 0 prior review threads (first review). This review raises findings below.
R26 — QA sign-off: Awaiting QA. Test instructions are comprehensive and include a deployed scene on zone. SDK test scene PR linked (sdk7-test-scenes#100).
Categories checked clean
- R1/R2 (hot-path allocations, LINQ): No per-frame allocations in system
Updatepaths. Font operations only run onIsDirtyor promise completion. No LINQ in systems. ✅ - R3 (class refs in structs):
SceneFontRequestholdsFontPromise?(a struct) andstring?— no class reference stored in a struct on a hot path. ✅ - R5 (entity by-ref): All
[Query]methods use the sanctionedin Entityconvention. No structural changes over by-ref entities. ✅ - R6 (acquire/release pairs): All traced above. ✅
- R8 (root cause): Feature implementation, not a symptom fix. ✅
- R9 (logging): All logging uses
ReportHubwith explicitReportCategory.SDK_FONTS. NoDebug.Log. ✅ - R10 (catch-scope):
catch (Exception e) when (e is not OperationCanceledException)inDownloadVariantIfPresentAsync— correct pattern for optional variant downloads. ThecatchinFontFileStore.Clear/Releasetargets specific exceptions (IOException,UnauthorizedAccessException). ✅ - R11 (async/cancellation): CTs threaded through all async paths.
FlowInternalAsyncuses try/finally for cleanup..Forget(handler)used with error handler per DCLA002.SuppressCancellationThrow()used for optional variant downloads. ✅ - R12 (abstractions): No single-implementation interfaces introduced.
FontsCacheis the required cache type for the pipeline. ✅ - R14 (contract honesty):
FontSourceKindenum,FontVariantenum — state modeled in types. ✅ - R15 (dead weight): No commented-out code, unused flags, or platform-specific code without defines. ✅
- R16/R17 (naming): PascalCase types/methods, camelCase locals.
GetFontIntentionfollows theIntentionfamily.LoadFontSystemfollows theLoadSystemfamily.FontDatafollows theDatafamily. ✅ - R19 (file hygiene): One class per file. No build scripts under
Assets/. ✅ - R20 (idiom cluster): No
Concurrent*collections. Fields that should bereadonlyarereadonly. ✅ - R22 (tests): Comprehensive — 1,562 lines across 12 new/updated test files covering all new types, all integration points, font request lifecycle, release systems (including deferred deletion), and deferred loading. Edit-mode and play-mode tests present. ✅
- R23 (comments): No AI-style narrating comments. Comments explain "why" (e.g., "FreeType re-reads the file whenever the atlas needs glyphs"). ✅
- R24 (scope): All files are directly related to the font_src feature.
GraphicsSettings.assetadds the SDF shader needed for runtime font creation.ReleaseReferenceComponentsSystemchange is a necessary cross-cutting fix for deferred deletion. ✅
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ECS systems/components, StreamableLoading pipeline, async/UniTask patterns, plugin registration, assembly definitions, and resource cleanup paths across 4 SDK component types.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review: feat: font_src on TextShape, UiText, UiInput and UiDropdown
Re-review after commit 8a6f22c5 ("fix: bound font downloads and follow threading conventions").
STEP 2 — Root-cause check
PASS. Same as prior review — this is a feature implementation.
STEP 3 — Design & integration
PASS. No structural changes to the design since the previous review. The new FontDownloadHandler is the correct mechanism for streaming download size enforcement within Unity's UnityWebRequest pipeline — it does not introduce a new lifecycle owner, it replaces a post-hoc buffer check with an inline streaming check.
STEP 4 — Member audit
FontDownloadHandler exposes one new public member: LimitExceeded. Consumed by LoadFontSystem.DownloadBytesAsync (2 call sites: post-success check and catch when filter). Not single-use-should-merge — the handler is a separate concern from the system. Clean.
UiElementUtils.ReleaseCustomFont is a new static method consumed by 3 UI release systems — correct extraction of the duplicated ReleaseFont body.
STEP 5 — Line-level findings
Resolved findings from prior review:
| # | Prior finding | Status |
|---|---|---|
| 1 | P1 — Full download before size check | ✅ Resolved: FontDownloadHandler enforces limit during streaming via ReceiveData/ReceiveContentLengthHeader |
| 2 | P2 — npmVersion interpolated without sanitization |
✅ Resolved: semver regex validation in FontsourceCatalog.TryGetTtf (line 94–96) with test coverage for path traversal, query, fragment, percent-encoding |
| 3 | P2 — Null-forgiving ! without justifying comment |
✅ Resolved: comment on line 53 states the contract ("Both download branches populate the required regular face before returning") |
| 4 | P2 — ReleaseFont duplicated across 3 UI release systems |
✅ Resolved: extracted to UiElementUtils.ReleaseCustomFont |
Remaining findings (unchanged from prior review, still active as inline comments):
| # | Finding | File | Severity |
|---|---|---|---|
| 5 | [LogCategory(ReportCategory.PRIMITIVE_MESHES)] should be ReportCategory.SDK_FONTS |
ReleaseTextShapeSystem.cs:22 |
P2 |
| 6 | REGULAR_WEIGHT_INDEX/BOLD_WEIGHT_INDEX duplicated between RuntimeFontAssetFactory and test code |
RuntimeFontAssetFactory.cs:23 |
P2 |
Both have existing inline comments from the prior review that are still applicable.
New code assessment:
FontDownloadHandler: Correct streaming implementation.ReceiveDatareturnsfalseto abort when the limit is exceeded. Buffer growth strategy (Math.Min(maxBytes, ...)) ensures the buffer never exceeds the cap. The|=onLimitExceededinReceiveContentLengthHeadercorrectly latches the flag. 4 new tests cover accept-up-to-limit, overflow-rejection, header-rejection, and UTF-8 decoding.DownloadBytesAsyncinLoadFontSystem:RetryPolicy.NONEis correct — theusing var handlerscopes the handler to the method, and retry is handled at the intention level byLoadSystemBase. The dual error handling (post-successLimitExceededcheck +catch whenfilter) covers both content-length-declared-but-body-smaller and data-exceeded-during-streaming scenarios.MAX_CATALOG_BYTES = 1MB: The Fontsource catalog JSON is also now bounded. Good defense-in-depth.- Threading conventions:
DCLTask.SwitchToThreadPool()andDCLInterlocked.Exchangefollow the project's established wrappers. - Semver regex uses
\A/\zanchors (not^/$) preventing partial matches even withRegexOptions.Multiline.
STEP 6 — Complexity assessment
COMPLEX. Same as prior review — touches ECS systems, StreamableLoading pipeline, async patterns, plugin registration, and resource cleanup paths.
STEP 7 — QA assessment
QA_REQUIRED: YES. Runtime changes affecting font rendering.
STEP 8 — Non-blocking warnings
No Main.unity changes. ✅
Merge gates
R25 — Outstanding comments: 2 remaining P2 findings from prior review. No P0/P1 outstanding.
R26 — QA sign-off: Awaiting QA.
CI: Tests pending on this push (editmode/playmode not yet run).
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ECS systems/components, StreamableLoading pipeline, async/UniTask patterns, plugin registration, assembly definitions, and resource cleanup paths across 4 SDK component types.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Vitaly Popuzin (<@U03V3D7E0NL>) via Slack
Resolve the UIInput overlap with #10143: - UIInputComponent.Initialize resets IsFocused alongside the font state. - UIInputInstantiationSystemShould keeps both the font test and the two panel-based focus tests, with the merged fixture and constructor. - The release-system tests added on both sides declared the same type name in two assemblies. Move the focus one next to ours in the EditMode folder as UIInputReleaseSystemFocusShould; the fixtures cannot be merged because ours asserts on a mocked pool while it needs the real onRelease callback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Correct nullability annotations to match actual behavior (DCLImage.Texture, CommonLoadingArguments, CacheCleaner profiling counters) - Resolve UIDropdownComponent.TextElement once in the constructor, like UIInputComponent - Drop always-true pool null check in UITextReleaseSystem - Replace obsolete TMP enableWordWrapping with textWrappingMode - Remove [Serializable] from the Newtonsoft-only FontsourceFamilyRecord DTO - Remove redundant usings, arguments, qualifiers, using-disposals and a dead constant Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| { | ||
| ReleaseComponentsToPool(query, componentPoolsRegistry); | ||
| } | ||
| public static void ReleaseComponentsToPool(in Query query, IComponentPoolsRegistry componentPoolsRegistry) => |
There was a problem hiding this comment.
ReleaseReferenceComponentsSystem: please move this change to its own PR
The respectDeferredDeletion change isn't related to font_src, but it changes cleanup for every pooled reference component in every scene world. I don't think it should be hidden inside a 3.7k-line feature PR.
As far as I can tell, it also has no effect at runtime today:
ReleaseReferenceComponentsSystemis only added to scene worlds (ECSWorldFactory.cs:95).- Nothing in a scene world sets
DeferDeletion = true. The only places that set it are in the global world (AvatarShapeHandlerSystem,ResolveSceneStateByIncreasingRadiusSystem,RapidSceneReloadDebugSystem, andAvatarCleanUpSystem
resetting it tofalse). This PR only reads the flag outside of tests. - So the new
continueonly runs inReleaseReferenceComponentsSystemShould.
The fix itself makes sense as a guard for the future. It lines up with ReleasePoolableComponentSystem and DestroyEntitiesSystem. It would also stop repeated double-releases to the pool if a scene-world system ever defers deletion. But
that's a speculative change to shared ECS cleanup, and it deserves its own PR and review: something like fix: don't release pooled components of deferred-deletion entities, together with ReleaseReferenceComponentsSystemShould. It also
keeps the font feature easy to revert or bisect.
A small nit for when it moves: the public 2-argument overload (which means false) next to a private 3-argument one is a bit awkward. Putting the check inline in Update(), or giving the flag a default value, would be simpler.
The same applies, less strongly, to the if (deleteEntityIntention.DeferDeletion) return; early-outs in the new ReleaseTextShapeSystem / UITextReleaseSystem / UIInputReleaseSystem / UIDropdownReleaseSystem. They can't trigger in
scene worlds either, though at least they follow the existing ReleasePoolableComponentSystem pattern.
| public static FontFileStore InPersistentData() | ||
| { | ||
| var store = new FontFileStore(Path.Combine(Application.persistentDataPath, DIRECTORY_NAME)); | ||
| store.Clear(); |
There was a problem hiding this comment.
This means that fonts dont survive sessions, right? How can we do it?
:AG2:
I get why FontFileStore exists: FreeType keeps re-reading the font from its path, and DiskCache can't guarantee a file stays put (LRU clean-up or an overwrite may remove it). But could we still use the disk cache for the downloaded bytes? DownloadAsync would look up the raw IDiskCache by URL first and only hit the network on a miss (then PutAsync the result), while FontFileStore keeps its role of providing the pinned copy FreeType reads from. It would just need an IDiskCache passed in through FontsLoadingPlugin, the same way TexturesLoadingPlugin gets one.
| this.referenceFont = referenceFont; | ||
| } | ||
|
|
||
| public FontFamilyAssets? Create(string assetName, string regularFilePath, string? boldFilePath = null, string? italicFilePath = null, string? boldItalicFilePath = null) |
There was a problem hiding this comment.
Every font creates both a TMP and a UI Toolkit asset for every face, so a Fontsource family ends up with 8 CreateFontAsset calls, each with its own 1024² atlas (which can grow into more atlases). That's twice the atlas memory whenever the font is only used by TextShape or only by UI. And since it all runs on the main thread in one go after SwitchToMainThread in LoadFontSystem, it's likely to cause a noticeable hitch.
Could the TMP and UI Toolkit variants be created lazily, the first time a consumer of that type asks for them?
| private async UniTask DownloadFamilyAsync(GetFontIntention intention, FontFileStore.Lease?[] files, CancellationToken ct) | ||
| { | ||
| byte[] catalogBytes = await DownloadBytesAsync(intention.CommonArguments, MAX_CATALOG_BYTES, ct); | ||
| FontsourceFamilyRecord record = JsonConvert.DeserializeObject<FontsourceFamilyRecord>(Encoding.UTF8.GetString(catalogBytes)) |
There was a problem hiding this comment.
I worry this runs on the main thread, could we swithc to the thread pool?
| return false; | ||
| } | ||
|
|
||
| private static bool TryGetTtf(FontsourceFamilyRecord record, string weight, string style, out string url) |
There was a problem hiding this comment.
The ttf URL is taken from the Fontsource response as-is and downloaded, and those bytes then go straight into FreeType, which has a long CVE history with malformed fonts. The 16 MB cap and the sfnt header check help, but if the API response were ever tampered with, the client would download from any host it's given.
Could we validate the host against a small allowlist (e.g. cdn.jsdelivr.net) before downloading? It's cheap and limits the blast radius.
| - {fileID: 4800000, guid: d197ba80c994be24b8d3c757150237f4, type: 3} | ||
| - {fileID: 4800000, guid: c87696e8d053c4e49a546cd5b75e224d, type: 3} | ||
| - {fileID: 4800000, guid: 7c27dfc00496c5442a73aeede131877d, type: 3} | ||
| - {fileID: 4800000, guid: fe393ace9b354375a9cb14cdbbc28be4, type: 3} |
There was a problem hiding this comment.
Is this really needed?
Assets/Rendering/ShaderVariants.shadervariants is already listed in GraphicsSettings → m_PreloadedShaders, and it already references TMP_SDF-Mobile with 3 variants (none, UNDERLAY_ON, UNITY_UI_CLIP_RECT). A preloaded variant collection pulls its shaders into the build and loads them at startup, so Shader.Find("TextMeshPro/Mobile/Distance Field") should already resolve in a player without the Always Included entry.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: feat: font_src on TextShape, UiText, UiInput and UiDropdown
STEP 2 — Root-cause check
This PR implements a new protocol feature (font_src from decentraland/protocol#489), not a bug fix. The diff adds the capability for scenes to specify custom fonts via bundled TTF files or Fontsource family names. The change addresses the cause (no custom font support) directly — PASS.
STEP 3 — Design & integration
Owner search for new long-lived units:
-
FontsLoadingPlugin— a newIDCLWorldPluginthat ownsFontsCache,FontFileStore, andRuntimeFontAssetFactory. Constructed inStaticContainer(line 292), injected per-world viaInjectToWorld. This follows the existing plugin pattern (TextShapePlugin,SceneUIPlugin). The font subsystem is orthogonal to existing font storage (IFontsStorageholds the 3 built-in fonts) and correctly introduces a parallel loading pipeline for scene-provided fonts. ✅ -
LoadFontSystem— extendsLoadSystemBase<FontData, GetFontIntention>, the same base used byLoadTextureSystem,LoadAudioClipSystem, etc. Registered inAssetsDeferredLoadingSystem. Follows the established streamable loading pattern. ✅ -
FontFileStore— holds persistent state (aDictionary<string, int>reference counter) outside ECS. Per CLAUDE.md §1, systems must not hold persistent collections — butFontFileStoreis not a system; it is an infrastructure service (likeCacheCleaneror the disk cache) that manages file lifecycle. The ref-counting is necessary because FreeType re-reads font files from disk, so files must outlive individual font assets. The store is constructed once inFontsLoadingPluginand shared across worlds. This is a legitimate infrastructure concern, not a lifecycle violation. ✅ -
SceneFontRequest— a struct embedded in each component (TextShapeComponent,UITextComponent,UIInputComponent,UIDropdownComponent). It manages promise lifecycle (create, consume, release) co-located with the component that uses it. This is the correct ECS-aligned pattern — the state lives on the component, not in the system. ✅ -
ReleaseTextShapeSystem— new system that handles font cleanup on entity destruction, component removal, and world disposal. The existingTextShapePluginhad no dedicated release system for TextShape (it relied onReleasePoolableComponentSystem). The new system addsIFinalizeWorldSystemfor world disposal and handlesDeleteEntityIntention.DeferDeletion. This is the correct lifecycle owner for font cleanup. ✅
Teardown trace:
SceneFontRequest.Release()→promise.TryDereference(world)+promise.ForgetLoading(world)— traced in all 4 release systems (ReleaseTextShapeSystem,UITextReleaseSystem,UIInputReleaseSystem,UIDropdownReleaseSystem) and in all 3 lifecycle paths (entity destruction, component removal, world finalization). ✅FontData.DestroyObject()→FontFamilyAssets.Destroy()+FontFileStore.ReleaseAfterDestructionAsync()— traced.ReleaseAfterDestructionAsyncwaits one frame forObject.Destroyto complete, then releases file leases on the thread pool. ✅FontFileStore.Lease.Dispose()→DCLInterlocked.Exchangeensures single-release, thenFontFileStore.Release()decrements ref count and deletes file at zero. ✅FontFamilyAssets.Destroy()→TMP_ResourceManager.RemoveFontAsset+SafeDestroyfor all TMP and UI Toolkit assets. ✅
Design verdict: PASS. The new subsystem follows established patterns, does not duplicate existing lifecycle owners, and has complete teardown coverage.
STEP 4 — Member audit
SceneFontRequest.Update()— called from 4 instantiation/update systems, returnsboolindicating whether the source changed. Single responsibility, correctly resetsCustomFontin callers. ✅SceneFontRequest.TryConsume()— called from 4ApplyLoadedFontqueries. Early-returns on null/consumed promise. ✅SceneFontRequest.Release()— called from 4 release systems +Update()(on source change). ✅UiElementUtils.SetFont()— centralizes font application for UI elements (3 callers + 4ApplyLoadedFontcallers). ✅UiElementUtils.ReleaseCustomFont()— centralizes font cleanup for UI elements (3 UI release systems). ✅FontFileStore.LooksLikeTrueTypeFont()— validates SFNT header. Used inLoadFontSystem.DownloadAsync. ✅
STEP 5 — Line-level findings
See inline comments below.
STEP 6 — Complexity assessment
COMPLEX. This PR introduces a new ECS subsystem (systems, components, plugin, cache), modifies component structural operations, adds async loading flows, changes plugin/container wiring, and touches 110+ files across infrastructure and SDK components.
STEP 7 — QA assessment
QA_REQUIRED: YES. The changes affect runtime rendering of text in scenes (TextShape, UI Text, UI Input, UI Dropdown), introduce network downloads (Fontsource API, CDN), and modify how fonts are applied to all text-bearing SDK components.
STEP 8 — Non-blocking warnings
No Main.unity scene modification detected. ✅
Merge gates
R25 — Outstanding comments: There are 6 prior inline comments from decentraland-bot and 6 from dalkia that remain unresolved. Key open threads include: ReleaseReferenceComponentsSystem scope concern (dalkia), font persistence across sessions (dalkia), atlas memory budget (dalkia), main-thread concern for font loading (dalkia), Fontsource TTF CVE surface (dalkia), and GraphicsSettings shader inclusion question (dalkia). These must be addressed before approval.
R26 — QA sign-off: Awaiting QA. The PR includes detailed test instructions with a deployed test scene. QA_REQUIRED: YES.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: New ECS font loading subsystem with async flows, plugin wiring, 4 component integrations, cache management, and file I/O
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| return false; | ||
|
|
||
| if (!string.IsNullOrEmpty(record.npmVersion) | ||
| && !Regex.IsMatch(record.npmVersion, @"\A[0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?\z", RegexOptions.CultureInvariant)) |
There was a problem hiding this comment.
[P2 — R1] Regex.IsMatch without RegexOptions.Compiled allocates the compiled automaton on every call. This runs on the font loading path (not per-frame), so it is not a hot-path blocker, but the repo's allocation discipline applies to all runtime code. Pre-compile the regex as a static readonly Regex field.
| && !Regex.IsMatch(record.npmVersion, @"\A[0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?\z", RegexOptions.CultureInvariant)) | |
| && !SemVerRegex.IsMatch(record.npmVersion)) |
With a field added at the top of the class:
private static readonly Regex SemVerRegex = new Regex(
@"\A[0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?\z",
RegexOptions.CultureInvariant | RegexOptions.Compiled);| private const int BOLD_WEIGHT_INDEX = 7; | ||
|
|
There was a problem hiding this comment.
[P2 — R18/R13] Weight table indices duplicated between production and test code. REGULAR_WEIGHT_INDEX = 4 and BOLD_WEIGHT_INDEX = 7 are defined identically in RuntimeFontAssetFactory and RuntimeFontAssetFactoryShould. If TMP changes the weight table layout, only one site will be updated. Extract these into a shared location (e.g. a FontWeightConstants class in the Fonts namespace) or have the tests reference the production constants via InternalsVisibleTo.
| private const int BOLD_WEIGHT_INDEX = 7; | |
| internal const int REGULAR_WEIGHT_INDEX = 4; | |
| internal const int BOLD_WEIGHT_INDEX = 7; |
Then in the test, use RuntimeFontAssetFactory.REGULAR_WEIGHT_INDEX instead of a local constant.
| [UpdateBefore(typeof(ReleasePoolableComponentSystem<TextMeshPro, TextShapeComponent>))] | ||
| [LogCategory(ReportCategory.PRIMITIVE_MESHES)] | ||
| [ThrottlingEnabled] | ||
| public partial class ReleaseTextShapeSystem : BaseUnityLoopSystem, IFinalizeWorldSystem |
There was a problem hiding this comment.
[P2 — R9] ReportCategory mismatch. This system uses [LogCategory(ReportCategory.PRIMITIVE_MESHES)] (inherited from existing TextShape systems), but font loading/release is a distinct concern with its own ReportCategory.SDK_FONTS. All the new Fonts infrastructure uses SDK_FONTS. For consistency and to help filter font-related logs, consider using SDK_FONTS here too — or keep PRIMITIVE_MESHES if TextShape lifecycle logging should stay unified regardless of the font subsystem.
This is informational — the existing TextShape systems all use PRIMITIVE_MESHES, so keeping it is defensible for consistency within the TextShape subsystem.
| using DCL.SDKComponents.SceneUI.Components; | ||
| using DCL.SDKComponents.SceneUI.Defaults; | ||
| using ECS.StreamableLoading.Fonts; | ||
| using System.Linq; |
There was a problem hiding this comment.
[P2 — R15] Unused using System.Linq import. No LINQ methods are called in this file. Remove it to keep the file clean and avoid accidental LINQ usage in future edits.
| using System.Linq; |
| { | ||
| ReleaseComponentsToPool(query, componentPoolsRegistry); | ||
| } | ||
| public static void ReleaseComponentsToPool(in Query query, IComponentPoolsRegistry componentPoolsRegistry) => |
There was a problem hiding this comment.
[P2 — R24] Scope: respectDeferredDeletion change is unrelated to font_src. This change modifies ReleaseReferenceComponentsSystem to skip entities with DeferDeletion = true during normal updates, which is a correctness fix for component pooling — but it is orthogonal to the font_src feature. As dalkia noted in a prior comment, this should ideally be in its own PR.
The change itself is correct and well-tested (ReleaseReferenceComponentsSystemShould covers both paths), but bundling it here makes the font_src PR harder to revert independently.
| } | ||
|
|
||
| public Texture Texture | ||
| public Texture? Texture |
There was a problem hiding this comment.
[P2 — R24/R7] Scope: Texture? nullability fix is tangential to font_src. Making DCLImage.Texture nullable is a correct R7 fix (the setter already handles null via SetTexture), but it is unrelated to the font feature. Consider splitting this into its own commit or PR for cleaner history.
| // Both download branches populate the required regular face before returning. | ||
| FontFamilyAssets? assets = fontAssetFactory.Create(intention.Src, files[0]!.Path, files[1]?.Path, files[2]?.Path, files[3]?.Path); |
There was a problem hiding this comment.
[P2 — R7] Null-forgiving ! without justifying comment. files[0]!.Path uses the null-forgiving operator. Both code paths above (DownloadFamilyAsync sets files[0], DownloadAsync returns non-null) guarantee files[0] is non-null at this point, but per R7 the ! should carry a brief comment explaining the invariant.
| // Both download branches populate the required regular face before returning. | |
| FontFamilyAssets? assets = fontAssetFactory.Create(intention.Src, files[0]!.Path, files[1]?.Path, files[2]?.Path, files[3]?.Path); | |
| // Both download branches populate the required regular face before returning. | |
| FontFamilyAssets? assets = fontAssetFactory.Create(intention.Src, files[0]!.Path, files[1]?.Path, files[2]?.Path, files[3]?.Path); // files[0] set by DownloadAsync/DownloadFamilyAsync above |
| } | ||
|
|
||
| private void WireVariant(string assetName, string? filePath, FontVariant variant, TMP_FontAsset textMeshProRegular, FontAsset uiToolkitRegular, | ||
| List<TMP_FontAsset> textMeshProAssets, List<FontAsset> uiToolkitAssets) |
There was a problem hiding this comment.
[P2 — R10] Bare catch re-throws but destroys assets first — good, but the catch-all has no stated type. The catch block correctly cleans up on failure, but per R10 an untyped catch should state the exception type if known, or carry a brief comment explaining why it must be catch-all.
| } | |
| private void WireVariant(string assetName, string? filePath, FontVariant variant, TMP_FontAsset textMeshProRegular, FontAsset uiToolkitRegular, | |
| List<TMP_FontAsset> textMeshProAssets, List<FontAsset> uiToolkitAssets) | |
| catch (Exception) | |
| { | |
| FontFamilyAssets.Destroy(textMeshProAssets, uiToolkitAssets); | |
| throw; | |
| } |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Jarvis review: font_src on TextShape / UiText / UiInput / UiDropdown
Inspected head: 4966c7b5. The only change since the previous bot round (9e7cc84) is the profiler markers in RuntimeFontAssetFactory. The lint pre-flight script (scripts/lint/custom-rules.sh) is not on this branch, so the review-bar rules were checked by hand.
Step 2: Root cause
This is a new feature, not a fix. Scenes can set font_src to a bundled TTF, resolved only through the scene's content-hash map, or to a Google Fonts family name, resolved through Fontsource. External URLs are rejected up front (FontSrcResolver). The approach addresses the feature itself; nothing hides a symptom.
Step 3: Design and integration (owner search)
- Loading stack (
LoadFontSystem,GetFontIntention,FontData,FontsCache,FontsLoadingPlugin): follows the existing StreamableLoading pattern. The siblings areTextures/(TexturesLoadingPlugin/TexturesCache),AudioClips/(AudioSourcesPlugin/AudioClipsCache) andCache/RefCountStreamableCacheBase. It is registered inCacheCleanerandAssetsDeferredLoadingSystemthe same way those caches are. Placement OK. - Release systems (
ReleaseTextShapeSystem, plus font release in the UiText/UiInput/UiDropdown release systems, allIFinalizeWorldSystem):ReleasePoolableComponentSystemcan't dereference promises because its providerDispose()has noWorld, so a per-component release step is justified. I traced all three paths (component removed, entity destroyed, world finalized): each releases the font exactly once, and the pooled TMP/VisualElement is reset to a built-in font. The one exception is the stale-font cases flagged inline. FontFileStoreduplicates part ofCache/Disk/(CacheDirectory,DiskCache.PutAsynctemp-then-move,FilesLock). A pinned file is a genuine extra requirement, because FreeType re-reads it. The maintainer's open thread already asks for the byte cache to go throughIDiskCache, so it is not repeated inline.FontsourceCatalog: there is no existing owner for Fontsource. Third-party endpoints belong inDecentralandUrlsSource(finding inline).- Teardown trace: every
FontPromise.Createis matched bySceneFontRequest.Release(TryDereference+ForgetLoading). EveryFontFileStore.Leaseis released inLoadFontSystem'sfinallyor inFontData.DestroyObject. EveryFontDatais released throughDisposeAbandonedResultor the refcounted cache. No subscriptions or CTS are added.
Step 4: Member audit
SceneFontRequest.Srcand.Promise: public, but used only by tests outside the struct.UiElementUtils.SetFont: 4 consumers.UiElementUtils.ReleaseCustomFont: 3 consumers.UiElementUtils.ClearCustomFont: 1 consumer (inline).TMPProSdkExtensions.SetFont: 2 consumers.FontFileStore.MAX_FILE_BYTES/LooksLikeTrueTypeFont: used only byLoadFontSystem; this is download validation sitting on the storage type.CustomFontis kept as a separate field on all four components, and eachFontRequest.Update(...)call site resets it by hand. Folding the loadedFontFamilyAssets?intoSceneFontRequestwould remove the four copies of that reset (P2, R14; no inline comment).
Step 5: Findings (inline, each with a suggestion where there is a source fix)
| # | Sev | Rule | Location | Summary |
|---|---|---|---|---|
| 1 | P1 | R22 | TextShape.gen.cs:713 (+ UiText/UiInput/UiDropdown) |
font_src was generated from the unmerged decentraland/protocol#489; @dcl/protocol pin not bumped |
| 2 | P1 | R13 / privacy | FontsourceCatalog.cs:10,49-50 |
Hardcoded third-party API URL; any scene can make every visitor contact Fontsource/jsDelivr without a media-host permission |
| 3 | P1 | R22 | LoadFontSystem.cs:73 |
No loader tests for the family path (missing regular, face fallback, cancel mid-WhenAll) |
| 4 | P2 | R9 | FontSrcResolver.cs:24-38 |
Each valid family name also logs a "not found in fileToHash" SCENE_LOADING warning |
| 5 | P2 | R8 | TMPProSdkExtensions.cs:27 |
Old custom font can stay on the TMP after release, then be destroyed while in use |
| 6 | P2 | R8 | UiElementUtils.cs:335-336 |
Same for UI; also a negative font index still throws |
| 7 | P2 | availability | FontsourceCatalog.cs:46 |
Unbounded family-name length from scene input |
| 8 | P2 | R18 | LoadFontSystem.cs:41 |
Magic 4 / slot-to-variant mapping by convention |
| 9 | P2 | R23 | RuntimeFontAssetFactory.cs:116 |
Comment narrates project settings |
| 10 | P2 | Step 4 | UiElementUtils.cs:339-340 |
Single-use public ClearCustomFont |
Checked and clean:
- No LINQ or per-frame allocation was added in
[Query]/Update(R1/R2). - No structural change over a by-ref
Entity(R5). - Catch blocks name the exception type, and cancellation is passed through (R10/R11).
- No
Concurrent*collections (R20). - HTTP goes through
IWebRequestController. - Downloads are size-capped against both the declared length and the running total.
- The family id charset (
[A-Za-z0-9-]) rules out URL/path injection. - Files in the store are named by SHA-256, so there is no path traversal.
Merge gates
-
R25: open threads. 14 review threads are unresolved at this head. They include 6 from @dalkia, none addressed since
13370a2:- move the
ReleaseReferenceComponentsSystemchange to its own PR - reuse
IDiskCachefor downloaded bytes, so fonts survive sessions - create TMP and UI Toolkit assets lazily (8
CreateFontAssetcalls and 8 1024² atlases per family, all on the main thread) - JSON parse on the main thread
- host allowlist for Fontsource
ttfURLs - whether the
GraphicsSettings.assetalways-included shader is needed
The other 8 are prior bot findings:
Regexallocation, the duplicated weight-table indices, theReportCategorymismatch onReleaseTextShapeSystem, the unusedSystem.Linqimport, theDCLImagescope item,files[0]!, and the barecatch. This review does not repeat them inline. - move the
-
R26: QA. The PR has no
no QA neededlabel, and QA has not signed off yet (the checklist item is unchecked). The test instructions are literal and followable, and they name the deployedsdk7testscenes.dcl.ethscene at 84,-6. QA must cover both Windows and Mac. -
CI: the Build status comment reports success; the semantic-title and security-review checks pass.
-
Labels:
new-dependencyandext-contribution. The new runtime dependency is on Fontsource/jsDelivr (finding 2).
REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Adds a new StreamableLoading pipeline (system, refcounted cache, disk store, plugin wiring) and changes ECS instantiate/update/release systems for TextShape and three SceneUI components, plus regenerated protocol code.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| } | ||
|
|
||
| /// <summary>Field number for the "font_src" field.</summary> | ||
| public const int FontSrcFieldNumber = 22; |
There was a problem hiding this comment.
[P1 · R22] The generated font_src fields come from an unreleased protocol branch.
FontSrcFieldNumber (TextShape 22, UiText 9, UiInput 16, UiDropdown 13) comes from decentraland/protocol#489, which is still open. scripts/package.json still pins "@dcl/protocol": "^1.0.0-35089025179.commit-5810768", the same version as dev, so the next npm run build-protocol would regenerate these four files without font_src and silently break the feature. The field numbers could also still change during protocol review, and that would be a wire-level mismatch with deployed scenes.
Fix (not a source-line fix, so no suggestion block): merge protocol#489 → bump @dcl/protocol in scripts/package.json to the released version that contains it → regenerate TextShape.gen.cs, UiText.gen.cs, UiInput.gen.cs and UiDropdown.gen.cs from that version and commit the result.
| public static string ApiUrl(string familyId) => | ||
| API_BASE_URL + familyId; |
There was a problem hiding this comment.
[P1 · R13 + privacy] The third-party API base URL (line 10, API_BASE_URL) is a scattered hardcoded const, and scenes can reach it without any permission.
R13: URLs are resolved through DecentralandUrlsSource. Third-party APIs are already centralised there too; see DecentralandUrl.ManaUsdRateApiUrl → DecentralandUrlsSource.cs:421. Being there also lets the endpoint be swapped (for a mirror or proxy) or turned off without shipping a client build.
Privacy: setting font_src: "Roboto" makes every visitor's client call api.fontsource.org and then cdn.jsdelivr.net, up to 5 requests. That sends the player's IP, plus a font choice that effectively identifies the scene, to two outside operators. Other outside media needs the scene's allowedMediaHostnames (SceneData.TryGetMediaUrl / IsUrlDomainAllowed); this path skips that. Whether family names should need that permission, or a Decentraland-run mirror, is a product decision. Please record it explicitly in the PR or in protocol#489.
Fix: add FontsourceFontsApi to DecentralandUrl, map it in DecentralandUrlsSource.Url to https://api.fontsource.org/v1/fonts/, delete API_BASE_URL, and pass IDecentralandUrlsSource from StaticContainer → FontsLoadingPlugin → the resolver:
| public static string ApiUrl(string familyId) => | |
| API_BASE_URL + familyId; | |
| public static string ApiUrl(IDecentralandUrlsSource urlsSource, string familyId) => | |
| urlsSource.Url(DecentralandUrl.FontsourceFontsApi) + familyId; |
| } | ||
| } | ||
|
|
||
| private async UniTask DownloadFamilyAsync(GetFontIntention intention, FontFileStore.Lease?[] files, CancellationToken ct) |
There was a problem hiding this comment.
[P1 · R22] The Fontsource family path has no loader tests.
LoadFontSystemShould only builds FontSourceKind.File intentions. FontSourceKind.FontsourceFamily only appears in FontSrcResolverShould, so none of the DownloadFamilyAsync behaviour is tested:
- a catalog with no regular face →
FontLoadException - a failed bold/italic face → warning and fallback to regular, with the lease released
- cancellation during
UniTask.WhenAll→ every lease written so far is released, and nothing is left inFontFileStore - a
npmVersionthat fails the semver check → the face is skipped
Fix: add LoadFontSystemShould cases that set Kind = FontSourceKind.FontsourceFamily and point CommonArguments.URL at a local file:// catalog JSON fixture whose variants.*.url.ttf point at the existing test TTF. Use one fixture per scenario above, and assert on the result and on the store's lease count. (This is a test gap, so there is no suggestion block.)
| FontSourceKind kind = FontSourceKind.File; | ||
|
|
||
| if (!sceneData.TryGetContentUrl(fontSrc, out URLAddress url)) | ||
| { | ||
| string? familyId = FontsourceCatalog.ToFamilyId(fontSrc); | ||
|
|
||
| if (familyId == null) | ||
| { | ||
| ReportHub.LogWarning(ReportCategory.SDK_FONTS, $"font_src \"{fontSrc}\" is neither a content file of the scene {sceneData.SceneShortInfo} nor a font family name"); | ||
| return false; | ||
| } | ||
|
|
||
| kind = FontSourceKind.FontsourceFamily; | ||
| url = URLAddress.FromString(FontsourceCatalog.ApiUrl(familyId)); | ||
| } |
There was a problem hiding this comment.
[P2 · R9] Every family name also logs a misleading SCENE_LOADING warning.
TryGetContentUrl is tried first, and SceneHashedContent.TryGetContentUrl logs LogWarning(SCENE_LOADING, "... {contentPath} not found in fileToHash") on every miss. So each valid family name ("Lora", "Bungee Shade") raises a "file not found" warning for an expected path. ToFamilyId only accepts [A-Za-z0-9 -], which a content path with an extension or / never matches. Classifying the family name first keeps the content lookup, and its warning, for things that really are paths. (One behaviour change: an extensionless bundled file whose name is a valid family id would resolve as a family. TTF files in scene content carry .ttf, so this seems acceptable; please confirm.)
| FontSourceKind kind = FontSourceKind.File; | |
| if (!sceneData.TryGetContentUrl(fontSrc, out URLAddress url)) | |
| { | |
| string? familyId = FontsourceCatalog.ToFamilyId(fontSrc); | |
| if (familyId == null) | |
| { | |
| ReportHub.LogWarning(ReportCategory.SDK_FONTS, $"font_src \"{fontSrc}\" is neither a content file of the scene {sceneData.SceneShortInfo} nor a font family name"); | |
| return false; | |
| } | |
| kind = FontSourceKind.FontsourceFamily; | |
| url = URLAddress.FromString(FontsourceCatalog.ApiUrl(familyId)); | |
| } | |
| FontSourceKind kind = FontSourceKind.FontsourceFamily; | |
| string? familyId = FontsourceCatalog.ToFamilyId(fontSrc); | |
| URLAddress url; | |
| if (familyId != null) | |
| url = URLAddress.FromString(FontsourceCatalog.ApiUrl(familyId)); | |
| else if (sceneData.TryGetContentUrl(fontSrc, out url)) | |
| kind = FontSourceKind.File; | |
| else | |
| { | |
| ReportHub.LogWarning(ReportCategory.SDK_FONTS, $"font_src \"{fontSrc}\" is neither a content file of the scene {sceneData.SceneShortInfo} nor a font family name"); | |
| return false; | |
| } | |
| TextMeshPro tmpText = textShapeComponent.TextMeshPro; | ||
|
|
||
| tmpText.font = fontsStorage.Font(textShape.Font) ?? tmpText.font; | ||
| SetFont(ref textShapeComponent, textShapeComponent.CustomFont ?? fontsStorage.Font(textShape.Font) ?? tmpText.font); |
There was a problem hiding this comment.
[P2 · R8] A released custom font can stay on screen and then be destroyed while in use.
When font_src changes or is cleared, UpdateTexts calls FontRequest.Update. That dereferences the old promise and sets CustomFont = null, and then this line runs. If fontsStorage.Font(textShape.Font) returns null (its return type is TMP_FontAsset?, e.g. for an unmapped font value), the ?? tmpText.font fallback keeps the old custom font. Nothing references it any more, so CacheCleaner → FontData.DestroyObject destroys it and the text disappears. ReleaseTextShapeSystem.ReleaseFont doesn't fix it either: it returns early because CustomFont == null, so the pooled TMP goes back still pointing at a destroyed asset. Before this PR the fallback could only keep a built-in font, so it was harmless.
| SetFont(ref textShapeComponent, textShapeComponent.CustomFont ?? fontsStorage.Font(textShape.Font) ?? tmpText.font); | |
| SetFont(ref textShapeComponent, textShapeComponent.CustomFont ?? fontsStorage.Font(textShape.Font) ?? fontsStorage.Font(DCL.ECSComponents.Font.FSansSerif) ?? tmpText.font); |
(The enum type is written out in full because this file imports both UnityEngine and DCL.ECSComponents.)
| if (fontIndex < styleFontDefinitions.Length) | ||
| element.style.unityFontDefinition = styleFontDefinitions[fontIndex]; |
There was a problem hiding this comment.
[P2 · R8] The UI side has the same stale-font problem as TMPProSdkExtensions.Apply.
When customFont is null and fontIndex is out of range, nothing is written. The element keeps new StyleFontDefinition(<released custom FontAsset>) after FontRequest.Update has dropped its reference, and UI Toolkit keeps drawing with it until CacheCleaner destroys it. A negative enum value also still throws IndexOutOfRangeException here (that predates this PR).
| if (fontIndex < styleFontDefinitions.Length) | |
| element.style.unityFontDefinition = styleFontDefinitions[fontIndex]; | |
| if (fontIndex >= 0 && fontIndex < styleFontDefinitions.Length) | |
| element.style.unityFontDefinition = styleFontDefinitions[fontIndex]; | |
| else | |
| ClearCustomFont(element); |
| id.Append(char.ToLowerInvariant(c)); | ||
| } | ||
|
|
||
| return id.Length > 0 ? id.ToString() : null; |
There was a problem hiding this comment.
[P2 · availability] The family name has no length limit, and each distinct name costs network requests.
font_src is an unbounded string from the scene. Any length of [A-Za-z0-9 -] becomes a Fontsource API request, and each unknown name is one more outgoing request, and a cache entry, per client. A scene cycling through generated names turns every visitor into a request source against Fontsource, which can also get their IP rate-limited for other scenes. Real family ids are short, so capping the id length costs nothing. A per-scene cap on distinct families (e.g. 8) is also worth considering as a follow-up.
| return id.Length > 0 ? id.ToString() : null; | |
| return id.Length is > 0 and <= MAX_FAMILY_ID_LENGTH ? id.ToString() : null; |
with /// <summary>Longest Fontsource family id accepted from a scene.</summary> private const int MAX_FAMILY_ID_LENGTH = 64; declared at the top of the type.
|
|
||
| protected override async UniTask<StreamableLoadingResult<FontData>> FlowInternalAsync(GetFontIntention intention, StreamableLoadingState state, IPartitionComponent partition, CancellationToken ct) | ||
| { | ||
| var files = new FontFileStore.Lease?[4]; |
There was a problem hiding this comment.
[P2 · R18] The magic 4 and the slot mapping 0..3 match FontVariant only by convention.
files[0..3] must match FontVariant.Regular/Bold/Italic/BoldItalic, and the same 4 appears again in RuntimeFontAssetFactory.Create (new List<...>(4)). Derive the count from the enum and index by variant (files[(int)FontVariant.Regular], …) at lines 49, 54, 82 and 90–92 so the mapping cannot drift.
| var files = new FontFileStore.Lease?[4]; | |
| var files = new FontFileStore.Lease?[FONT_VARIANTS_COUNT]; |
with /// <summary>One file slot per <see cref="FontVariant"/>, indexed by the enum value.</summary> private const int FONT_VARIANTS_COUNT = (int)FontVariant.BoldItalic + 1; next to MAX_CATALOG_BYTES.
| asset.name = name; | ||
| owned.Add(asset); | ||
|
|
||
| // CreateFontAsset looks up "TextMeshPro/Mobile/Distance Field" by name: that shader is in the always-included list |
There was a problem hiding this comment.
[P2 · R23] This comment describes project settings, not this code.
It tells the reader about GraphicsSettings → Always Included Shaders, and the need for that entry is itself still being questioned in the open GraphicsSettings.asset thread. Whatever that thread decides, the comment will drift from the setting it describes. Delete it; if the dependency matters, keep the explanation in the PR/settings discussion.
| // CreateFontAsset looks up "TextMeshPro/Mobile/Distance Field" by name: that shader is in the always-included list |
| public static void ClearCustomFont(VisualElement element) => | ||
| element.style.unityFontDefinition = new StyleFontDefinition(StyleKeyword.Null); |
There was a problem hiding this comment.
[P2 · Step 4, single-use member] public static ClearCustomFont has one consumer, ReleaseCustomFont in this same class.
If you apply the SetFont suggestion above, that adds a second in-class caller, so make it private; otherwise inline it into ReleaseCustomFont. Either way it should not be part of the public surface of UiElementUtils.
| public static void ClearCustomFont(VisualElement element) => | |
| element.style.unityFontDefinition = new StyleFontDefinition(StyleKeyword.Null); | |
| private static void ClearCustomFont(VisualElement element) => | |
| element.style.unityFontDefinition = new StyleFontDefinition(StyleKeyword.Null); |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Jarvis review: font_src on TextShape / UiText / UiInput / UiDropdown
Inspected head: 753b1d12. Since the previous bot round (4966c7b5), the only change is 753b1d1 ("add profiler markers to scene font request and receipt"), in SceneFontRequest.cs. I re-checked the full diff anyway, using three parallel review passes (correctness/lifecycle, security, performance/standards). The lint pre-flight script (scripts/lint/custom-rules.sh) is not on this branch, so I checked the review-bar rules by hand.
Step 2: Root cause
PASS. This is a new feature from decentraland/protocol#489, not a bug fix. font_src resolves to a bundled TTF (through the scene's content map) or to a Fontsource family name. External URLs are rejected. Nothing here hides a symptom.
Step 3: Design and integration
No design change since 4966c7b5. The owner search in the previous round still holds:
- Placement:
LoadFontSystem/FontsCache/FontData/FontsLoadingPluginfollow theTextures/andAudioClips/StreamableLoading pattern. They are registered inCacheCleanerandAssetsDeferredLoadingSystem. - Release: the release systems run before
ReleasePoolableComponentSystemon all three paths (component removed, entity destroyed, world finalized). - Teardown: every
FontPromise.Createis matched bySceneFontRequest.Release, and everyFontFileStore.Leaseis released inLoadFontSystem'sfinallyor inFontData.DestroyObject.
New in this round:
FontFileStore's disk lifetime is per-process, but its directory is shared across processes. References are counted in memory, but the folderpersistentDataPath/SceneFontsis shared and is wiped on every startup. Two clients started with--multi-instance, or the Editor next to a player, can delete files the other still serves to FreeType (inline, P2). This is separate from @dalkia's open thread on fonts not surviving sessions.- Profiler markers added in
753b1d1:REQUEST_MARKERcovers the resolve-and-create work correctly.RECEIVE_MARKERstarts afterpromise.TryConsume, so it measures only a field read (inline, P2).
Step 4: Member audit
- Unchanged since the last round:
SceneFontRequest.Src/.Promiseare used by tests only outside the struct;UiElementUtils.SetFonthas 4 consumers;ReleaseCustomFonthas 3;ClearCustomFonthas 1 (open thread);TMPProSdkExtensions.SetFonthas 2. - New in
753b1d1: twoprivate static readonly ProfilerMarkerfields. The interpolated names are built once at type initialisation, so the query path does not allocate.
Step 5: Findings (new in this round)
| # | Sev | Rule | Location | Summary |
|---|---|---|---|---|
| 1 | P1 | R22 | UITextInstantiationSystemShould.cs:187 (+ UiInput/UiDropdown) |
No UI test checks that a loaded font is applied; they only check that a request is created |
| 2 | P2 | R8 / lifecycle | FontFileStore.cs:33-38 |
The shared SceneFonts folder is wiped at startup and deleted per file by per-process refcounts, so concurrent clients break each other's fonts |
| 3 | P2 | R15 | SceneFontRequest.cs:15, 54-55 |
RECEIVE_MARKER scopes only a field read, so the sample always reads close to 0 ms |
| 4 | P2 | code-style (AAA) | ReleaseTextShapeSystemShould.cs:65-71 (+ the other new font test files) |
New tests don't use // Arrange / // Act / // Assert, unlike the older tests in the same files |
Still open from earlier rounds, not repeated inline. The P1s from the previous round are unchanged at this head:
font_srcis generated from the unmerged decentraland/protocol#489 (still OPEN as of this review), and the@dcl/protocolpin is not bumped.- Fontsource is reached through a hardcoded third-party URL with no media-host permission or host allowlist.
LoadFontSystemhas no tests for the Fontsource family path.
Checked and clean:
- R1/R2: no LINQ or per-frame allocation in
[Query]/Update. - R5: no structural change over a by-ref
Entity; promise entities live in other archetypes. - R10/R11: catch filters name their exception types, and cancellation is passed through.
FontDownloadHandlerlimits: no overflow.- Fontsource URLs: the family-id charset and the semver check on
npmVersionblock URL and path injection. - Untrusted JSON: parsed with
TypeNameHandling.Noneand capped at 1 MB. - Cache keys:
Kind+ content URL prevent cross-scene cache poisoning.
Merge gates
- R25: open threads. 24 review threads are unresolved at this head.
- 6 from @dalkia, none addressed since
13370a2:- move the
ReleaseReferenceComponentsSystemchange to its own PR - reuse
IDiskCacheso fonts survive sessions - create TMP/UI Toolkit assets lazily
- JSON parse on the main thread
- host allowlist for Fontsource
ttfURLs GraphicsSettings.assetalways-included shader
- move the
- 18 from earlier bot rounds.
- 6 from @dalkia, none addressed since
- R26: QA. The PR has no
no QA neededlabel, and the QA checklist item is unchecked. The test instructions are concrete and name the deployedsdk7testscenes.dcl.ethscene at 84,-6. QA must cover both Windows and Mac. - CI: the semantic-title and security-review checks pass.
- Labels:
new-dependency(runtime dependency on Fontsource/jsDelivr) andext-contribution.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Adds a new StreamableLoading pipeline (system, refcounted cache, disk store, plugin wiring) and changes ECS instantiate/update/release systems for TextShape and three SceneUI components, plus regenerated protocol code.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| Assert.That(world.IsAlive(firstPromiseEntity), Is.False); | ||
| Assert.That(uiTextComponent.FontRequest.Src, Is.EqualTo("fonts/Lobster-Regular.ttf")); | ||
| Assert.That(world.Get<GetFontIntention>(uiTextComponent.FontRequest.Promise!.Value.Entity).Src, Is.EqualTo(input.FontSrc)); | ||
| } |
There was a problem hiding this comment.
[P1 · R22] None of the UI systems has a test that applies a loaded font.
UITextInstantiationSystemShould, UIInputInstantiationSystemShould and UIDropdownInstantiationSystemShould only check that a font request is created (RequestFontWhen…, ReplaceRequestWhenFontSrcChanges). No test:
- completes the promise and asserts that
CustomFontandstyle.unityFontDefinitionend up as the UI Toolkit font, which is the visible result of the feature, or - covers the failure case, where a failed load keeps the built-in font.
InstantiateTextShapeSystem has no tests either. Only UpdateTextShapeSystemShould exercises the apply path.
Suggested test (it needs using ECS.StreamableLoading.Common.Components;, using TMPro; and using Object = UnityEngine.Object;). Add the equivalent for Input (TextField) and Dropdown:
| } | |
| } | |
| [Test] | |
| public void ApplyTheLoadedFontToTheLabel() | |
| { | |
| // Arrange | |
| sceneData.TryGetContentUrl("fonts/Roboto.ttf", out Arg.Any<URLAddress>()) | |
| .Returns(x => | |
| { | |
| x[1] = URLAddress.FromString("https://peer.decentraland.org/content/contents/bafyfont"); | |
| return true; | |
| }); | |
| TMP_FontAsset referenceFont = TestFonts.CreateTextMeshProFont(); | |
| FontFamilyAssets family = new RuntimeFontAssetFactory(referenceFont).Create("Custom", TestFonts.PATH)!; | |
| var fontData = new FontData(family); | |
| world.Add(entity, new PBUiText { FontSrc = "fonts/Roboto.ttf", IsDirty = true }); | |
| system.Update(0); | |
| world.Add(world.Get<UITextComponent>(entity).FontRequest.Promise!.Value.Entity, new StreamableLoadingResult<FontData>(fontData)); | |
| try | |
| { | |
| // Act | |
| system.Update(0); | |
| // Assert | |
| ref UITextComponent component = ref world.Get<UITextComponent>(entity); | |
| Assert.That(component.CustomFont, Is.SameAs(family.UIToolkitFont)); | |
| Assert.That(component.Label.style.unityFontDefinition.value.fontAsset, Is.SameAs(family.UIToolkitFont)); | |
| } | |
| finally | |
| { | |
| fontData.Dispose(); | |
| Object.DestroyImmediate(referenceFont); | |
| } | |
| } |
| public static FontFileStore InPersistentData() | ||
| { | ||
| var store = new FontFileStore(Path.Combine(Application.persistentDataPath, DIRECTORY_NAME)); | ||
| store.Clear(); | ||
| return store; | ||
| } |
There was a problem hiding this comment.
[P2 · R8 / lifecycle] Explorer processes running side by side can delete each other's font files.
InPersistentData() uses the fixed folder persistentDataPath/SceneFonts and calls Clear() (Directory.Delete(directory, true)) on every startup. Files are named by SHA-256, but references is counted only inside each process. Two clients can share persistentDataPath:
- two clients started with
--multi-instance(AppArgsFlags.MULTIPLE_RUNNING_INSTANCES), or - the Editor next to a built player (
ShouldForceSingleRunningInstancereturnsfalseunderUNITY_EDITOR).
Failure scenario:
- Instance A has
font_src: "Roboto"loaded. - Instance B starts and its
Clear()wipes A's file. Even without a restart: if B loads the same family it gets the same path, and when B releases it, B's count drops to 0 andFile.Deleteremoves the file A still uses. - A's next glyph that is not in the atlas makes FreeType re-open
sourceFontFilePath, which is exactly what the comment on line 14 warns about. The open fails, and A's text renders missing glyphs or falls back. This happens mostly on macOS/Linux; on Windows only while the file isn't held open.
Fix: give each process its own subdirectory, and clear only the subdirectories whose owner process has exited. This still works if the byte cache later moves to IDiskCache, as @dalkia's thread asks.
| public static FontFileStore InPersistentData() | |
| { | |
| var store = new FontFileStore(Path.Combine(Application.persistentDataPath, DIRECTORY_NAME)); | |
| store.Clear(); | |
| return store; | |
| } | |
| public static FontFileStore InPersistentData() | |
| { | |
| string root = Path.Combine(Application.persistentDataPath, DIRECTORY_NAME); | |
| ClearAbandonedSessions(root); | |
| return new FontFileStore(Path.Combine(root, System.Diagnostics.Process.GetCurrentProcess().Id.ToString())); | |
| } | |
| // Each process owns a subdirectory: clients started with --multi-instance share persistentDataPath | |
| private static void ClearAbandonedSessions(string root) | |
| { | |
| if (!Directory.Exists(root)) | |
| return; | |
| foreach (string session in Directory.GetDirectories(root)) | |
| { | |
| if (int.TryParse(Path.GetFileName(session), out int pid) && IsRunning(pid)) | |
| continue; | |
| new FontFileStore(session).Clear(); | |
| } | |
| } | |
| private static bool IsRunning(int pid) | |
| { | |
| try | |
| { | |
| using var process = System.Diagnostics.Process.GetProcessById(pid); | |
| return !process.HasExited; | |
| } | |
| catch (ArgumentException) { return false; } | |
| } |
| Promise = promise; | ||
| using ProfilerMarker.AutoScope _ = RECEIVE_MARKER.Auto(); |
There was a problem hiding this comment.
[P2 · R15] RECEIVE_MARKER measures nothing useful.
The scope starts after promise.TryConsume, which is where the receive work happens (world.TryGet plus destroying the promise entity). The cost of applying the font (SetFont/Apply) happens in the callers after this method returns. So the scope contains only a field read or TryLogException, and the SceneFontRequest.Receive sample always reads close to 0 ms, which is misleading when profiling.
Moving the marker above TryConsume would sample every pending request on every frame, so remove it. If receipt cost matters, wrap the callers' apply step instead (ApplyLoadedFont(s) in the four systems).
| Promise = promise; | |
| using ProfilerMarker.AutoScope _ = RECEIVE_MARKER.Auto(); | |
| Promise = promise; |
| private static readonly ProfilerMarker REQUEST_MARKER = new ($"{nameof(SceneFontRequest)}.Request"); | ||
| private static readonly ProfilerMarker RECEIVE_MARKER = new ($"{nameof(SceneFontRequest)}.Receive"); |
There was a problem hiding this comment.
[P2 · R15] Remove the RECEIVE_MARKER declaration together with its only use on line 55 (see the comment there).
| private static readonly ProfilerMarker REQUEST_MARKER = new ($"{nameof(SceneFontRequest)}.Request"); | |
| private static readonly ProfilerMarker RECEIVE_MARKER = new ($"{nameof(SceneFontRequest)}.Receive"); | |
| private static readonly ProfilerMarker REQUEST_MARKER = new ($"{nameof(SceneFontRequest)}.Request"); |
| world.Remove<PBTextShape>(entity); | ||
|
|
||
| system.Update(0); | ||
|
|
||
| Assert.That(world.Has<TextShapeComponent>(entity), Is.False); | ||
| textMeshProPool.Received(1).Release(textMeshPro); | ||
| AssertFontReleased(); |
There was a problem hiding this comment.
[P2 · code style (docs/code-style-guidelines.md, AAA)] The new tests don't mark // Arrange / // Act / // Assert.
The older tests in the same suites do (for example UITextInstantiationSystemShould.InstantiateUIText). Counted as [Test] count / // Act count, the new font tests have none:
ReleaseTextShapeSystemShould4/0UITextReleaseSystemShould4/0UIInputReleaseSystemShould5/0UIDropdownReleaseSystemShould5/0SceneFontRequestShould14/0FontsourceCatalogShould24/0FontSrcResolverShould17/0FontFileStoreShould17/0RuntimeFontAssetFactoryShould7/0FontDownloadHandlerShould5/0LoadFontSystemShould1/0- the new
font_srctests inUITextInstantiationSystemShould
Example for this test:
| world.Remove<PBTextShape>(entity); | |
| system.Update(0); | |
| Assert.That(world.Has<TextShapeComponent>(entity), Is.False); | |
| textMeshProPool.Received(1).Release(textMeshPro); | |
| AssertFontReleased(); | |
| // Arrange | |
| world.Remove<PBTextShape>(entity); | |
| // Act | |
| system.Update(0); | |
| // Assert | |
| Assert.That(world.Has<TextShapeComponent>(entity), Is.False); | |
| textMeshProPool.Received(1).Release(textMeshPro); | |
| AssertFontReleased(); |
|
migrated to #10317 |
4 participants
What does this PR change?
Adds
font_srcto TextShape, UiText, UiInput and UiDropdown: scenes can use bundled TTF files or a Google Fonts family name resolved through Fontsource. Unsupported or failed sources fall back to the built-in font. Arbitrary URLs and OTF files are not supported.Protocol: decentraland/protocol#489.
Test Instructions
Run
metaforge explorer run 10118and launch with:The scene is already deployed. If already on zone, use
/goto sdk7testscenes.dcl.eth/84,-6. MetaMask users: select SEPOLIA.Hold Alt to click. Check all four samples: 3D text, UI text, input and dropdown.
Run buttons 1, 2, 3, 4, 7 one at a time, watching until Finished. Skip 5–6: developer-only local tests. Finished is not an automatic pass. Judge font appearance using Latin text; Cyrillic may use fallback. Report failures with the button/step and a screenshot.
Quality Checklist
Screenshots