Skip to content

Move Server Guide to upper navigation panel - #141

Merged
jneb802 merged 4 commits into
mainfrom
fix/server-guide-crafting-overlap
Oct 15, 2026
Merged

jneb802 merged 4 commits into
mainfrom
fix/server-guide-crafting-overlap

Conversation

@jneb802

@jneb802 jneb802 commented Oct 15, 2026 •

Copy link
Copy Markdown
Owner

Move the Server Guide button from the crafting controls to the left edge of the upper screen-navigation panel. Keep Repair’s native frame and backing artwork, size the backing to a square, and parent the button to the upper panel so it follows that panel’s layout. Remove the per-frame positioning and Dverger Automation dependency.

Validation: Release build passed with 84 existing warnings and no errors. Tested on Valnet client 01 at 1920×1080 with the deployed Season 8 8.0.44 client mod set, My Little UI 1.2.31, and Dverger Automation 0.12.3. Checked the workbench and normal inventory views. The guide stays on the upper panel, all crafting filters remain clear, the armor filter changes recipes, and clicking the guide opens the production guide with 48 pages.

No guide-related exceptions occurred. The log retains existing shader/mod warnings and Extra Slots inventory-backup warnings. Logging out to reload the production guide produced eight PieceManager.BuildPiece.SnapshotPiece null-reference errors in ZDOMan.AddIfPortal; these occurred during main-menu return, outside the guide click flow. Other resolutions and custom layouts were not tested.

Evidence: /Users/benjmarston/Develop/valheim-validation-evidence/guide-nav-20261015/ contains workbench-nav.png, inventory-nav.png, filter.png, guide-open.png, and candidate-bepinex.log. Original overlap evidence remains in guide-compat-20261015. This PR does not publish or deploy a release.

@github-actions

Copy link
Copy Markdown

No blocking issues found.

PR #141 — Fix Server Guide overlap with My Little UI crafting filters

The change moves button positioning from a one-shot anchoredPosition assignment in Create() to a LateUpdate-driven PositionButton() that tracks the Dverger Automation button dynamically. The implementation looks correct for its stated purpose.

What I verified:

  • Null guards are correct. LateUpdate() short-circuits on _buttonRect != null && _buttonRect.gameObject.activeInHierarchy, and PositionButton() independently guards on _repairRect == null. Unity's overloaded == for UnityEngine.Object handles C# null and destroyed-object null correctly, so the null! field initializers are safe.

  • Anchor/pivot propagation is sound. Copying anchorMin, anchorMax, and pivot from the chosen anchor on each frame keeps the guide button's layout frame consistent with its reference, which is necessary for anchoredPosition + Vector2.up * height to produce a pixel-correct offset regardless of whether the anchor is the repair button or DA's button.

  • Fallback chain is correct. _repairRect.parent.Find("DA_CraftFromStorageToggle") as RectTransform ?? _repairRect falls back silently if DA is absent, if its button isn't a direct sibling (non-recursive Find), or if the found object lacks a RectTransform component.

  • DA button disappearance is handled. If DA is unloaded mid-session and its button is destroyed, Unity's Find returns null next frame, and the guide button silently relocates above repair — the right fallback.

  • Direction change is intentional and correct. The old Vector2.down * (repairRect.rect.height + 12f) (below repair) is replaced with Vector2.up * (anchor.rect.height + 12f) (above anchor). anchor.rect.height reads the computed layout height, which is valid for pre-existing buttons and will be corrected by subsequent LateUpdate calls for any first-frame layout edge cases.

Minor observations (non-blocking):

  • Transform.Find() is called every frame in LateUpdate. It's a string-keyed linear scan of siblings. Given the typical small sibling count on an inventory panel, this won't be noticeable, but it's worth knowing if profiling ever flags this area.
  • If _buttonRect.gameObject is never deactivated independently, the activeInHierarchy guard in LateUpdate is redundant (since GuideWindow lives on inventory.gameObject — a deactivated inventory also silences the component). Not a bug; just dead guard path in the no-DA scenario.

@github-actions

Copy link
Copy Markdown

Now I have the full picture. Let me write the review.


No blocking issues found.

Reviewed: Source/ServerGuide/GuideWindow.cs — button-repositioning logic, null handling, LateUpdate lifecycle, and fallback behavior when Dverger Automation is absent.

The approach is sound. GuideWindow lives on inventory.gameObject, so its LateUpdate runs throughout the inventory session. The split into PositionButton() lets Create() do an initial placement and LateUpdate track the DA toggle if it appears later. Unity's overloaded == operator means the _buttonRect != null guard in LateUpdate and the _repairRect == null guard in PositionButton both handle destroyed-object cases correctly. The ?? _repairRect fallback is the right way to degrade gracefully when DA_CraftFromStorageToggle is not present.

Nit — PositionButton (line 97): Transform.Find is called every LateUpdate frame. _repairRect.parent.Find("DA_CraftFromStorageToggle") does a linear scan of all siblings on every frame while the inventory is open. The Valheim crafting panel has very few children, so this won't matter in practice, but a cached reference (invalidated when the result changes between null and non-null) would eliminate the churn entirely.

