docs(contract): write down the spatial conventions; state the import callback rule the code keeps

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,

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 |
 |-------|------|-------|

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;

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.

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);

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;

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;
 }
 

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<bool> cancelled{false};
   std::atomic<bool> dataset_notified{false};

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.