feat(GlobeViewOp): add pitch, bearing, resolution, and touch rotation#415
feat(GlobeViewOp): add pitch, bearing, resolution, and touch rotation#415charlieforward9 wants to merge 1 commit into
Conversation
… support
GlobeViewOp was missing fields that GlobeView supports — pitch, bearing,
resolution, and controller options. This meant nib definitions passing
these values had them silently dropped, and there was no way to enable
touch rotation for mobile devices.
- Add resolution NumberField (mesh detail for flat-to-3D conversion)
- Add controller BooleanField (enables/disables interaction)
- Add bearing and pitch to viewState CompoundPropsField
- Convert controller:true to { touchRotate: true } in execute(),
enabling two-finger rotation for pitch/bearing on mobile
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR adds
Confidence Score: 4/5Mergeable with caution — the structural changes are sound but the advertised mobile rotation feature won't work as deck.gl's GlobeController explicitly doesn't support rotation. One P1 finding: touchRotate: true is a no-op per deck.gl docs, so the PR's primary stated feature won't function. The added fields and refactoring are otherwise correct. Score is 4 rather than 3 because the changes don't break existing behavior — they just add non-functional fields. noodles-editor/src/noodles/operators.ts — GlobeViewOp.execute and createInputs Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[GlobeViewOp.execute] --> B{controller input}
B -- true --> C["{ touchRotate: true }"]
B -- false --> D["false (no interaction)"]
C --> E["new GlobeView({ id, ...viewProps, controller })"]
D --> E
E --> F[GlobeController]
F --> G["dragPan ✅ pan"]
F --> H["scrollZoom ✅ zoom"]
F --> I["touchRotate ⚠️ not effective per deck.gl docs"]
F --> J["dragRotate ⚠️ not effective per deck.gl docs"]
E --> K["viewState: { lat, lng, zoom, bearing?, pitch? }"]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: noodles-editor/src/noodles/operators.ts
Line: 3726
Comment:
**`touchRotate` is documented as a no-op for GlobeController**
The deck.gl docs for `GlobeController` explicitly state:
> `touchRotate`: not effective, this view does not currently support rotation
> `dragRotate`: not effective, this view does not currently support rotation
Passing `{ touchRotate: true }` to the controller has no effect — the globe does not support pitch/bearing rotation at the controller level. The `bearing` and `pitch` viewState fields may set an initial orientation if deck.gl accepts them, but the two-finger-rotate gesture described in the PR test plan won't function. Worth verifying against the actual deck.gl version in use before adding these as advertised interactive features.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: noodles-editor/src/noodles/operators.ts
Line: 3702
Comment:
**`resolution` scale is inverted relative to the field label**
Per deck.gl's docs, `resolution` is in *degrees*, and *smaller* numbers produce a more detailed mesh (default `10`). With `max: 90` the slider's high end yields only ~4 triangles across the globe — nearly invisible tessellation artifacts. A tighter upper bound (e.g. `max: 32`) and a `showByDefault: false` or tooltip noting "lower = finer mesh" would prevent accidental performance or quality surprises.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(GlobeViewOp): add pitch, bearing, r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| view: new GlobeView({ | ||
| id: this.id, | ||
| ...viewProps, | ||
| controller: controller ? { touchRotate: true } : false, |
There was a problem hiding this comment.
touchRotate is documented as a no-op for GlobeController
The deck.gl docs for GlobeController explicitly state:
touchRotate: not effective, this view does not currently support rotation
dragRotate: not effective, this view does not currently support rotation
Passing { touchRotate: true } to the controller has no effect — the globe does not support pitch/bearing rotation at the controller level. The bearing and pitch viewState fields may set an initial orientation if deck.gl accepts them, but the two-finger-rotate gesture described in the PR test plan won't function. Worth verifying against the actual deck.gl version in use before adding these as advertised interactive features.
Prompt To Fix With AI
This is a comment left during a code review.
Path: noodles-editor/src/noodles/operators.ts
Line: 3726
Comment:
**`touchRotate` is documented as a no-op for GlobeController**
The deck.gl docs for `GlobeController` explicitly state:
> `touchRotate`: not effective, this view does not currently support rotation
> `dragRotate`: not effective, this view does not currently support rotation
Passing `{ touchRotate: true }` to the controller has no effect — the globe does not support pitch/bearing rotation at the controller level. The `bearing` and `pitch` viewState fields may set an initial orientation if deck.gl accepts them, but the two-finger-rotate gesture described in the PR test plan won't function. Worth verifying against the actual deck.gl version in use before adding these as advertised interactive features.
How can I resolve this? If you propose a fix, please make it concise.| return { | ||
| ...createBaseViewFields(), | ||
| ...createGeoViewFields(), | ||
| resolution: new NumberField(10, { min: 1, max: 90, step: 1 }), |
There was a problem hiding this comment.
resolution scale is inverted relative to the field label
Per deck.gl's docs, resolution is in degrees, and smaller numbers produce a more detailed mesh (default 10). With max: 90 the slider's high end yields only ~4 triangles across the globe — nearly invisible tessellation artifacts. A tighter upper bound (e.g. max: 32) and a showByDefault: false or tooltip noting "lower = finer mesh" would prevent accidental performance or quality surprises.
Prompt To Fix With AI
This is a comment left during a code review.
Path: noodles-editor/src/noodles/operators.ts
Line: 3702
Comment:
**`resolution` scale is inverted relative to the field label**
Per deck.gl's docs, `resolution` is in *degrees*, and *smaller* numbers produce a more detailed mesh (default `10`). With `max: 90` the slider's high end yields only ~4 triangles across the globe — nearly invisible tessellation artifacts. A tighter upper bound (e.g. `max: 32`) and a `showByDefault: false` or tooltip noting "lower = finer mesh" would prevent accidental performance or quality surprises.
How can I resolve this? If you propose a fix, please make it concise.
|
Seems reasonable. @charlieforward9 what's the state of this pull? Anything holding back merge? |
|
visgl/deck.gl#10207 which Ib said is in @Pessimistress territory |
2 participants
Summary
resolution,controller,bearing, andpitchfields toGlobeViewOp— supported by deck.gl's GlobeView but missing from the operator, so values were silently droppedcontrolleris true, pass{ touchRotate: true }to GlobeView, enabling two-finger rotation for pitch/bearing on mobileDependencies
Context
deck.gl's GlobeView supports pitch and bearing (via visgl/deck.gl#10207), but the noodles GlobeViewOp only exposed latitude, longitude, and zoom in its viewState. This meant there was no gesture path to change pitch or bearing on touch devices —
touchRotatedefaults tofalsein deck.gl's base Controller.Changes
resolutionNumberField (default 10, controls mesh detail for flat-to-3D conversion)controllerBooleanField (default true, enables/disables interaction)bearingNumberField in viewState (optional)pitchNumberField in viewState (0-60, optional, matching GlobeView maxPitch)execute()convertscontroller: trueto{ touchRotate: true }so mobile two-finger rotation works out of the boxTest plan
controller: falsedisables all interaction