docs: UI defaults follow-ups (review of #76) - #77
Conversation
Review follow-ups verified against @dcl/react-ecs on the merged branch. - build-ui/SKILL: an incomplete virtual size does not warn. There is no such log in react-ecs; the only console.log is the mobile 16:9 override. Since the skill tells an agent to trust that warning, the wrong claim is worse here than in prose. - build-ui/SKILL: add the scene-wide consequence of an incomplete size -- setUiRenderer wins the arbitration if it mentions either dimension, so it disables the virtual screen for the whole scene and discards a valid size passed to addUiRenderer, silently. - ui-components: UiCanvasInformation width/height are RAW canvas pixels, not "virtual/scaled units when a virtual size is set". The SDK derives the scale factor from them, so they cannot already be scaled. This matters for the slider helper, which divides by them. - build-ui/SKILL, advanced-input/SKILL: the "~3x scaling for mobile" rule of thumb survived in both files, one of them in a paragraph this branch rewrote. With dpr out of the scale factor most of that 3x is already applied, and the 1600x720 mobile virtual screen adds ~1.2x more. - build-ui/SKILL, ui-components: 'interactable' is recommended for whole-UI application without a client floor. It needs an explorer that reports the area: mobile client 1.12.1 onwards, which is also the release that normalizes the 'device' area between Android and iOS. Older clients report no margins and the inset silently does nothing. The 7.26.0 version gate is left as written -- confirmed as the release number.
regenesis-claw
left a comment
There was a problem hiding this comment.
Review of the follow-ups on top of #76. Verified every claim against packages/@dcl/react-ecs on main at 08dff786 (the squash-merge of decentraland/js-sdk-toolchain#1489), not against the PR text.
Verified correct — these are good catches and land as written:
setUiRendererwins the arbitration whenever it mentions either dimension:hasVirtualSize()isvirtualWidth !== undefined || virtualHeight !== undefined, andgetActiveVirtualSize()returnsmainOptionsbefore ever looking at the additional renderers. SosetUiRenderer(ui, { virtualWidth: 1920 })really does disable the virtual screen scene-wide and discard a valid size passed toaddUiRenderer. This consequence was missing everywhere and is worth stating.UiCanvasInformation.width/.heightare raw canvas pixels.UiScaleSystemcomputesMath.min(width / virtualWidth, height / virtualHeight)from those exact fields, so they cannot be pre-scaled.getScaleCtx()feeds the same raw values tocalcOnViewport, which confirmsvw/vhare raw too. The old wording would have made an agent "fix" the slider helper and break the drag.- The
~3x for mobilerule needed both multipliers stated, andadvanced-input/SKILL.mdwas indeed the untouched second copy. devicePixelRatio"does not take part in UI layout" matches the code:calcOnViewportexplicitly does not readctx.ratio, andnextScalehas no ratio term.
[P1] The incomplete-size warning does exist on main — this correction inverts a true statement
The premise of the two edits below is that resolveVirtualSize() returns undefined silently for a half-given size. That was true when the analysis was written, but it stopped being true on 11 Aug: commit 2c1128ff ("applied some PR feedback", 18:09 UTC) added isPartialVirtualSize() and a once-per-size console.log, and it is in the merged 08dff786:
if (isPartialVirtualSize(provided)) {
const width = provided.virtualWidth ?? 0
const height = provided.virtualHeight ?? 0
if (loggedPartialW !== width || loggedPartialH !== height) {
loggedPartialW = width
loggedPartialH = height
console.log(
`Incomplete virtual screen size (virtualWidth: ${provided.virtualWidth}, virtualHeight: ${provided.virtualHeight}): both dimensions are required, so the virtual screen is disabled and no UI scaling is applied.`
)
}
}So there are two console.logs in @dcl/react-ecs, not one, and #76's original "and warns once to the console" is accurate as of the merge. The direction of the fix is still right — the guarantee it was worth documenting is that the <= 0 opt-out is the silent one and the half-given size is the loud one — but as written this PR would teach an agent the opposite of the shipped behavior. Suggested replacements inline on both spots.
Everything else in the diff stands. Happy to re-review as soon as the two log statements are corrected.
Requested by Lean via Slack
| | A non-16:9 size passed | used as-is on every platform | | ||
| | A size with any value `<= 0` | virtual screen **disabled** — raw canvas pixels, no scaling | | ||
| | Only one of the two dimensions passed | also **disabled** (both are required), and warns once to the console | | ||
| | Only one of the two dimensions passed | also **disabled** (both are required), silently — nothing is logged | |
There was a problem hiding this comment.
[P1] This inverts the shipped behavior. resolveVirtualSize() on merged main logs once per distinct partial size (isPartialVirtualSize() → console.log('Incomplete virtual screen size ...'), added in 2c1128ff). The <= 0 case is the silent one, since it is the documented opt-out.
| | Only one of the two dimensions passed | also **disabled** (both are required), silently — nothing is logged | | |
| | Only one of the two dimensions passed | also **disabled** (both are required), and logged once per distinct size — unlike the `<= 0` opt-out, which is silent | |
|
|
||
| The virtual size is scene-wide, resolved as: the size on `setUiRenderer` wins → else the first `addUiRenderer` that passed one → else the platform default. Options carrying only a `screenInset` don't count as a passed size. | ||
|
|
||
| Note that `setUiRenderer` wins the arbitration if it mentions *either* dimension, even when the size is incomplete and therefore invalid. So `setUiRenderer(ui, { virtualWidth: 1920 })` disables the virtual screen for the whole scene and discards a valid size passed to any `addUiRenderer` — with nothing logged. Never emit a single dimension. |
There was a problem hiding this comment.
[P1] Same correction as the table row — the trailing "with nothing logged" is not what ships. The scene-wide-override point is the valuable half and is worth keeping.
| Note that `setUiRenderer` wins the arbitration if it mentions *either* dimension, even when the size is incomplete and therefore invalid. So `setUiRenderer(ui, { virtualWidth: 1920 })` disables the virtual screen for the whole scene and discards a valid size passed to any `addUiRenderer` — with nothing logged. Never emit a single dimension. | |
| Note that `setUiRenderer` wins the arbitration if it mentions *either* dimension, even when the size is incomplete and therefore invalid. So `setUiRenderer(ui, { virtualWidth: 1920 })` disables the virtual screen for the whole scene and discards a valid size passed to any `addUiRenderer`. The incomplete size is logged to the console once, but the discarded `addUiRenderer` size is not mentioned there, so the message understates the blast radius. Never emit a single dimension. |
| **ScreenInsetArea** — Wrapper that keeps children inside the device's hardware-reserved margins (notch, status bar, home indicator, rounded corners). **Usually unnecessary now: `screenInset` defaults to `'device'`, which already does this for the whole renderer.** Reach for the component only when the renderer opted out with `screenInset: 'none'` and you want to protect just one subtree — wrapping on top of the default double-applies the inset. On mobile it positions itself absolutely using the insets the device reports; on desktop the insets are `(0,0,0,0)`, so it's a no-op. It owns its own `positionType` and `position`; any values you pass for those in `uiTransform` are ignored. All other `uiTransform` props (`padding`, `flexDirection`, `alignItems`, …) and components (`uiBackground`, `onMouseDown`, …) work as usual. A child sized `width: '100%', height: '100%'` fills the safe area exactly. Distinct from the *Decentraland system HUD* reserved zones (joystick, chat, profile, interaction button) — avoid those with `screenInset: 'interactable'` or by hand. Do **not** apply the old "scale sizes ~3× for mobile" rule of thumb on 7.26.0+: with `devicePixelRatio` out of the scale factor, pixel-sized UI is already ~2–3× larger on a phone than it used to be, and the `1600x720` mobile virtual screen adds ~1.2× on top. Start from the desktop sizes and only scale up what actually measures too small on a device. | ||
|
|
||
| **InteractableArea** — Wrapper that keeps children inside the renderer-reported *interactable area* — the part of the screen NOT covered by the client's own UI (minimap, chat window, platform overlays). Reads `UiCanvasInformation.interactableArea` and constrains children via absolute positioning; on the Unity desktop client the left ~25% of the screen is reserved, so children fill the remaining ~75%. **Prefer `screenInset: 'interactable'` on the renderer for a whole-UI application**; use the component for a single subtree, or when the renderer uses a different inset. Like `ScreenInsetArea`, it owns `positionType`/`position` (values you pass are ignored) and falls back to zero insets (no-op) when unavailable. Import from `@dcl/sdk/react-ecs`; usage `<InteractableArea><MyHud /></InteractableArea>`. Distinct from `ScreenInsetArea` (which avoids *device* hardware margins, not client UI). See `{baseDir}/references/ui-components.md` → InteractableArea. | ||
| **InteractableArea** — Wrapper that keeps children inside the renderer-reported *interactable area* — the part of the screen NOT covered by the client's own UI (minimap, chat window, platform overlays). Reads `UiCanvasInformation.interactableArea` and constrains children via absolute positioning; on the Unity desktop client the left ~25% of the screen is reserved, so children fill the remaining ~75%. **Prefer `screenInset: 'interactable'` on the renderer for a whole-UI application**; use the component for a single subtree, or when the renderer uses a different inset. Either form needs an explorer that reports the area: it works on desktop, and on mobile from client `1.12.1` onwards — older mobile clients report no margins and the inset silently does nothing. Like `ScreenInsetArea`, it owns `positionType`/`position` (values you pass are ignored) and falls back to zero insets (no-op) when unavailable. Import from `@dcl/sdk/react-ecs`; usage `<InteractableArea><MyHud /></InteractableArea>`. Distinct from `ScreenInsetArea` (which avoids *device* hardware margins, not client UI). See `{baseDir}/references/ui-components.md` → InteractableArea. |
There was a problem hiding this comment.
[P2] screenInset: 'interactable' is now recommended here with the client floor attached, which is the right fix. One thing the mobile-client version claim depends on: per the release thread, 1.12.1 is still in flight (Android EOD, iOS end of week) and the 'interactable' fix was explicitly declared droppable from it if it wasn't ready. Worth confirming the fix actually landed in 1.12.1 before this merges — if it slips, both this line and the one in references/ui-components.md name a version that doesn't report the area.
…t one My earlier correction was verified against the cherry-picked branch, not against what merged. Commit 2c1128ff added isPartialVirtualSize() and a once-per-size console.log, so main carries two logs in @dcl/react-ecs and the original wording was right for this case. The distinction is still worth stating, inverted: a value <= 0 is the documented opt-out and stays silent; a half-given size is treated as a mistake and is reported. Both disable the virtual screen either way.
Follow-ups on top of #76, verified against
@dcl/react-ecson the merged branch. Targetschore/ui-defaults-updateso it can be merged into #76 before it lands.Worth saying first: the version gate in #76 is a good call, and the slider fix — removing the
devicePixelRatiodivisor while keeping the pre-7.26.0 form documented — is the kind of thing that would have cost someone a whole afternoon. The notes below are narrow.Corrections
An incomplete virtual size does not warn. The table row says "and warns once to the console". There is no such log:
resolveVirtualSize()returnsundefinedfor an invalid size with no output, and the onlyconsole.login all of@dcl/react-ecsis the mobile 16:9 override (system.ts:194). This matters more here than in prose, because the skill is instructing an agent to trust a signal that never arrives.Also added the consequence that was missing:
setUiRendererwins the arbitration if it mentions either dimension, even when the size is invalid. SosetUiRenderer(ui, { virtualWidth: 1920 })disables the virtual screen for the whole scene and silently discards a valid size passed toaddUiRenderer.UiCanvasInformation.width/.heightare raw canvas pixels, not "virtual/scaled units when a virtual size is set". The SDK derives the scale factor from them —Math.min(width / virtualWidth, height / virtualHeight)— so they cannot already be scaled; if they were, the factor would always be 1. The line is pre-existing, but #76 edits it to add the twoBorderRectfields. It matters concretely forui-sliders.md, whose helper divides by these values: an agent that believes they arrive pre-scaled will "correct" the formula and break the drag.The "~3× scaling for mobile" rule survives in two files, one of them (
build-ui/SKILL.md) in a paragraph this branch rewrote, the other untouched inadvanced-input/SKILL.md. WithdevicePixelRatioout of the scale factor, most of that 3× is now applied automatically, and the1600x720mobile virtual screen adds ~1.2× more — so following it lands at roughly 3.6× on a phone. Both replaced with the two real multipliers and "measure on a device".'interactable'is recommended without a client floor.build-ui/SKILL.mdsays to preferscreenInset: 'interactable'for whole-UI application, andui-components.mdrepeats it. Either form needs an explorer that reports the area: mobile client1.12.1onwards, which is also the release that normalizes the'device'area between Android and iOS. Older clients report no margins, so the module value stays at zero insets and the wrapper silently covers the whole screen.Left alone
The
7.26.0version gate is unchanged — confirmed as the release number.One observation from testing on a device with
1.12.1, not applied here because it is a judgment call about how strongly to hedge: the reported interactable area excludes the client's left-hand controls but not the action buttons on the bottom right. So'interactable'is the explorer's best offer rather than a guarantee that every client control is avoided. The companion docs PR (decentraland/docs#169) carries device captures showing this, and words it that way.