Skip to content

Add Builder Belt camera with Builder’s Ward - #113

Open
jneb802 wants to merge 7 commits into
mainfrom
feature/builder-belt-camera
Open

jneb802 wants to merge 7 commits into
mainfrom
feature/builder-belt-camera

Conversation

@jneb802

@jneb802 jneb802 commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Players can buy a Builder Belt from Hildir and use B to move a detached build camera while holding a build tool near a Builder's Ward. The hotkey is configurable per client through BuilderCamera.ToggleKey and is not synchronized by the server. Existing saved hotkey choices are preserved. Camera use is unlimited. The body stays vulnerable; damage, lost equipment, and ward removal end camera mode.

The belt costs 1,500 coins by default. The ward has a one-time construction cost of 10 Wood, 10 Stone, and 5 Greydwarf Eyes. Trader, price, and separate ward/body distance limits are configurable. Camera collision blocks terrain and solid structures. Building retains normal costs and protected-area checks. Gameplay settings use Jotunn admin-only synchronization.

Validation:

  • Release build passes after removing all fuel storage, refueling, timed grants, configuration, and UI. No remaining references to the removed fuel paths were found. The build has 84 existing warnings and zero errors.
  • Earlier live validation on Valnet client 01 used the full deployed Season 8 8.0.29 client mod set. It covered purchase/payment, equipment gates, normal mouse placement costs, missing resources, protected areas, range limits, wall/terrain collision, stationary body, and damage/equipment/ward/teleport exits. The original profile and world were restored afterward.
  • The unlimited-use revision has not been rerun in the live client. The retained 32-second recording and screenshots in /Users/benjmarston/Develop/PraetorisClient-builder-camera-evidence/ show the earlier fueled revision.

Dedicated-server and two-player validation remain incomplete because Valdev's selected and mounted profiles disagreed. Before production deployment, verify the unlimited-use revision, configuration synchronization, remote ward visibility, ownership transfer, reconnect behavior, repair/removal, and alternate build tools on aligned client/server profiles. Production has not been changed.

@github-actions

Copy link
Copy Markdown

Blocking issues found.


PR #113 — Add Builder Belt camera with fueled Builder's Ward

Scope: Three new files (BuilderCamera.cs, BuilderCameraContent.cs, BuilderWard.cs) and a small wire-up in Plugin.cs.


Blocking: Null reference in OnGrant when grant arrives after player disconnect

File: Source/BuilderCamera/BuilderCamera.cs, line 120

if (!_active) Player.m_localPlayer.Message(MessageHud.MessageType.Center, "Build camera active. Your body remains vulnerable.");

Player.m_localPlayer is dereferenced unconditionally. In multiplayer, Player.m_localPlayer can become null during a disconnect or loading-screen transition while a BuilderLease RPC is in flight. If ZNet.Update() (which drains the RPC queue) executes before Plugin.Update() in that frame, _pending is still true and the _requestId guard passes — reaching the Message call with a null player. The existing guard block at line 56–62 (!player → Stop(null)) only clears _pending when Plugin.Update() runs first; when ZNet.Update() wins the race it never fires.

Fix: Use the null-conditional operator:

if (!_active) Player.m_localPlayer?.Message(MessageHud.MessageType.Center, "Build camera active. Your body remains vulnerable.");

The PR description explicitly flags that dedicated-server and two-player testing is incomplete, raising the likelihood of this path being exercised in production.


Non-blocking: Comment contradicts transpiler patch for RemovePiece

File: Source/BuilderCamera/BuilderCamera.cs, lines 162–206

The comment at lines 162–163 reads:

"Change only the distance checks used by building. Normal placement, cost, station, protected-area, repair and removal checks still run on the real player."

However, RemovePiece is explicitly included in TargetMethods() alongside PieceRayTest and UpdateWearNTearHover, so the camera-relative BuildDistance is substituted into removal as well. Players in camera mode can remove pieces from the camera position rather than the body position. This may be intentional (for a useful UX), but the comment is incorrect and the behavioural discrepancy should be confirmed — if removal from camera range is unintended, RemovePiece needs to be removed from TargetMethods().


Observation: Acknowledged pre-production risks

The PR description calls out: remote ward ownership, concurrent use/refueling, ownership transfer, configuration synchronization, reconnect behaviour, repair/removal, and alternate build tools as untested. These are multiplayer correctness surfaces beyond the scope of this static review but are worth tracking against the noted validation limitation before production deployment.

@jneb802 jneb802 changed the title Add Builder Belt camera with fueled Builder’s Ward Add Builder Belt camera with Builder’s Ward Sep 29, 2026
@github-actions

Copy link
Copy Markdown

Blocking issues found.

Reviewed: BuilderCamera.cs, BuilderCameraContent.cs, BuilderWard.cs, and the Plugin.cs hook-up — covering activation flow, exit conditions, ward registration, Harmony patches, configuration binding, and the build-distance transpiler.


Important — Silent failure when initial camera position is blocked

Source/BuilderCamera/BuilderCamera.cs, lines 84–88

if (!WithinLimits(_position) || Physics.CheckSphere(_position, 0.2f, CollisionMask, QueryTriggerInteraction.Ignore))
{
    Stop("Move the camera into clear space within the ward's range.");
    return;
}

Stop() is the wrong call here. At this point in Start(), _active is still false — it is not set to true until line 91. Inside Stop():

bool wasRunning = _active;   // false
_active = false;             // no change
_ward = null;
if (wasRunning && ...)       // false → message never sent

The guard wasRunning && short-circuits, so the message "Move the camera into clear space within the ward's range." is silently dropped. The player presses F8 near a ward, nothing happens, and there is no explanation. The _ward reference is also nulled out by Stop(), though that is harmless since we are already returning.

The other two early-exit paths in Start() (lines 68–70 and 79) call player.Message(...) directly and work correctly. This one diverges from that pattern and breaks user-facing feedback for a reachable failure case (camera spawns inside geometry in caves or tight interiors).

Fix: Replace the Stop(...) call with a direct player.Message(MessageHud.MessageType.Center, "Move the camera into clear space within the ward's range.") and then return, matching the style used by the other early exits in the same method.


No other blocking issues found. The exit-condition checks (EndOnDamage, health delta, !_ward Unity null check, body-position drift guard), the CameraUpdate Prefix return-value logic, the BodyControls zero-out, and the BuildDistance transpiler all behave correctly for their intended paths. The BuilderWard.Awake ZNetView guard correctly excludes the prefab object registration. Configuration bindings are properly admin-only except ToggleKey, which is intentionally a local client setting.

@github-actions

Copy link
Copy Markdown

Blocking issues found.


Important — BuildReach transpiler replaces every Vector3.Distance call, not only the build-reach check

Source/BuilderCamera/BuilderCamera.cs lines 179–188

private static IEnumerable<CodeInstruction> Transpiler(IEnumerable<CodeInstruction> instructions)
{
    MethodInfo distance = AccessTools.Method(typeof(Vector3), nameof(Vector3.Distance));
    MethodInfo replacement = AccessTools.Method(typeof(BuilderCamera), nameof(BuildDistance));
    foreach (CodeInstruction instruction in instructions)
    {
        if (instruction.Calls(distance)) instruction.operand = replacement;
        yield return instruction;
    }
}

The transpiler patches PieceRayTest, RemovePiece, and UpdateWearNTearHover by replacing every Vector3.Distance callsite in all three methods, not just the one that enforces build reach. The comment at lines 133–134 states: "Normal placement, cost, station, protected-area, repair and removal checks still run on the real player," but the transpiler cannot enforce this — it is blind to call-site semantics.

UpdateWearNTearHover is the highest-risk target. That method processes hover state for pieces and is likely to contain more than one distance check (hover reach, snap radius, and/or repair-station proximity). Every one of those calls is rerouted to BuildDistance. During camera mode, BuildDistance returns float.MaxValue for any point that fails WithinLimits:

// BuilderCamera.cs:140-141
Vector3 target = (first - eye).sqrMagnitude < (second - eye).sqrMagnitude ? second : first;
return WithinLimits(target) ? Vector3.Distance(_position, target) : float.MaxValue;

