diff --git a/ncore/data/__init__.py b/ncore/data/__init__.py index b2bee31a..78d4c2ed 100644 --- a/ncore/data/__init__.py +++ b/ncore/data/__init__.py @@ -53,6 +53,7 @@ ReferencePolynomial, RowOffsetStructuredSpinningLidarModelParameters, ShutterType, + register_external_distortion_parameters, ) @@ -76,6 +77,7 @@ "ConcreteCameraModelParametersUnion", "ConcreteExternalDistortionParametersUnion", "ExternalDistortionParameters", + "register_external_distortion_parameters", "ConcreteLidarModelParametersUnion", "PointCloud", "LabelCategory", diff --git a/ncore/impl/data/types.py b/ncore/impl/data/types.py index 67db92b3..ccde6463 100644 --- a/ncore/impl/data/types.py +++ b/ncore/impl/data/types.py @@ -21,10 +21,11 @@ import sys from abc import ABC, abstractmethod -from dataclasses import dataclass, replace +from dataclasses import dataclass, field, replace from enum import IntEnum, auto, unique from typing import ( TYPE_CHECKING, + Any, Callable, ClassVar, Dict, @@ -34,6 +35,8 @@ Optional, Protocol, Tuple, + Type, + TypeVar, Union, ) @@ -127,7 +130,71 @@ def __post_init__(self) -> None: ) +#: Type-var for the external distortion parameters registrar +ExternalDistortionParametersT = TypeVar("ExternalDistortionParametersT", bound=ExternalDistortionParameters) + +#: Key under which the concrete type identifier is stored *inside* the serialized external +#: distortion parameters. The identifier has to travel with the nested object: `dataclasses_json` +#: reconstructs whatever type the field is annotated with, so without it an abstract annotation +#: cannot be resolved back to a concrete class. +EXTERNAL_DISTORTION_TYPE_KEY = "external_distortion_type" + +#: Identifier assumed for external distortion parameters serialized before the type key existed. +#: The bivariate windshield model was the only concrete type at that point, so untagged data is +#: unambiguous. +_LEGACY_EXTERNAL_DISTORTION_TYPE = "bivariate-windshield" + +#: Maps the serialized identifier to the concrete external distortion parameters class +_EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE: Dict[str, Type[ExternalDistortionParameters]] = {} + + +def register_external_distortion_parameters( + parameters_class: Type[ExternalDistortionParametersT], +) -> Type[ExternalDistortionParametersT]: + """Registers a concrete external distortion parameters class for deserialization + + Usable as a class decorator. The class is keyed by its :meth:`type` identifier, which is what + the serialized form carries, so that out-of-tree external distortion parameters round-trip + through :meth:`CameraModelParameters.from_dict` as well as the in-tree ones. + """ + identifier = parameters_class.type() + existing = _EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE.get(identifier) + if existing is not None and existing is not parameters_class: + raise ValueError(f"External distortion type {identifier!r} is already registered to {existing.__name__}") + _EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE[identifier] = parameters_class + return parameters_class + + +def external_distortion_parameters_field(default: Optional[ExternalDistortionParameters] = None) -> Any: + """Field carrying external distortion parameters as a type-tagged (discriminated) object + + Encodes the concrete :meth:`ExternalDistortionParameters.type` alongside the object's own + fields and dispatches on it when decoding, so the field can be declared against the abstract + base rather than a closed union of concrete types. + """ + + def encoder(parameters: Optional[ExternalDistortionParameters]) -> Optional[Dict]: + if parameters is None: + return None + return {**parameters.to_dict(), EXTERNAL_DISTORTION_TYPE_KEY: parameters.type()} + + def decoder(encoded: Any) -> Optional[ExternalDistortionParameters]: + if encoded is None or isinstance(encoded, ExternalDistortionParameters): + # Already-typed values are passed through: `from_dict` is not the only construction + # path, and callers assign concrete parameters directly. + return encoded + encoded = dict(encoded) + identifier = encoded.pop(EXTERNAL_DISTORTION_TYPE_KEY, _LEGACY_EXTERNAL_DISTORTION_TYPE) + parameters_class = _EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE.get(identifier) + if parameters_class is None: + raise ValueError(f"Unknown external distortion type: {identifier}") + return parameters_class.from_dict(encoded) + + return field(default=default, metadata=dataclasses_json.config(encoder=encoder, decoder=decoder)) + + @dataclass +@register_external_distortion_parameters class BivariateWindshieldModelParameters(ExternalDistortionParameters): """Represents parameters required to create a windshield external distortion model""" @@ -190,9 +257,9 @@ class CameraModelParameters(dataclasses_json.DataClassJsonMixin, ABC): ) #: Width and height of the image in pixels (uint64, [2,]) shutter_type: ShutterType = util.enum_field(ShutterType) #: Shutter type of the camera's imaging sensor - external_distortion_parameters: Optional[ConcreteExternalDistortionParametersUnion] = ( - None #: Optional external distortion source associated to the camera (e.g. windshield). If a source exists, rays will be distorted prior to reaching the camera and its associated lens distortion if applicable - ) + external_distortion_parameters: Optional[ExternalDistortionParameters] = ( + external_distortion_parameters_field() + ) #: Optional external distortion source associated to the camera (e.g. windshield). If a source exists, rays will be distorted prior to reaching the camera and its associated lens distortion if applicable @abstractmethod def transform( @@ -236,7 +303,7 @@ def __post_init__(self) -> None: self.shutter_type = ShutterType(self.shutter_type) assert self.shutter_type in ShutterType.__members__.values() - assert isinstance(self.external_distortion_parameters, (type(None), ConcreteExternalDistortionParametersUnion)) + assert isinstance(self.external_distortion_parameters, (type(None), ExternalDistortionParameters)) @dataclass @@ -860,9 +927,11 @@ def encode_camera_model_parameters(camera_model_parameters: ConcreteCameraModelP "camera_model_parameters": camera_model_parameters.to_dict(), } - # Store type of external distortion, if available + # The type also travels inside the serialized parameters (see EXTERNAL_DISTORTION_TYPE_KEY). + # It is kept here as well so that readers predating the nested tag keep resolving the concrete + # type; drop this once no such reader remains. if camera_model_parameters.external_distortion_parameters: - encoded["external_distortion_type"] = camera_model_parameters.external_distortion_parameters.type() + encoded[EXTERNAL_DISTORTION_TYPE_KEY] = camera_model_parameters.external_distortion_parameters.type() return encoded @@ -875,15 +944,18 @@ def decode_camera_model_parameters(encoded_parameters: Mapping) -> ConcreteCamer # Copy as we might modify the dictionary in place camera_model_parameters = encoded_parameters["camera_model_parameters"].copy() - # Hook up typed external distortion type, if present - external_distortion_type: Optional[str] = encoded_parameters.get("external_distortion_type") - if external_distortion_type is not None: - if external_distortion_type == "bivariate-windshield": - camera_model_parameters["external_distortion_parameters"] = BivariateWindshieldModelParameters.from_dict( - camera_model_parameters["external_distortion_parameters"] - ) - else: + # Older payloads carry the external distortion type beside the camera parameters rather than + # inside them; move it into the nested object so the field's own decoder can dispatch on it. + # Payloads written since then already carry it inside and need nothing here. + external_distortion_type: Optional[str] = encoded_parameters.get(EXTERNAL_DISTORTION_TYPE_KEY) + nested_distortion = camera_model_parameters.get("external_distortion_parameters") + if external_distortion_type is not None and isinstance(nested_distortion, Mapping): + if external_distortion_type not in _EXTERNAL_DISTORTION_PARAMETERS_BY_TYPE: raise ValueError(f"Unknown external distortion type: {external_distortion_type}") + camera_model_parameters["external_distortion_parameters"] = { + **nested_distortion, + EXTERNAL_DISTORTION_TYPE_KEY: external_distortion_type, + } # Return typed camera model parameters if camera_model_type == "ftheta": diff --git a/ncore/impl/sensors/camera.py b/ncore/impl/sensors/camera.py index 00654de2..a763a94c 100644 --- a/ncore/impl/sensors/camera.py +++ b/ncore/impl/sensors/camera.py @@ -1319,7 +1319,7 @@ def get_parameters(self) -> types.FThetaCameraModelParameters: resolution=self.resolution.cpu().numpy().astype(np.uint64), shutter_type=self.shutter_type, external_distortion_parameters=cast( - Optional[types.ConcreteExternalDistortionParametersUnion], + Optional[types.ExternalDistortionParameters], map_optional(self.external_distortion, lambda x: x.get_parameters()), ), principal_point=self.principal_point.cpu().numpy().astype(np.float32) - 0.5, @@ -1547,7 +1547,7 @@ def get_parameters(self) -> types.IdealPinholeCameraModelParameters: resolution=self.resolution.cpu().numpy().astype(np.uint64), shutter_type=self.shutter_type, external_distortion_parameters=cast( - Optional[types.ConcreteExternalDistortionParametersUnion], + Optional[types.ExternalDistortionParameters], map_optional(self.external_distortion, lambda x: x.get_parameters()), ), principal_point=self.principal_point.cpu().numpy().astype(np.float32), @@ -1616,7 +1616,7 @@ def get_parameters(self) -> types.OpenCVPinholeCameraModelParameters: resolution=self.resolution.cpu().numpy().astype(np.uint64), shutter_type=self.shutter_type, external_distortion_parameters=cast( - Optional[types.ConcreteExternalDistortionParametersUnion], + Optional[types.ExternalDistortionParameters], map_optional(self.external_distortion, lambda x: x.get_parameters()), ), principal_point=self.principal_point.cpu().numpy().astype(np.float32), @@ -1852,7 +1852,7 @@ def get_parameters(self) -> types.OpenCVFisheyeCameraModelParameters: resolution=self.resolution.cpu().numpy().astype(np.uint64), shutter_type=self.shutter_type, external_distortion_parameters=cast( - Optional[types.ConcreteExternalDistortionParametersUnion], + Optional[types.ExternalDistortionParameters], map_optional(self.external_distortion, lambda x: x.get_parameters()), ), principal_point=self.principal_point.cpu().numpy().astype(np.float32), diff --git a/ncore/impl/sensors/camera_test.py b/ncore/impl/sensors/camera_test.py index 42255c5b..e3d91e3f 100644 --- a/ncore/impl/sensors/camera_test.py +++ b/ncore/impl/sensors/camera_test.py @@ -18,7 +18,7 @@ import os import unittest -from typing import List, Tuple, Union, cast +from typing import Dict, List, Tuple, Union, cast import cv2 import numpy as np @@ -31,6 +31,7 @@ from ncore.impl.common.util import unpack_optional from ncore.impl.data.types import ( + EXTERNAL_DISTORTION_TYPE_KEY, BivariateWindshieldModelParameters, CameraModelParameters, ConcreteCameraModelParametersUnion, @@ -2427,6 +2428,16 @@ def _windshield() -> BivariateWindshieldModelParameters: vertical_poly_inverse=np.array([0.0, 0.0, 1.0, 0.0, 0.0, 0.0], dtype=np.float32), ) + @classmethod + def _camera_with_distortion(cls) -> IdealPinholeCameraModelParameters: + return IdealPinholeCameraModelParameters( + resolution=np.array([640, 480], dtype=np.uint64), + shutter_type=ShutterType.GLOBAL, + principal_point=np.array([320.0, 240.0], dtype=np.float32), + focal_length=np.array([500.0, 500.0], dtype=np.float32), + external_distortion_parameters=cls._windshield(), + ) + def test_dispatches_to_concrete_model(self): parameters = self._windshield() model = external_distortion_model_from_parameters(parameters, device="cpu") @@ -2486,11 +2497,11 @@ def encode(parameters: ExternalDistortionParameters) -> dict: ExternalDistortionParameters.type() def test_from_dict_reconstructs_the_concrete_distortion_type(self): - # `dataclasses_json` constructs whatever type the field is annotated with, so this is the - # path that breaks if `external_distortion_parameters` names an abstract type: the nested - # dict would deserialize into that base instead of the concrete model's parameters, and the - # failure only shows up later where the value is used. Cover the raw `from_dict` path in - # addition to the encode/decode helpers, which construct the concrete type by hand. + # `dataclasses_json` constructs whatever type the field is annotated with, so with the + # field declared against the abstract base this only works because the serialized form + # carries the concrete type (EXTERNAL_DISTORTION_TYPE_KEY) and the field's decoder + # dispatches on it. Without that, the nested dict deserializes into the base itself and the + # failure only surfaces later, where the value is used. camera = IdealPinholeCameraModelParameters( resolution=np.array([640, 480], dtype=np.uint64), shutter_type=ShutterType.GLOBAL, @@ -2502,6 +2513,75 @@ def test_from_dict_reconstructs_the_concrete_distortion_type(self): self.assertIsInstance(restored.external_distortion_parameters, BivariateWindshieldModelParameters) self.assertEqual(restored.to_json(), camera.to_json()) + def test_serialized_form_carries_the_concrete_type(self): + # The discriminator has to sit inside the nested object: that is what a field declared + # against the abstract base has to dispatch on when reconstructing. + camera = self._camera_with_distortion() + nested: Dict = dict(cast(Dict, camera.to_dict()["external_distortion_parameters"])) + self.assertEqual(nested[EXTERNAL_DISTORTION_TYPE_KEY], "bivariate-windshield") + + def test_untagged_legacy_payload_still_decodes(self): + # Payloads written before the nested type key exist in stored data; they are unambiguous + # because the bivariate windshield was the only concrete type at the time. + camera = self._camera_with_distortion() + encoded: Dict = dict(camera.to_dict()) + legacy: Dict = dict(cast(Dict, encoded["external_distortion_parameters"])) + legacy.pop(EXTERNAL_DISTORTION_TYPE_KEY) + encoded["external_distortion_parameters"] = legacy + restored = IdealPinholeCameraModelParameters.from_dict(encoded) + self.assertIsInstance(restored.external_distortion_parameters, BivariateWindshieldModelParameters) + self.assertEqual(restored.to_json(), camera.to_json()) + + def test_legacy_camera_level_type_is_migrated_by_the_decoder(self): + # The old encoded layout stored the type beside the camera parameters, with an untagged + # nested object. decode_camera_model_parameters has to move it inside. + camera = self._camera_with_distortion() + encoded = encode_camera_model_parameters(camera) + camera_parameters: Dict = dict(cast(Dict, encoded["camera_model_parameters"])) + nested: Dict = dict(cast(Dict, camera_parameters["external_distortion_parameters"])) + nested.pop(EXTERNAL_DISTORTION_TYPE_KEY) + camera_parameters["external_distortion_parameters"] = nested + legacy: Dict = dict(encoded) + legacy["camera_model_parameters"] = camera_parameters + decoded = decode_camera_model_parameters(legacy) + self.assertIsInstance(decoded.external_distortion_parameters, BivariateWindshieldModelParameters) + + def test_decode_accepts_old_and_new_payload_structures(self): + # The two serialized layouts must decode to the same parameters: + # + # old: the type sits beside the camera parameters, the nested object is untagged + # new: the type sits inside the nested object (both are written, for older readers) + # + # Stored data contains the old layout, so this equivalence is what lets the field be + # declared against the abstract base without a migration. + camera = self._camera_with_distortion() + new_payload = encode_camera_model_parameters(camera) + + camera_parameters: Dict = dict(cast(Dict, new_payload["camera_model_parameters"])) + nested: Dict = dict(cast(Dict, camera_parameters["external_distortion_parameters"])) + self.assertIn(EXTERNAL_DISTORTION_TYPE_KEY, nested) + nested.pop(EXTERNAL_DISTORTION_TYPE_KEY) + camera_parameters["external_distortion_parameters"] = nested + old_payload: Dict = dict(new_payload) + old_payload["camera_model_parameters"] = camera_parameters + + decoded_old = decode_camera_model_parameters(old_payload) + decoded_new = decode_camera_model_parameters(new_payload) + + for decoded in (decoded_old, decoded_new): + self.assertIsInstance(decoded.external_distortion_parameters, BivariateWindshieldModelParameters) + self.assertEqual(decoded_old.to_json(), decoded_new.to_json()) + self.assertEqual(decoded_old.to_json(), camera.to_json()) + + def test_unknown_serialized_type_raises(self): + camera = self._camera_with_distortion() + encoded: Dict = dict(camera.to_dict()) + nested: Dict = dict(cast(Dict, encoded["external_distortion_parameters"])) + nested[EXTERNAL_DISTORTION_TYPE_KEY] = "not-a-real-distortion" + encoded["external_distortion_parameters"] = nested + with self.assertRaises(ValueError): + IdealPinholeCameraModelParameters.from_dict(encoded) + def test_abstract_base_is_not_instantiable(self): # `ABC` does not prevent instantiation without an abstract method, and `type()` is # deliberately non-abstract, so the base guards itself explicitly. @@ -2509,12 +2589,8 @@ def test_abstract_base_is_not_instantiable(self): ExternalDistortionParameters() def test_encode_decode_roundtrip_with_distortion(self): - # The `external_distortion_parameters` field deliberately keeps naming the concrete union - # rather than the new abstract base: widening a field a caller *reads* is a breaking change - # for downstream consumers that pass it on to concretely-typed APIs, and it also changes - # what `dataclasses_json` reconstructs (see test_from_dict_reconstructs_the_concrete_ - # distortion_type). It is left to the separate widening pass that covers the other read - # positions. This pins the round-trip that has to keep working either way. + # The round-trip through the encode/decode helpers, which carry the type beside the camera + # parameters as well as inside them. camera = IdealPinholeCameraModelParameters( resolution=np.array([640, 480], dtype=np.uint64), shutter_type=ShutterType.GLOBAL,