Skip to content

Add common Three.js primitive geometries, polar_grid scene parameter, and rotate(order=...) - #5989

Open
Jepson2k wants to merge 19 commits into
zauberzeug:mainfrom
Jepson2k:scene-primitives-rotate
Open

Jepson2k wants to merge 19 commits into
zauberzeug:mainfrom
Jepson2k:scene-primitives-rotate

Conversation

@Jepson2k

@Jepson2k Jepson2k commented Apr 24, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

ui.scene is missing several commonly-used Three.js primitives (Plane, Cone, Torus, Capsule), a Line-with-vertex-colors-and-dashing primitive (Polyline), and a surface-of-revolution primitive (Lathe). Many 3D visualizations also benefit from a circular polar floor instead of a rectangular grid.

This PR fills those gaps and generalizes Object3D.rotate(...) to support all six intrinsic Euler orders rather than only the implicit 'XYZ'.

pr-5989

Implementation

  • New geometry primitives: Polyline (with optional per-vertex colors and dashed material), Lathe, Plane, Cone (with open_ended / theta_start / theta_length), Torus (with optional partial arc), Capsule. Each is a module in objects/ like the existing primitives: Plane, Cone, Torus, Capsule and Lathe return their Three.js geometry from create_geometry, and Polyline builds its own Line in create_mesh so it can carry per-vertex colors and dashing.
  • Polar grid: Scene(polar_grid=(radius, sectors, rings)) or (radius, sectors, rings, divisions), where divisions sets how smooth each ring is (default 64). Replaces the rectangular grid with PolarGridHelper. Mutually exclusive with grid (polar takes precedence).
  • Euler rotation order: Object3D.rotate(..., order='ZYX') accepts any of {'XYZ', 'XZY', 'YXZ', 'YZX', 'ZXY', 'ZYX'}. Default 'XYZ' is bit-for-bit unchanged. Matrix is composed Python-side so the rotation survives a re-send to the client.

Progress

  • The PR title is a short phrase starting with a verb like "Add ...", "Fix ...", "Update ...", "Remove ...", etc.
  • The implementation is complete.
  • This PR does not address a security issue.
  • Pytests have been added.
  • Documentation has been added.
  • No breaking changes to the public API.

Jepson2k and others added 2 commits April 24, 2026 09:05
…r=...)

Four new `scene_objects` primitives wrapping the corresponding Three.js
objects, all available via the chainable `scene.<name>()` factory:

- `Polyline` — connects a sequence of 3D points with optional per-vertex
  colors and GPU-dashed `LineDashedMaterial` (defaults `dash_size=3`,
  `gap_size=1`, matching `LineDashedMaterial`'s own defaults).
- `Lathe` — surface of revolution generated by spinning a 2D profile
  around the y axis (wireframe-capable like other geometry primitives).
- `ArrowHelper` — wraps Three.js' `ArrowHelper` with optional radial
  segments for a smoother cone head and a `line_width` hint
  (documented as commonly clamped to 1 by WebGL).
- `PolarGridHelper` — circular reference grid in the XZ plane.

`Object3D.rotate(...)` gains an optional intrinsic Euler `order` kwarg
matching `THREE.Euler(rx, ry, rz, order)` — one of `'XYZ'`, `'XZY'`,
`'YXZ'`, `'YZX'`, `'ZXY'`, `'ZYX'` (default `'XYZ'`, preserving the
previous behavior). The rotation matrix is composed Python-side so that
the rotation is stored in `self.R` and survives a re-send to the client
(reconnect, page revisit, etc.) like every other `rotate_R` call.
`rotation_matrix_from_euler` accepts the same `order` argument and
delegates to a small generic `_matmul3` helper.

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…e, add polar_grid scene parameter

Reshape the slice into a coherent "missing common geometry primitives + polar
grid floor" PR:

- Drop ArrowHelper and PolarGridHelper. They were buggy (ArrowHelper passed
  Python None through to Three.js as null, defeating the constructor's
  `=== undefined` default-handling and producing invisible / mis-sized arrow
  heads) and they are helpers, not geometries -- belong in a separate slice
  alongside Box3Helper / PlaneHelper / light helpers.
- Add Plane, Cone, Torus, Capsule wrapping the corresponding Three.js
  geometries via the existing generic dispatch in scene.js.
- Add a `polar_grid: tuple[float, int, int] | None` scene constructor kwarg
  that replaces the rectangular floor with a circular ground + PolarGridHelper
  (mutually exclusive with `grid`; polar takes precedence).
- Validate `len(colors) == len(points)` in Polyline at the API boundary
  instead of letting Three.js silently render extra points black.
- Move EULER_ORDERS to the top of Object3D alongside other class-level state.
- Drop a redundant `import pytest as _pytest` shim in the test file.
- Add a parametrized smoke test covering Plane / Cone / Torus / Capsule that
  asserts each dispatches to the expected Three.js geometry class -- guards
  against silent removals or renames in future Three.js upgrades.
- Add a `test_polar_grid` integration test for the scene kwarg.
- Rework the docs demo to showcase the geometry primitives and add a
  standalone Polar Grid demo.
@Jepson2k Jepson2k changed the title Add Polyline / Lathe / ArrowHelper / PolarGridHelper plus rotate(order=...) Add common Three.js primitive geometries, polar_grid scene parameter, and rotate(order=...) Apr 25, 2026
Inserting polar_grid between `grid` (3rd positional) and `camera` (was 4th)
silently shifted every later parameter by one slot, so any caller passing
`camera` (or anything after it) positionally would have started binding a
`tuple[float, int, int] | None` to a `SceneCamera | None` arg and crashed
later in JS.

The established convention from recent param additions (`control_type`,
`fps`, `show_stats`) is to append to the end of the signature, leaving the
deprecated positional zone untouched until the NiceGUI 4.0 keyword-only
enforcement promised by the inline DEPRECATED comment lands. The :param
docstring entry moves to match.
…hness, polyline guard

- Replace incorrect THREE.Euler claim on rotation_matrix_from_euler / rotate
  with an accurate description (leftmost letter rotates first about world frame)
  and pin the per-order expected matrix as an explicit dict in the test.
- Drop the dead add_rename('polar_grid', 'polar-grid') line; the kwarg never
  had a pre-rename history.
- Expose the PolarGridHelper smoothness as an optional 4th tuple element
  (radius, sectors, rings, divisions) defaulting to 64; cover the new path
  with a parametrized test that pins the helper vertex count.
- Reject Polyline with fewer than 2 points at the API boundary.
We confirmed during development that Object3D.rotate(...) does not match
THREE.Euler(rx, ry, rz, order) semantics. The demo's docstring kept
saying it did, which would mislead readers. Just describe the kwarg.
The previous demos showed primitives on a square grid and polar grid
with three plain spheres separately. Combining them into a single demo
gives readers all the new geometry types arranged around a circular
floor with distinct colors — a richer visual that fits both features
in one frame, and removes the redundancy of the separate Polar Grid
demo.
@falkoschindler
falkoschindler self-requested a review May 5, 2026 12:57
@falkoschindler falkoschindler added this to the Next milestone May 5, 2026
@falkoschindler falkoschindler added feature Type/scope: New or intentionally changed behavior review Status: PR is open and needs review labels May 5, 2026
… loads

The previous STL handling computed `EdgesGeometry` synchronously against an
empty `BufferGeometry` placeholder, then assigned the loaded geometry onto
`mesh.geometry` from the loader callback. This crashed in
`BufferGeometryUtils.mergeVertices` with "Cannot read properties of undefined
(reading 'count')" and never produced wireframe edges for the real geometry.

Move STL out of the geometric-primitives branch into its own block that
mirrors the GLTF flow: create a `THREE.Group` placeholder marked
`userData.isStl`, then build the `LineSegments` (wireframe) or `Mesh` child
inside the loader callback once the actual geometry is available. Replay any
`material()` call queued via `pendingMaterialInfo` once loaded, matching the
GLTF deferral.

Extend `material()` to treat `isStl` like `isGltf` — defer until loaded, and
traverse both `isMesh` and `isLine` children when applying material props.
@falkoschindler falkoschindler added in progress Status: Someone is working on it and removed review Status: PR is open and needs review labels May 5, 2026
@Jepson2k
Jepson2k marked this pull request as ready for review May 6, 2026 15:18
Jepson2k added a commit to Jepson2k/nicegui that referenced this pull request May 6, 2026
Stacks on zauberzeug#5989 (scene primitives + STL Group restructure). The STL
clipping-plane deferral relies on 5989's STL Group + userData.loaded
structure: when set_clipping_planes is called before the STL geometry
loads, the planes are stashed on userData.pendingClippingPlanes and
flushed from the loader callback (mirroring pendingMaterialInfo).

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
@falkoschindler falkoschindler added review Status: PR is open and needs review and removed in progress Status: Someone is working on it labels May 27, 2026
@Jepson2k Jepson2k mentioned this pull request Jun 4, 2026
6 tasks done
@falkoschindler falkoschindler self-assigned this Jun 30, 2026
@evnchn

evnchn commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks @Jepson2k — really nice work here. The feature code is correct (I re-derived the rotation math against numpy for all 6 orders + legacy 3-arg, and checked every new primitive's args against its Three.js constructor), the tests are a cut above, and CI's green. Two notes:

1. The STL-loader rework fixes a real crash — let's get it out ahead of the rest

Nice catch reworking the STL path (empty-BufferGeometry → deferred-material THREE.Group, mirroring the gltf path). I built a minimal repro to understand it, and it's better than it looks: it fixes a hard crash on main, not just a cosmetic glitch.

Minimal repro (12-triangle cube STL, wireframe=True):

from nicegui import app, ui

@ui.page('/')
def index():
    app.add_static_file(local_file='cube.stl', url_path='/cube.stl')
    with ui.scene() as scene:
        scene.stl('/cube.stl', wireframe=True)

ui.run()
main (3.13.0.post24.dev0+c40f3e3f) this PR
object registered ❌ objects.get(id) → undefined ✅ a Group
console ❌ 2× Uncaught TypeError ✅ clean
renders ❌ nothing ✅ edges (EdgesGeometry, non-empty)

Same app, same cube.stl — main renders nothing, this PR renders the wireframe cube:

main — wireframe STL crashes, renders nothing this PR — wireframe renders correctly
wireframe STL on main: empty scene wireframe STL on this PR: red wireframe cube

On main, EdgesGeometry is handed an empty BufferGeometry, so it throws reading .count of an undefined position attribute during creation (the object never even gets added), and the async load callback then throws again setting .geometry on the never-assigned mesh. Your version — building EdgesGeometry inside the load callback + deferring material via pendingMaterialInfo — is the correct fix.

Because this is a genuine bugfix to an existing, broken public-API path (wireframe=True on scene.stl), I think it's worth landing sooner than the rest of this PR rather than riding along with the new geometry work. Two ways to do that:

  • (b) — my suggestion: split it into its own tiny PR. The repro + test below are already a complete bug report, so it can go green and merge quickly on its own, and the primitives here aren't held up by it. I'm happy to open that PR (with the test + a tests/media/cube.stl fixture) and credit you.
  • (a) or keep it here, add the STL test below, and call the fix out in the PR description so it's not a silent change.

Ready-to-use regression test (crashes on main as above, passes here):

def test_stl_wireframe(screen: Screen):
    """A wireframe STL must render as edges: a LineSegments whose geometry is EdgesGeometry."""
    scene = None
    obj = None

    @ui.page('/')
    def page():
        nonlocal scene, obj
        app.add_static_file(local_file=TEST_DIR / 'media' / 'cube.stl', url_path='/cube.stl')
        with ui.scene() as scene:
            obj = scene.stl('/cube.stl', wireframe=True)

    screen.open('/')
    screen.wait(1.0)
    result = screen.selenium.execute_script(
        f'const o = getElement({scene.id}).objects.get("{obj.id}");'
        'const child = o.children && o.children[0];'
        'return {'
        '  root_type: o.type,'
        '  child_geometry: child ? child.geometry.type : null,'
        '  edge_count: (child && child.geometry.attributes.position) ? child.geometry.attributes.position.count : 0,'
        '};'
    )
    assert result['root_type'] == 'Group', f'expected a Group wrapper, got {result}'
    assert result['child_geometry'] == 'EdgesGeometry', f'expected EdgesGeometry child, got {result}'
    assert result['edge_count'] > 0, f'expected non-empty edges, got {result}'

2. rotate docs say "intrinsic" but the implementation is extrinsic

The demo (and the PR description) describe the order as "intrinsic", but the implementation is extrinsic: order='XYZ' → Rz @ Ry @ Rx, i.e. Rx applied first about the fixed world axes. The method docstring gets this right ("the leftmost letter rotates first about the world frame"). Since extrinsic-XYZ ≡ intrinsic-ZYX, calling the 'XYZ' string "intrinsic" is misleading — it'd point users at the wrong order string. Just align the demo/description wording with the (correct) method docstring.

Everything I verified
  • Rotation math: all 6 orders + legacy 3-arg re-derived against an independent numpy reference — all match. Default 'XYZ' reduces exactly to the prior Rz @ Ry @ Rx (backward-compatible). Bad-order ValueError guard present. Ran the pure-Python tests locally: 5 passed.
  • Primitive arg alignment: clean — cone/plane/torus/capsule/lathe/polyline all line up positionally with the Three.js constructors (three@^0.180.0).
  • Polyline vertex colors: material(color=None) → JS vertexColors = (color === null) → neutral white + vertexColors=true; does not clobber per-vertex colors. traverse predicate correctly widened to isMesh || isLine.
  • polar_grid: rotateX(π/2) + translateZ(-0.01) + object_id="ground" match the existing rectangular-grid branch, so click/picking is unchanged; if (polarGrid) … else if (grid) gives it precedence, matching the docstring.
  • STL differential run on both branches: crash on main (2× Uncaught TypeError, object unregistered) vs pass on this PR. Chrome 149, headless.
  • CI: mypy / pylint / pre-commit / quick-test all green.

@evnchn

evnchn commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

@falkoschindler You may want to look at the real bug fixed also by this PR and see how to proceed, in this PR or break-out

Jepson2k and others added 3 commits July 2, 2026 14:10
The implementation composes world-frame (extrinsic) rotations; the demo
called it intrinsic, steering users to the wrong order string.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The wireframe-STL crash fix is split out per review; the STL and
material() regions now match main exactly so the fixed version merges
in cleanly once zauberzeug#6137 lands.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Jepson2k

Jepson2k commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@evnchn @falkoschindler the STL fix was split out to #6137 and docs updated as well.

@evnchn

evnchn commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Thanks @Jepson2k — that closes out both notes cleanly:

  • STL crash fix → split into Fix crash when loading STL with wireframe=True #6137, with the repro/analysis credit carried across, so it can land on its own timeline ahead of the feature work here. 👍
  • rotate order wording → the demo now reads "extrinsic" + "the leftmost letter rotates first about the world frame", which matches the (already-correct) method docstring. Exactly the fix — no more pointing users at the wrong order string.

Feature code was already correct on my end, so with the docs squared away this PR is in good shape from my side. Merge-order/timing between this and #6137 I'll leave to @falkoschindler.

Jepson2k and others added 2 commits August 11, 2026 16:31
Port the six primitives out of the deprecated scene_objects.py into
per-object components under objects/ (geometry classes via
create_geometry, Polyline via create_mesh), registered in
objects/__init__.py and the Scene alias block. rotate(order=) and
polar_grid re-apply unchanged; the old create-dispatcher branches are
superseded by the component files. Screen tests read .mesh from the
object registry records.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkVPPMQg33tFuGCVLkyrJQ
@Jepson2k
Jepson2k marked this pull request as draft August 20, 2026 17:17
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@Jepson2k
Jepson2k marked this pull request as ready for review September 26, 2026 23:47
# Conflicts:
#	nicegui/elements/scene/scene_object3d.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Type/scope: New or intentionally changed behavior review Status: PR is open and needs review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants