Skip to content

Commit 333a216

Browse files
authored
fix(library): inset row-action icons to match status-icon padding (#831)
## Summary Follow-up to #830. The Select / Teleport / Remove buttons on the Library's Instantiated tab were rendering their icons at the full 80×80 button area, so the icon strokes ran right up to (and sometimes past) the colored bevel edges. The row's left-side status icons (which use `PE Image Simple Square`) don't have this problem — that prefab insets the icon RectTransform with `sizeDelta: -30`, leaving a 15px margin on each side. This PR applies the same `sizeDelta: -30` to the three row-action button icons after `SetIcon`, so their padding matches the status-icon column. Four lines: one comment + three identical inset assignments. ### Before vs after In an 80×80 button, the icon's effective render area goes from `80×80` (filling the button) to `50×50` (inset by 15px on each side, centered). <img width="1038" height="87" alt="{D00B40D1-E2C8-442F-B73B-9B39E1F8E0F1}" src="https://github.com/user-attachments/assets/2c11efcc-f40a-4fea-9753-60116e6fa003" /> ### Attribution Bug discovered by @Mr-Zero88 ## Required checks All boxes below must be ticked before this PR can merge. If a check is genuinely N/A, tick it anyway and explain under **Notes**. <!-- required-checks-start --> <!-- Tick the boxes in place — do not edit the line text. The pr-checklist workflow parses this block; per-PR context goes under Notes. --> - [x] **Tested** — I built and ran this locally. The change works in the editor and (where relevant) in a built player. - [x] **Transform access is combined and limited** — In hot paths, transform reads/writes go through `TransformAccessArray` or are otherwise batched. I have not added per-frame `transform.position` / `transform.rotation` / `transform.localPosition` calls inside loops. Whenever I need both position and rotation, I use the combined APIs — `SetPositionAndRotation` / `SetLocalPositionAndRotation` for writes, `GetPositionAndRotation` / `GetLocalPositionAndRotation` for reads — instead of two separate property accesses; the combined call does one local-to-world matrix traversal instead of two. - [x] **Addressables used for asset/memory loading** — Any new asset loads go through Addressables. No new `Resources.Load`, no direct asset references that pull large content into memory on scene load. - [x] **No new `GetComponent` / `AddComponent` where avoidable** — Where unavoidable, the result is cached on a field, and any `GetComponent<T>` is replaced with `TryGetComponent<T>(out var x)` — bare `GetComponent` will be denied. `TryGetComponent` is the modern API (Unity 2019.2+) and skips the Editor-only GC allocation `GetComponent` causes when a component is missing: Unity wraps the `null` return in a managed "fake null" object so its overloaded `==` operator can still detect destroyed C++ objects, and constructing that wrapper allocates; `TryGetComponent` returns a `bool` plus `out` parameter and never builds the wrapper. None of these calls run inside `Update`, `LateUpdate`, `FixedUpdate`, jobs, or other per-frame code paths. - [x] **Per-frame work is scheduled through `BasisEventDriver`** — Any new per-frame work hooks into `BasisEventDriver` rather than adding standalone `Update` / `LateUpdate` / `FixedUpdate` callbacks on a MonoBehaviour. - [x] **Considered jobification** — I asked whether this work can be moved to a Unity Job (Burst-compiled where possible). If it can, it is. If it cannot, the reason is in **Notes**. - [x] **No needless `{ get; set; }` properties or access lockdowns** — Public fields are fine; Basis is meant to be read and modified freely, so don't wall things off `private`/`internal` without a real reason. Don't wrap a field in `{ get; set; }` when the accessors do nothing — property accessors have a real performance cost vs direct field access, and the lead maintainer prefers plain fields (or a method / setter-only property when only the setter needs logic) over a noop-getter pair. For `.Instance` singletons, callers reassigning `Type.Instance` is allowed; if that would break your code, log a warning or throw — don't block the assignment. Locking down access is not your call. - [x] **Camera access goes through `BasisLocalCameraDriver`** — Code that needs the local camera (transform, projection, rig data, etc.) pulls it from `BasisLocalCameraDriver` rather than looking one up itself. Don't roll a separate camera discovery path. - [x] **Logging uses `BasisDebug`** — All new logging calls go through `BasisDebug.Log` / `BasisDebug.LogWarning` / `BasisDebug.LogError` (with an appropriate `LogTag`) instead of `UnityEngine.Debug.Log` / `Debug.LogWarning` / `Debug.LogError`. `BasisDebug` routes through Basis's tagged, color-coded logger and respects the project-wide `LoggingDisabled` toggle so logging can be killed at runtime; bare `Debug.Log` calls bypass that and will be denied. - [x] **No scene-wide discovery for dependencies** — New code is architected so it does not need `FindObjectOfType` / `FindObjectsOfType` / `GameObject.Find` / `FindGameObjectsWithTag` to locate what it depends on. References are wired in — registered through an existing manager/driver, injected at init, or passed in by the caller — rather than discovered by scanning the scene at runtime. If a scene scan is genuinely unavoidable, justify it under **Notes**. - [x] **No allocations in hot paths** — Per-frame code (Update / LateUpdate / FixedUpdate, simulation loops, jobs, anything called once per frame or more) does not allocate. No `new` on reference types, no LINQ, no `string` concatenation/interpolation, no boxing, no `foreach` over interface-typed collections. Allocate once at init and reuse the buffer. - [x] **No debugging in hot paths** — No log calls of any kind on per-frame paths, including `BasisDebug`. Hot-path logging floods the console and incurs cost on every frame regardless of whether the message is filtered out downstream. If a hot-path log is needed while iterating, gate it behind `#if UNITY_EDITOR` and remove (or leave gated) before merge. - [x] **Hot-path collection access is optimized** — Cache `.Count` (lists) / `.Length` (arrays) into a local `int` before the loop instead of re-reading the property each iteration. Prefer `T[]` (with a separate length int when the array is over-sized) over `List<T>` where the data is hot — Unity's mono BCL doesn't expose `CollectionsMarshal.AsSpan(List<T>)`, so a list can't be fed into `Span<T>` / unsafe paths cleanly. Where the perf justifies it, drop into `Span<T>` / `ref` locals / `Unsafe.As` / `unsafe` pointer code to skip bounds checks and copies, and call out the invariants you're relying on under **Notes** so reviewers can sanity-check them. <!-- required-checks-end --> ## Testing details Tick the platforms you actually tested on. Leave the rest unticked — these are informational and do not block merge. - [x] Windows - [ ] Linux - [ ] Android - [ ] iOS - [ ] macOS Input / control mode coverage: - [ ] Tested in VR (note headset under **Notes**) - [x] Tested in desktop / non-VR mode - [ ] Tested with phone controls (mobile touch input) - [ ] N/A — change does not touch player/XR/input code Where applicable, confirm these flows still work after your changes: - [ ] Hot-switching (desktop ↔ VR mode swap at runtime) - [ ] Avatar swapping - [ ] Server swapping (joining / leaving / changing servers) - [x] N/A — change does not touch any of the above ## Notes - **Required-check N/A justifications.** Trivial UI fix — three `RectTransform.sizeDelta` assignments inside `LibraryProvider.CreateListEntry`. No transform/per-frame/job/camera/logging/discovery/hot-path code touched. - **Trackerobjects follow-up.** PR #829 (`feat/trackerobjects`) has the same icon pattern on its Assign/Unbind button — a matching one-liner will land there once this merges, so the Library row stays visually consistent across all four icon buttons.
2 parents 735db87 + 0cd5f85 commit 333a216

1 file changed

Lines changed: 4 additions & 0 deletions

File tree

Basis/Packages/com.basis.framework/BasisUI/Menus/Library/LibraryProvider.cs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1955,6 +1955,8 @@ private static void CreateListEntry(BasisRuntimeSpawnRegistry.SpawnInstance item
19551955
selectItem.Descriptor.SetTitle(string.Empty);
19561956
selectItem.SetIcon(AddressableAssets.Sprites.Select);
19571957
selectItem.SetSize(new Vector2(80, 80));
1958+
// Inset the icon so its strokes stay clear of the bevel — matches PE Image Simple Square's pattern.
1959+
selectItem.Descriptor.IconImage.rectTransform.sizeDelta = new Vector2(-30, -30);
19581960

19591961
selectItem.OnClicked += async () =>
19601962
{
@@ -1979,6 +1981,7 @@ private static void CreateListEntry(BasisRuntimeSpawnRegistry.SpawnInstance item
19791981
TeleportToItem.Descriptor.SetTitle(string.Empty);
19801982
TeleportToItem.SetIcon(AddressableAssets.Sprites.TeleportTo);
19811983
TeleportToItem.SetSize(new Vector2(80, 80));
1984+
TeleportToItem.Descriptor.IconImage.rectTransform.sizeDelta = new Vector2(-30, -30);
19821985

19831986
TeleportToItem.OnClicked += () =>
19841987
{
@@ -2012,6 +2015,7 @@ private static void CreateListEntry(BasisRuntimeSpawnRegistry.SpawnInstance item
20122015
removeItem.Descriptor.SetTitle(string.Empty);
20132016
removeItem.SetIcon(AddressableAssets.Sprites.Trash);
20142017
removeItem.SetSize(new Vector2(80, 80));
2018+
removeItem.Descriptor.IconImage.rectTransform.sizeDelta = new Vector2(-30, -30);
20152019

20162020
// only apply this to items that are spawned on the network
20172021
if(itemKey.SpawnMethod == BasisRuntimeSpawnRegistry.SpawnMethod.Network)

0 commit comments

Comments
 (0)