Nit — PositionButton (line 97): no guard on _repairRect.parent. The null check is if (_repairRect == null) return, but the very next line dereferences _repairRect.parent without checking whether the repair button has been detached from its parent. In Valheim's UI this never happens, but a defensive if (_repairRect == null || _repairRect.parent == null) return would make the intent explicit.

@github-actions

Copy link
Copy Markdown

No blocking issues found.

Summary of changes reviewed: GuideWindow.cs gains four new fields (_buttonRect, _repairRect, _backdropRect, _repairPanelRect), a cloned backing panel (_backdropRect), a LateUpdate() repositioning loop, and a PositionButton() helper. The original anchoredPosition assignment in Create() is replaced by the helper.

Things that are correct:

  • LateUpdate() guard (_buttonRect.gameObject.activeInHierarchy) is the right way to detect that Dverger Automation has finished placing its button on a later frame, and the condition prevents unnecessary work when the panel is inactive.
  • raycastTarget = false on all cloned Graphics correctly prevents the backdrop from intercepting pointer events; Button/Selectable interaction in Unity is gated on the graphic raycaster hit-test, so cloned interactive children are effectively non-functional.
  • Backdrop sibling index (SetSiblingIndex(repairPanel.GetSiblingIndex())) inserts the backdrop directly before the repair panel, so it renders behind the buttons added later — correct z-order assuming the button and panel share a parent.
  • Spacing math with DA present (anchor.anchoredPosition.y − _repairRect.anchoredPosition.y) mirrors the existing DA-to-Repair gap upward, producing the equal-spaced column the PR describes.
  • Fallback without DA (_repairPanelRect.rect.height) replaces the old button.rect.height + 12f; the panel height as spacing is consistent with deriving position from the panel geometry rather than the button geometry, matching the "same panel style and spacing" goal.
  • OnDestroy() omission of _backdropRect: GuideWindow is a component of inventory.gameObject; _backdropRect is a child of the same inventory hierarchy. Unity destroys the entire hierarchy when the game object is destroyed, so no leak occurs in normal lifecycle. The Destroy(Reader.gameObject) pattern is needed there because Reader lives on a separate instantiated game object; _backdropRect does not have that problem.
  • PositionButton() writing anchorMin/anchorMax/pivot every frame: these properties are copied from the anchor transform, which in this UI column is expected to use the same anchor settings as the repair button. No layout instability expected.

Minor observations (non-blocking):

  • If repairRect.parent and _repairPanelRect.parent differ (e.g. Valheim separates button and panel containers), the backdrop's sibling-index-based z-order relative to the guide button would depend on parent ordering rather than local index. The validation evidence at 1920×1080 covers the common case; other UI layouts are noted as untested in the PR description.
  • PositionButton() does not null-guard _backdropRect (only _repairRect is guarded). Under normal lifecycle this is fine, but an external mod destroying the backdrop object while the guide button is still active would produce an NRE in LateUpdate. Low risk given the object's private ownership.

@jneb802 jneb802 changed the title Fix Server Guide overlap with My Little UI crafting filters Move Server Guide to upper navigation panel Oct 15, 2026
@github-actions

Copy link
Copy Markdown

No blocking issues found.

PR #141 — Move Server Guide to upper navigation panel

The diff replaces per-frame relative positioning with a cloned-backing panel parented to m_infoPanel. The core behaviour is sound: the backdrop clones the repair-panel artwork and artwork-only for visual fidelity, the ServerGuideIcon button is correctly centred inside it, and the onClick wiring and tooltip are unchanged.

One non-blocking observation worth awareness:

Cloned repair-button components remain active in the backdrop (Source/ServerGuide/GuideWindow.cs:48-49)

foreach (Graphic graphic in backdrop.GetComponentsInChildren<Graphic>(true))
    graphic.raycastTarget = false;

m_repairPanel is cloned wholesale as ServerGuideBackdrop, so the clone contains any Button children from the original panel (including the repair button's Button component and its onClick listeners). Setting raycastTarget = false on all Graphic children prevents mouse clicks from reaching those cloned buttons, which is correct. However, Button is a Selectable, not a Graphic, so its navigation settings and event system registration are unaffected by this loop. If Valheim's gamepad or keyboard navigation traverses into m_infoPanel's children, the invisible cloned repair button could receive focus and trigger a repair action when confirmed. The new guide button itself correctly sets Navigation.Mode.None (line 83), but the cloned contents of the backdrop do not.

This is low-severity because Valheim's inventory gamepad navigation typically follows explicit explicit or None navigation chains anchored to known UI roots, and the PR body reports no gamepad issues in testing. Mitigation, if desired, would be to call Destroy or DestroyImmediate on any Button or Selectable components found in backdrop after Instantiate, or to set their interactable = false.

No other behavioural issues were found. The anchor/pivot/position combination (anchorMin = anchorMax = (0, 0.5), pivot = (1, 0.5), anchoredPosition = Vector2.zero) intentionally places the backdrop's right edge flush with the left boundary of m_infoPanel, matching the stated design intent and confirmed by the author's test evidence.

@jneb802
jneb802 merged commit 5d8ba45 into main Oct 15, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant