Skip to content

Fix editor focus for scaled targets#1560

Merged
kfarr merged 1 commit into
mainfrom
fix-focus
Apr 27, 2026
Merged

Fix editor focus for scaled targets#1560
kfarr merged 1 commit into
mainfrom
fix-focus

Conversation

@vincentfretin

Copy link
Copy Markdown
Collaborator

EditorControls.focus used target.localToWorld to place the camera, which multiplied the world-space bbox-derived offset by the target's world scale. Focusing a model scaled by N ended up N times too far. Decompose matrixWorld instead and rotate the offset by the target's world rotation only. Preserves the focus-camera-pose override and the catalog baseRotation behaviour.

This is a backport of aframevr/aframe-inspector#854 but you don't seem to use scale on any entity currently, so you probably never saw that issue, but you may in the future if the user drag and drop a glb and scale the entity.

EditorControls.focus used target.localToWorld to place the camera,
which multiplied the world-space bbox-derived offset by the target's
world scale. Focusing a model scaled by N ended up N times too far.
Decompose matrixWorld instead and rotate the offset by the target's
world rotation only. Preserves the focus-camera-pose override and the
catalog baseRotation behaviour.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vincentfretin
vincentfretin requested a review from kfarr as a code owner April 18, 2026 10:24
@claude

claude Bot commented Apr 18, 2026

Copy link
Copy Markdown

Code Review: Fix editor focus for scaled targets

Overview

This PR fixes a camera focus bug in EditorControls.js where target.localToWorld() was being used to position the editor camera relative to a selected entity. Because localToWorld() applies the full world matrix (including scale), focusing on a model scaled by N would place the camera N× too far away. The fix decomposes matrixWorld to extract position and rotation independently, then applies only those (not scale) to the offset vectors.

This is a well-scoped, surgical fix with a clear rationale.


Code Quality

Positives:

  • Pre-allocating focusWorldPos, focusWorldQuat, focusWorldScale at closure scope (lines 40–42) avoids per-call GC pressure — correct Three.js idiom.
  • matrixWorld.decompose(pos, quat, scale) is the canonical Three.js way to extract components without scale contamination.
  • The explanatory comment above the decompose call clearly documents why this is done, which is exactly when a comment adds value.
  • The fix is consistent across both code paths: focus-camera-pose override and the default baseRotation fallback.

Minor observations:

  • focusWorldScale is declared and populated via decompose() but never subsequently read. This is unavoidable since decompose() requires all three output parameters, but consider a brief inline comment like // required by decompose() signature to prevent a future reader from questioning the unused variable.
  • In the poseRelativePosition path, a new THREE.Vector3 is allocated per focus call. This is fine given that focus() is an infrequent user interaction, but it's a slight inconsistency with the pre-allocated pattern used for the outer-scope vectors.

Correctness

The math is sound:

  • Before: worldMatrix * localOffset — includes scale, placing camera too far for scaled entities.
  • After: worldPos + worldQuat.rotate(localOffset) — applies only rotation and translation, no scale.

One subtle design question worth considering: for the focus-camera-pose / poseRelativePosition path, the relativePosition is authored in local space. If an entity is intentionally scaled, should the camera offset scale with it? The current fix (and the upstream aframe-inspector backport) says no — offsets are world-unit distances, which is consistent and the sensible default. If a scene author ever wants scale-aware pose offsets, that would need a separate mechanism.

The mutation of rotatedOffset via .applyQuaternion().add() is safe — the original code already mutated it via localToWorld() (which also modifies in place).


Potential Issues

  • No regression tests: Camera focus behavior is difficult to unit-test without a headless 3D environment, so the absence of tests is understandable. A brief manual test checklist in the PR description (e.g., "tested on scaled GLB, unscaled primitive, entity with focus-camera-pose") would increase confidence.
  • Impact on focus-camera-pose authored content: Any existing scenes using focus-camera-pose on non-scaled entities will behave identically (world rotation + world position is the same as localToWorld when scale = 1). No breakage expected.

Summary

This is a clean, well-reasoned backport from aframe-inspector. The fix is mathematically correct, follows Three.js idioms, and the scope is appropriately narrow. The focusWorldScale unused-variable comment is the only minor suggestion.

Verdict: Approve with minor nit. Nice work @vincentfretin!

@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in 3DStreet Dev Tracking Apr 18, 2026
@kfarr kfarr moved this from Backlog (Not Ready) to For Review in 3DStreet Dev Tracking Apr 18, 2026
@kfarr
kfarr merged commit 2450502 into main Apr 27, 2026
2 checks passed
@kfarr
kfarr deleted the fix-focus branch April 27, 2026 22:14
@github-project-automation github-project-automation Bot moved this from For Review to Done in 3DStreet Dev Tracking Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants