Replace drag-point editing with Unity splines - #567
Merged
Conversation
Record byte-exact goldens for every drag-point geometry consumer, including surface, ramp, rubber, wire guide, trigger-outline, and light-insert output. The fixtures include vertex positions, normals, UVs, indices, and control-point metadata so later storage and editor changes cannot alter generated geometry. Exercise open and closed curves across smooth flags and accuracy levels in both two and three dimensions. A VPX export/reload matrix also compares every BIFF field for all six Unity-backed item types, separating serialized semantics from the runtime-only IDs and calculated ramp heights.
Depend on Unity Splines 2.8.2 and expose it to the runtime, editor, and test assemblies. Convert raw VPX knot positions to broken Bezier knots with tangents ported from VPE's centripetal Catmull-Rom coefficients, preserving the exact curve while keeping the source floats out of coordinate-space round trips. Store every semantic and runtime drag-point field in serializable per-knot metadata, and rebuild derived tangents after knot edits. Edit-mode tests cover bit-identical data round trips, sampled Catmull-Rom/Bezier equivalence for open and closed curves, and rejection of authored tangent changes.
Move Rubber, Trigger, Surface, MetalWireGuide, LightInsertMesh, and Ramp onto a shared child SplineContainer with index-aligned VPE metadata. Legacy arrays now upgrade lazily, while mesh, collider, packable, and VPX export consumers keep reading computed DragPointData arrays and therefore retain their existing APIs. Place spline children under the exact VPX-to-Unity basis so knot floats remain verbatim in serialized data. Centralize insert/remove metadata alignment, derived-tangent restoration, planar constraints, Undo/prefab recording, and mesh invalidation; retain Ramp's endpoint Z-to-height behavior and cover the migration with edit-mode component tests.
Route all six drag-point inspectors through Unity's spline tool context and expose VPE metadata for the selected knots. Smoothness remains VPE-authored, while slingshot and texture fields stay aligned with native knot selection. Keep the established flip, reverse, and center-origin operations, including closed-loop orientation and slingshot rotation. Draw segment ownership and knot indices over the native gizmo, and cover the subtle transforms with editor tests.
Delete the custom control-point selection, curve traveller, context menus, and scene-view renderer now that Unity Splines owns knot editing. Remove the old inspector contract and its drag-point-only handle utility as well. Keep the remaining flip-axis detail local to the spline inspector so the new editor path no longer references any part of the retired handle stack.
Greptile SummaryThis PR replaces legacy drag-point editing with Unity spline workflows. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| VisualPinball.Unity/VisualPinball.Unity/DragPoint/DragPointSplineConverter.cs | Adds drag-point to spline conversion and recalculates derived tangents without emitting extra spline change events. |
| VisualPinball.Unity/VisualPinball.Unity/DragPoint/DragPointSplineComponent.cs | Adds spline ownership, metadata alignment, change handling, and mesh rebuild coordination for drag-point spline objects. |
| VisualPinball.Unity/VisualPinball.Unity.Test/DragPointSplineComponentTests.cs | Adds tests for spline editing, metadata alignment, generated child behavior, and single-event knot edits. |
Reviews (3): Last reviewed commit: "splines(editor): add/remove drag points ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
|
Follow-up: tangent notifications are fixed in be7e501. Inserted points remain unlocked to preserve legacy insertion behavior; ramp calculated heights are derived again during the required mesh rebuild. |
Owners null their migration copy of the drag points once a spline exists, but Unity deserializes a null array as an empty one. After any serialization round trip - entering play mode, a domain reload - an already-migrated owner therefore handed GetOrCreate an empty list rather than null, which passed the null check and applied itself over the spline it was asked to return. Both the knots and the metadata were dropped, so the item silently lost its shape. Reproducible with create trigger, start game, stop game. Treat an empty migration list as nothing to migrate, and make the surrounding code resilient to the same class of failure: - prefer the child holding the most knots when resolving a spline, so a blank created while the hierarchy was incomplete heals instead of winning - never destroy a generated child that still carries knots - log an error instead of silently creating a blank spline with no drag points Also fix the surface and ramp pivot, which relied on DragPoints aliasing the incoming data. The spline-backed getter returns fresh instances, so recentring no longer reached the data that SetReferencedData builds the child meshes from, and every new wall came out offset by its own centre. Rebase the data explicitly rather than restoring the aliasing. UnityObjectId handed out ids from a static counter, which is wiped on every domain reload, so serialized ids stopped matching and every procedural mesh concluded it had been duplicated. Derive the id from the object's EntityId. Defer the resulting rebuild out of Awake, where the hierarchy is still being restored. Guard the degenerate cases the above could produce: a rubber whose central curve collapses, and polygon colliders built from fewer than three vertices. Both indexed into empty arrays and threw from OnDrawGizmosSelected. Finally, suppress the tangent handles on drag-point splines. Drag points have no tangents - the converter derives them and reads back only positions - so dragging one changed the drawn curve but never the mesh or the colliders. The knots keep the package's own handles, backed by a sizeable ring that stays legible on a textured playfield.
`Position` is relative to the immediate parent, which only matches the playfield for kickers parented directly to it. Kickers grouped under sub-assemblies with their own rotation or offset therefore reported a wrong position to anything living in playfield space. Add `PositionInPlayfield`, which resolves the position through the whole parent chain, and use it for ball creation and for the trough's exit gizmo.
Double clicking a segment inserts a knot at the clicked position, double clicking an existing knot removes it. Picking is done in screen space so it matches what is drawn at any viewing angle. Removal keeps the minimum knot count (3 closed, 2 open) and warns instead. Also stop clearing SplineToolContext.useCustomSplineHandles when a knot placement tool owns the handles, which previously drew the default handles on top of the tool's own.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes #541.
Tests
DragPointSplineComponentTests(11 passed)RubberTests.ShouldKeepGeneratedSplinesOutOfPackageRoundTrips(passed)