The heuristic for identifying "which argument is the target piece" — pick the argument farther from the player's eye — holds for a single (eye, piece) call but silently misidentifies any secondary call of the form (piece1, piece2) or (player, station). For those calls both arguments may be far from the eye, so the heuristic may select the wrong one, and if that "target" sits outside WithinLimits, the call returns float.MaxValue. The concrete consequence is that hover tooltip display, snap snapping, or repair-station proximity can silently break for the duration of any camera session, even for pieces within camera range.

Fix: Instead of a blanket opcode scan, count the occurrence index of each Vector3.Distance call and only substitute the specific callsite index that corresponds to the build-reach check. Alternatively, introduce a dedicated named helper that the game already calls so the replacement is unambiguous.


Nit — BuildDistance calls Equipped(Player.m_localPlayer) without a null guard

Source/BuilderCamera/BuilderCamera.cs lines 138–139

if (!_active) return Vector3.Distance(first, second);
if (!Equipped(Player.m_localPlayer)) return float.MaxValue;

Equipped accesses player.RightItem directly (line 34) and will throw a NullReferenceException if Player.m_localPlayer is null. Update() clears _active when the player is null, but the transpiler-patched methods are invoked by Valheim's own Player.Update path, which may run in the same frame before or after this mod's Update() depending on Harmony execution order. Adding if (Player.m_localPlayer == null) return float.MaxValue; before the Equipped call closes the window.


Reviewed areas

  • Camera lifecycle (Start, Stop, Update, MoveCamera) — guard conditions are complete and the exit paths (damage, equipment loss, body drift, ward disappearance, UI block) are all wired up correctly.
  • BodyControls Prefix — correctly zeros movement and input refs before SetControls processes them.
  • CameraUpdate Prefix — the return !_active after MoveCamera correctly re-enables the base method if a collision stop fires mid-frame.
  • BuilderWard registration and Instances management — ghost-ZDO filtering in Awake is idiomatic and correct.
  • Trader postfix — duplicate-listing guard and price read are correct; IsAdminOnly synchronization covers the config value.
  • RegisterContent — disabling and destroying the PrivateArea before removing it avoids null-reference issues on the marker/effect children.
  • PrivateArea.CheckAccess usage in Start/Update — operates against the vanilla ward covering the builder ward location; behaviour is intentional and consistent.

@github-actions

Copy link
Copy Markdown

No blocking issues found.

BuilderCamera.cs

CameraUpdate.Prefix (BuilderCamera.cs:156–162): When MoveCamera calls Stop() mid-frame (e.g. blocked collision), _active flips to false and the prefix returns true, letting the original UpdateCamera run immediately in the same frame. The transition is seamless—no stale camera transform is left visible.

BuildReach transpiler (BuilderCamera.cs:179–208): The "exactly one Vector3.Distance call" invariant throws InvalidOperationException at patch-application time if the game is ever updated to add a second distance call to any of the three patched methods. This is a deliberate fail-fast design choice (as the comment documents) and correctly prevents silently patching the wrong call. It is not a silent runtime regression.

EndOnDamage prefix (BuilderCamera.cs:238–242): hit.GetTotalDamage() is read before armor/resistance mitigation runs inside ApplyDamage, so hits that would reduce to zero effective damage can still stop the camera. Given the stated intent ("damage ends camera mode"), this is a conservative safety choice rather than a bug.

Update() health tracking (BuilderCamera.cs:47–70): _health is updated each frame after the exit checks pass, so health increases (food) raise the baseline correctly. The player.GetHealth() < _health stop condition and the EndOnDamage patch are complementary—EndOnDamage catches damage applied through ApplyDamage; the frame-to-frame comparison in Update() catches any health decrease that bypasses the patch (e.g. another mod writing health directly). Both guard the same invariant without conflict.

WithinLimits body-distance leg (BuilderCamera.cs:111–113): uses Player.m_localPlayer.transform.position (live body position) rather than _bodyPosition. Since Update() stops the camera if the body drifts more than 0.5 m, the live position and _bodyPosition are always close while the camera is active; the range limit therefore behaves as intended.

BuilderCameraContent.cs

PrivateArea removal (BuilderCameraContent.cs:322–325): GetComponent<PrivateArea>() is not null-guarded before accessing area.m_areaMarker. guard_stone always carries PrivateArea in the current vanilla build, so this is safe now; worth a guard if the base prefab ever changes.

RegisterContent is wired to PrefabManager.OnVanillaPrefabsAvailable and unsubscribes itself after the first call (BuilderCameraContent.cs:331), so it fires exactly once. Harmony patches are applied by the assembly-wide AccessTools.GetTypesFromAssembly loop in Plugin.ApplyHarmonyPatches (Plugin.cs:179–190), which includes the BuilderCameraFeature namespace and all nested patch classes.

BuilderWard.cs

Awake correctly gates Instances.Add on view.IsValid() (BuilderWard.cs:370–371), excluding placement ghosts that temporarily instantiate the prefab without a valid ZDO. OnDestroy removes the entry unconditionally, so the list stays accurate over the piece lifetime.

Plugin.cs

BuilderCamera.Initialize is called before ApplyHarmonyPatches, which is correct: config entries are bound and the PrefabManager.OnVanillaPrefabsAvailable subscription is registered before patches are applied; the event fires later during game loading when all patches are already in place. ToggleKey is correctly bound with IsAdminOnly = false so it stays local to each client and is not overwritten by server sync.

Not yet validated (per PR description): the unlimited-use revision has not been re-run on the live client; dedicated-server and two-player paths remain untested on aligned profiles. These are stated pre-production risks, not bugs in the diffed code.

@github-actions

Copy link
Copy Markdown

No blocking issues found.

Reviewed BuilderCamera.cs, BuilderCameraContent.cs, BuilderWard.cs, and the Plugin.cs wiring.


EndOnDamage exits on pre-mitigation raw damage (behavior inconsistency)
Source/BuilderCamera/BuilderCamera.cs lines 241–248

hit.GetTotalDamage() is the sum of raw damage types before armor reduction. A player whose armor fully absorbs a hit (effective damage = 0) still triggers Stop("Build camera ended: you took damage.") even though _health does not change. The Update() health-drop check on line 53 (player.GetHealth() < _health) would never have fired in that case. The message is factually incorrect and the exit is spurious for heavily-armored players. The EndOnDamage prefix is redundant given the per-frame health check; consider removing it and relying solely on the health delta, or changing the check to fire after armor resolution (a Postfix that compares health before/after).


Transpiler fail-loud assumption (noted, not blocking)
Source/BuilderCamera/BuilderCamera.cs lines 197–212

The BuildReach transpiler throws InvalidOperationException if any of the three targeted methods has zero or more than one Vector3.Distance call. If the throw propagates through Harmony, the patch for that method silently fails and remote-position building reach is lost without user-visible feedback. The PR description confirms the transpiler was validated in the fueled revision (same logic), and this revision doesn't change the transpiler. No action needed now, but the assumption should be re-verified against the current game IL before deploying to a new Valheim version.


Other areas reviewed with no findings:

  • Stop(null) path when Player.m_localPlayer is null: safe (null-checked inside Stop).
  • CameraUpdate.Prefix returning !_active after MoveCamera calls Stop: correctly lets original camera run in the same frame.
  • BodyControls movement lock and the _bodyPosition > 0.5 m exit: consistent with each other.
  • BuildDistance/RemoveDistance stack contract replacing Vector3.Distance(Vector3, Vector3) with an identical signature: correct.
  • BuilderWard.Instances lifecycle (Awake/OnDestroy): correct registration gating on ZNetView.IsValid().
  • RegisterContent destroying PrivateArea, ItemStand, and EffectArea before adding BuilderWard: no reference to destroyed components after the call.
  • TraderItems postfix null guards on ObjectDB.instance and existing-item dedup: correct.
  • Shutdown() calling Stop(null) and unsubscribing RegisterContent: consistent with other feature teardowns in OnDestroy.

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