From 9192122ebbe36f72279e0bd1ffc9d8187b1d3863 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sat, 14 Nov 2026 15:03:46 +0100 Subject: [PATCH] docs(contract): write down the spatial conventions; state the import callback rule the code keeps Two contract texts that 1.0 would otherwise freeze wrong or missing. Spatial conventions. Nothing said which unit positions are in, which way a quaternion rotates, or how a FrameTransform maps a point. Geometry.proto now states them once: metres, right-handed frames, quaternions as (x, y, z, w) with v' = q * v * conjugate(q), all-zero read as the identity, and p_parent = R(rotation) * p_child + translation. This is what the decoders and PlotJuggler's transform code already do. The worked example in FrameTransforms.proto, copied from the upstream schema, gave (-1, 3, 0) where the result is (1, 3, 0). Descriptor import. The header promised that no job callback occurs before start_import returns true. A provider cannot keep that: a worker thread cannot be ordered against the return itself, and provider_job.hpp already said so. The rule is now what ProviderJob does: no callback before out_job is populated, none when start_import returns false, and the host has its callback state ready before it calls. Comments, docs and the changelog only. Co-Authored-By: Claude Fable 5.2 --- CHANGELOG.md | 12 ++++++++++++ docs/builtin_type.md | 18 ++++++++++++++++-- .../pj_base/builtin/frame_transforms.hpp | 9 ++++++--- .../pj_base/descriptor_import_protocol.h | 9 +++++++-- .../pj_base/sdk/source/provider_job.hpp | 6 +++--- pj_base/proto/pj/FrameTransforms.proto | 7 ++++++- pj_base/proto/pj/Geometry.proto | 18 ++++++++++++++++-- pj_base/src/source/provider_job.cpp | 4 ++-- .../tests/descriptor_import_extension_test.cpp | 11 ++++++----- 9 files changed, 74 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e8851a5..9860c2b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -216,6 +216,18 @@ Three changes that could not be made after the freeze: `kind`. Size, alignment and every offset are unchanged. No bit is defined: a host built against 1.0 or later writes 0, and a host built earlier leaves the bytes unset. +Contract text (docs only, no code change): + +- **Spatial conventions are now written down**, at the top of `Geometry.proto` and in + `docs/builtin_type.md`: metres, right-handed frames, quaternions as `(x, y, z, w)` with + `v' = q * v * conjugate(q)`, and `p_parent = R(rotation) * p_child + translation` for a + `FrameTransform`. They describe what the decoders and PlotJuggler's transform code already do. + The worked example in `FrameTransforms.proto` gave `(-1, 3, 0)` where the result is `(1, 3, 0)`. +- **Descriptor import:** the header said no job callback may occur before `start_import` returns + true. No provider can guarantee that, since a worker thread cannot be ordered against the return + itself. The rule is now what `ProviderJob` does: no callback before `out_job` is populated, none + when `start_import` returns false, and the host has its callback state ready before it calls. + ## [0.40.0] Host contract: extended: pj.mcp_endpoint.v1, PJ_mcp_endpoint_host_vtable_t::acquire_endpoint, diff --git a/docs/builtin_type.md b/docs/builtin_type.md index e60e62cf..04b26d96 100644 --- a/docs/builtin_type.md +++ b/docs/builtin_type.md @@ -346,8 +346,22 @@ reference frames. It is used for TF-style data where consumers need to place objects, point clouds, camera frustums, or markers in a shared frame graph. Use `FrameTransforms` when the semantic value is a parent/child frame -relationship: a translation vector and quaternion rotation from -`parent_frame_id` to `child_frame_id` at a specific timestamp. +relationship at a specific timestamp. Each transform places the child frame in +its parent: a point known in the child frame is +`p_parent = R(rotation) * p_child + translation`. + +**Spatial conventions, for every builtin type.** They are normative and stated +once, at the top of `pj_base/proto/pj/Geometry.proto`: + +- Positions and lengths are in metres unless a field says otherwise (image + annotations are in pixels; a depth image names its unit in `encoding`). +- Frames are right-handed. The meaning of the axes belongs to the frame named + by `frame_id`. +- A quaternion is `(x, y, z, w)` with `w` the scalar part, and rotates a vector + as `v' = q * v * conjugate(q)`. It must have unit length; all zeros is read + as the identity. +- A `Pose` places an object in its frame: + `p_frame = R(orientation) * p_local + position`. | Field | Type | Notes | |-------|------|-------| diff --git a/pj_base/include/pj_base/builtin/frame_transforms.hpp b/pj_base/include/pj_base/builtin/frame_transforms.hpp index 2276647d..ab079c7a 100644 --- a/pj_base/include/pj_base/builtin/frame_transforms.hpp +++ b/pj_base/include/pj_base/builtin/frame_transforms.hpp @@ -33,7 +33,8 @@ struct Vector3 { bool operator==(const Vector3&) const = default; }; -/// Unit quaternion representing 3D orientation. +/// Unit quaternion representing a 3D rotation, written (x, y, z, w) with w the +/// scalar part (Hamilton convention): v' = q * v * conjugate(q). struct Quaternion { double x = 0.0; double y = 0.0; @@ -42,7 +43,8 @@ struct Quaternion { bool operator==(const Quaternion&) const = default; }; -/// Rigid transform in 3D space: position + orientation. +/// Rigid transform in 3D space: position + orientation, in metres, in a +/// right-handed frame. p_frame = R(orientation) * p_local + position. /// Used by Mesh3D, OccupancyGrid, SceneEntities, and other types that /// place data in a frame of reference. struct Pose { @@ -51,7 +53,8 @@ struct Pose { bool operator==(const Pose&) const = default; }; -/// Transform from `parent_frame_id` to `child_frame_id`. +/// Places the child frame in its parent: a point known in the child frame is +/// p_parent = R(rotation) * p_child + translation. struct FrameTransform { Timestamp timestamp_ns = 0; std::string parent_frame_id; diff --git a/pj_base/include/pj_base/descriptor_import_protocol.h b/pj_base/include/pj_base/descriptor_import_protocol.h index 3e5ede42..652ea924 100644 --- a/pj_base/include/pj_base/descriptor_import_protocol.h +++ b/pj_base/include/pj_base/descriptor_import_protocol.h @@ -47,8 +47,13 @@ * until the NEXT query_descriptor call on the same plugin instance — the * caller copies immediately. * - start_import copies the request contents and the callback function - * pointers before returning; no job callback may occur before it returns - * true. + * pointers before returning. No job callback runs before out_job is + * populated, and none runs at all when start_import returns false. A + * callback MAY run before start_import has returned to its caller: the + * provider releases its worker as its last step, and a thread cannot be + * ordered against the return itself. The host therefore has everything + * its callbacks use (callback_ctx and what it points to) ready BEFORE it + * calls start_import, and must not wait for the return to set it up. * - Job callbacks are serialized but may arrive off the main thread; the * host marshals. on_dataset is zero-or-one and precedes the dataset's * progress_start, first publication, promotion, and on_terminal. diff --git a/pj_base/include/pj_base/sdk/source/provider_job.hpp b/pj_base/include/pj_base/sdk/source/provider_job.hpp index 97bb9ef3..ca1767de 100644 --- a/pj_base/include/pj_base/sdk/source/provider_job.hpp +++ b/pj_base/include/pj_base/sdk/source/provider_job.hpp @@ -102,9 +102,9 @@ class ProviderJob { /// populated and this call is returning. Make `return ProviderJob::start( /// ...)` the LAST statement of start_import — callback delivery from a /// spawned thread cannot be ordered against the ABI call's own return - /// instruction, so a host that finishes its callback bookkeeping before - /// calling start_import (as the protocol requires) observes exactly the - /// documented "no callback before start_import returns". + /// instruction. The protocol says so: a callback may run before + /// start_import has returned, and the host has its callback state ready + /// before it calls. [[nodiscard]] static bool start( Body body, const PJ_descriptor_import_callbacks_v1_t* callbacks, void* callback_ctx, PJ_joinable_job_t* out_job, PJ_error_t* out_error); diff --git a/pj_base/proto/pj/FrameTransforms.proto b/pj_base/proto/pj/FrameTransforms.proto index ad6b4077..1d08923d 100644 --- a/pj_base/proto/pj/FrameTransforms.proto +++ b/pj_base/proto/pj/FrameTransforms.proto @@ -16,13 +16,18 @@ package PJ; // frame within a parent frame. Translation moves the origin of the child frame relative to the parent origin. The // rotation changes the orientation of the child frame around its origin. // +// A point known in the child frame is expressed in the parent frame as +// p_parent = R(rotation) * p_child + translation +// with the conventions of Geometry.proto (metres, right-handed frames, unit quaternion). +// // Examples: // // - With translation (x=1, y=0, z=0) and identity rotation (x=0, y=0, z=0, w=1), a point at (x=0, y=0, z=0) in the // child frame maps to (x=1, y=0, z=0) in the parent frame. // // - With translation (x=1, y=2, z=0) and a 90-degree rotation around the z-axis (x=0, y=0, z=0.707, w=0.707), a point -// at (x=1, y=0, z=0) in the child frame maps to (x=-1, y=3, z=0) in the parent frame. +// at (x=1, y=0, z=0) in the child frame maps to (x=1, y=3, z=0) in the parent frame: the rotation turns it into +// (0, 1, 0), and the translation is added. message FrameTransform { // Timestamp of transform google.protobuf.Timestamp timestamp = 1; diff --git a/pj_base/proto/pj/Geometry.proto b/pj_base/proto/pj/Geometry.proto index 7bf706ed..3789896e 100644 --- a/pj_base/proto/pj/Geometry.proto +++ b/pj_base/proto/pj/Geometry.proto @@ -9,6 +9,20 @@ syntax = "proto3"; package PJ; +// Spatial conventions for every PJ message that uses the types in this file. +// +// - Positions and lengths are in metres. A field in another unit says so: image annotations are in pixels, a depth +// image names its unit in `encoding`, and some scene sizes are in screen pixels when `scale_invariant` is set. +// Scale factors and ratios have no unit. +// - Coordinate frames are right-handed. What the axes mean belongs to the frame named by `frame_id`; this schema fixes +// no "forward" or "up". +// - A Quaternion is a rotation written (x, y, z, w), with w the scalar part (Hamilton convention). It rotates a vector +// as v' = q * v * conjugate(q), so a positive angle turns counter-clockwise when seen from the tip of the axis. It +// must have unit length. All four components at zero is not a rotation: it is what a producer writes when it leaves +// the orientation unset, and it is read as the identity. +// - A Pose places an object in its frame: p_frame = R(orientation) * p_local + position. +// - A FrameTransform places a child frame in its parent: p_parent = R(rotation) * p_child + translation. + // A point representing a position in 2D space message Point2 { // x coordinate position @@ -51,7 +65,7 @@ message Vector3 { double z = 3; } -// A quaternion representing a rotation in 3D space +// A unit quaternion representing a rotation in 3D space; see the conventions at the top of this file message Quaternion { // x value double x = 1; @@ -62,7 +76,7 @@ message Quaternion { // z value double z = 3; - // w value + // w value (the scalar part) double w = 4; } diff --git a/pj_base/src/source/provider_job.cpp b/pj_base/src/source/provider_job.cpp index 3b2c0fb9..000f97bc 100644 --- a/pj_base/src/source/provider_job.cpp +++ b/pj_base/src/source/provider_job.cpp @@ -32,8 +32,8 @@ struct JobState { void* callback_ctx = nullptr; // The worker's FIRST action is start_gate.acquire(): the ABI forbids any job - // callback before start_import returns, and start() releases the gate as - // its last action, after out_job is fully populated. + // callback before out_job is populated, and start() releases the gate as + // its last action, after that. std::binary_semaphore start_gate{0}; std::atomic cancelled{false}; std::atomic dataset_notified{false}; diff --git a/pj_base/tests/descriptor_import_extension_test.cpp b/pj_base/tests/descriptor_import_extension_test.cpp index 81f80e45..5e1de01d 100644 --- a/pj_base/tests/descriptor_import_extension_test.cpp +++ b/pj_base/tests/descriptor_import_extension_test.cpp @@ -73,9 +73,10 @@ class ScopedLeakSanitizerDisable { // - force_unknown_trust / force_unknown_outcome: make the thunks hand back // out-of-range C enum values, pinning the view's fail-closed mapping. // - releaseStart(): the started job's worker thread blocks on a start gate -// as its FIRST action (conforming to the ABI's "no job callback may -// occur before start_import returns" rule deterministically, rather than -// racing a background thread against the caller). Tests release it once +// as its FIRST action (so no job callback runs until the test says so, +// which is stricter than the ABI's "none before out_job is populated" +// and deterministic, rather than racing a background thread against the +// caller). Tests release it once // they've observed whatever pre-release state they need to pin. class FakeImportToolbox : public PJ::ToolboxPluginBase { public: @@ -185,7 +186,7 @@ class FakeImportToolbox : public PJ::ToolboxPluginBase { } state->worker = std::thread([state, on_dataset, on_terminal, callback_ctx, force_unknown_outcome] { // FIRST action: block until the test explicitly releases us. This - // makes "no job callback before start_import returns" a deterministic, + // makes "no job callback before the gate is released" a deterministic, // assertable property (see NoJobCallbackBeforeStartGateReleased) rather // than a race the test would have to get lucky to catch. state->start_gate.acquire(); @@ -494,7 +495,7 @@ TEST(DescriptorImportExtension, MoveWhileRunningTransfersOwnershipSafely) { } TEST(DescriptorImportExtension, NoJobCallbackBeforeStartGateReleased) { - // Pins "no job callback may occur before start_import returns" using the + // Pins "no job callback before the start gate is released" using the // fake's deterministic start gate: right after startImport returns, the // worker is guaranteed still blocked, so zero callbacks having happened // yet is assertable synchronously instead of racy.