Merge pull request #163 from braden6521/133_OpticOrientation_fix

Optic orientation fix

example/sofast_fringe/example_process_facet_ensemble.py

@@ -164,19 +164,17 @@ def example_process_facet_ensemble():
     facet_ensemble_control = rcfe.RenderControlFacetEnsemble(default_style=facet_control, draw_outline=True)
     axis_control_m = rca.meters()
 
-    # TODO: enable once https://github.com/sandialabs/OpenCSP/issues/133 is resolved done
     # Plot slope map
-    # fig_record = fm.setup_figure(figure_control, axis_control_m, title='')
-    # ensemble.plot_orthorectified_slope(res=0.002, clim=7, axis=fig_record.axis)
-    # fig_record.save(dir_save, 'slope_magnitude', 'png')
+    fig_record = fm.setup_figure(figure_control, axis_control_m, title='')
+    ensemble.plot_orthorectified_slope(res=0.002, clim=7, axis=fig_record.axis)
+    fig_record.save(dir_save, 'slope_magnitude', 'png')
 
-    # TODO: enable once https://github.com/sandialabs/OpenCSP/issues/133 is resolved done
     # 5. Plot 3d representation of facet ensemble
     # ===========================================
-    # fig_record = fm.setup_figure_for_3d_data(figure_control, axis_control_m, title='Facet Ensemble')
-    # ensemble.draw(fig_record.view, facet_ensemble_control)
-    # fig_record.axis.axis('equal')
-    # fig_record.save(dir_save, 'facet_ensemble', 'png')
+    fig_record = fm.setup_figure_for_3d_data(figure_control, axis_control_m, title='Facet Ensemble')
+    ensemble.draw(fig_record.view, facet_ensemble_control)
+    fig_record.axis.axis('equal')
+    fig_record.save(dir_save, 'facet_ensemble', 'png')
 
     # 6. Save slope data as HDF5 file
     # ===============================

opencsp/app/sofast/lib/ProcessSofastFringe.py

@@ -645,10 +645,10 @@ def get_optic(
 
         Returns
         -------
-        FacetEnsemble | Facet
-            Optic object
+        FacetEnsemble if ProcessSofastFringe.optic_type = 'multi', otherwise Facet
         """
         facets = []
+        trans_list = []
         for idx_mirror, data in enumerate(self.data_characterization_facet):
             # Get surface points
             pts: Vxyz = data.v_surf_points_facet
@@ -673,16 +673,18 @@ def get_optic(
             mirror = MirrorPoint(pts, norm_vecs, shape, interp_type)
             # Create facet
             facet = Facet(mirror)
-            # Locate facet
+            # Locate facet within ensemble
             if self.optic_type == 'multi':
                 trans: TransformXYZ = self.data_characterization_ensemble[idx_mirror].trans_facet_ensemble
-                facet._self_to_parent_transform = TransformXYZ.from_R_V(trans.R, trans.V)
+                trans_list.append(trans)
             # Save facets
             facets.append(facet)
 
         # Return optics
         if self.optic_type == 'multi':
-            return FacetEnsemble(facets)
+            ensemble = FacetEnsemble(facets)
+            ensemble.set_facet_transform_list(trans_list)
+            return ensemble
         else:
             return facets[0]
 

opencsp/common/lib/csp/FacetEnsemble.py

@@ -1,26 +1,19 @@
 """Rigid ensemble of facets"""
 
-import copy
-from typing import Callable
-from warnings import warn
 import numpy as np
 from scipy.spatial.transform import Rotation
 
 from opencsp.common.lib.csp.OpticOrientationAbstract import OpticOrientationAbstract
 from opencsp.common.lib.csp.VisualizeOrthorectifiedSlopeAbstract import VisualizeOrthorectifiedSlopeAbstract
 from opencsp.common.lib.csp.Facet import Facet
 from opencsp.common.lib.csp.RayTraceable import RayTraceable
-from opencsp.common.lib.geometry.LoopXY import LoopXY
 from opencsp.common.lib.geometry.Pxy import Pxy
 from opencsp.common.lib.geometry.Pxyz import Pxyz
-from opencsp.common.lib.geometry.RegionXY import RegionXY, Resolution
+from opencsp.common.lib.geometry.RegionXY import Resolution
 from opencsp.common.lib.geometry.TransformXYZ import TransformXYZ
 from opencsp.common.lib.geometry.Vxyz import Vxyz
 from opencsp.common.lib.render.View3d import View3d
-from opencsp.common.lib.render_control.RenderControlFacet import RenderControlFacet
 from opencsp.common.lib.render_control.RenderControlFacetEnsemble import RenderControlFacetEnsemble
-from opencsp.common.lib.render_control.RenderControlMirror import RenderControlMirror
-from opencsp.common.lib.render_control.RenderControlPointSeq import RenderControlPointSeq
 
 
 class FacetEnsemble(RayTraceable, VisualizeOrthorectifiedSlopeAbstract, OpticOrientationAbstract):
@@ -42,16 +35,12 @@ def __init__(self, facets: list[Facet]):
 
     @property
     def facet_positions(self) -> Pxyz:
-        """
-        Finds the locations of the facets relative to the `FacetEnsemble` origin.
-        If the ensemble was only modified using the `set_facet_positions` and
-        `set_facet_canting` functions then this essentially just ignored the
-        rotations.
-        """
+        """The locations of the facets relative to the `FacetEnsemble` origin."""
         return Pxyz.merge([facet._self_to_parent_transform.apply(Pxyz.origin()) for facet in self.facets])
 
     @property
     def transform_mirror_to_self(self):
+        """List of transforms from Mirror to self"""
         return [facet.mirror.get_transform_relative_to(self) for facet in self.facets]
 
     # override from VisualizeOrthorectifiedSlopeAbstract
@@ -175,7 +164,7 @@ def draw(
 
         # individual facets
         if facet_ensemble_style.draw_facets:
-            for idx, facet in enumerate(self.facets):
+            for facet in self.facets:
                 transform_facet = transform * facet._self_to_parent_transform
                 facet_style = facet_ensemble_style.get_facet_style(facet.name)
                 facet.draw(view, facet_style, transform_facet)
@@ -194,20 +183,37 @@ def draw(
             corners_moved = transform.apply(corners)
             view.draw_Vxyz(corners_moved, close=True, style=facet_ensemble_style.outline_style)
 
-    # debug function
     def set_facet_transform_list(self, transformations: list[TransformXYZ]):
         """
         Combines the `set_facet_positions` and `set_facet_canting` functions into a single action.
         """
-        warn("Untested function")
         for transformation, facet in zip(transformations, self.facets):
             facet._self_to_parent_transform = transformation
 
     def set_facet_positions(self, positions: Pxyz):
         """
-        The function for setting the positions of the facets relative to one another.
+        Set the positions of the facets relative to one another.
+        This function updates the positions of the facets in the ensemble.
+        It will remove any previously set facet canting rotations.
 
+        Parameters
+        ----------
+        positions : Pxyz
+            A sequence of positions to set for each facet. The length of
+            this sequence must match the number of facets in the ensemble.
+
+        Raises
+        ------
+        ValueError
+            If the length of `positions` does not match the number of facets
+            in the ensemble.
+
+        Notes
+        -----
+        This method modifies the internal transformation of each facet
+        based on the provided positions.
         """
+        # "ChatGPT 4o-mini" assisted with generating this docstring.
         if len(positions) != len(self.facets):
             raise ValueError(
                 f"This FacetEnsemble contains {len(self.facets)} and"
@@ -219,6 +225,31 @@ def set_facet_positions(self, positions: Pxyz):
             facet._self_to_parent_transform = TransformXYZ.from_V(pos)
 
     def set_facet_canting(self, canting_rotations: list[Rotation]):
+        """
+        Set the canting rotations of the facets relative to the ensemble.
+        This function updates the canting rotations of the facets in the
+        ensemble. It will remove any previously set facet positional
+        transformations.
+
+        Parameters
+        ----------
+        canting_rotations : list[Rotation]
+            A list of rotation objects to set for each facet. The length
+            of this list must match the number of facets in the ensemble.
+
+        Raises
+        ------
+        ValueError
+            If the length of `canting_rotations` does not match the number
+            of facets in the ensemble.
+
+        Notes
+        -----
+        This method modifies the internal transformation of each facet
+        based on the provided canting rotations and their corresponding
+        positions.
+        """
+        # "ChatGPT 4o-mini" assisted with generating this docstring.
         if len(canting_rotations) != len(self.facets):
             raise ValueError(
                 f"This FacetEnsemble contains {len(self.facets)} and"
@@ -229,61 +260,3 @@ def set_facet_canting(self, canting_rotations: list[Rotation]):
             pos: Pxyz
             canting: Rotation
             facet._self_to_parent_transform = TransformXYZ.from_R_V(canting, pos)
-
-    # FUNCITONS BELOW THIS HAVE NOT BEEN TESTED !!!
-
-    def define_pointing_function_UNVERIFIED(self, func: Callable[..., TransformXYZ]) -> None:
-        """Sets the canting function to use. I.e., defines the
-        "set_pointing" function.
-
-        Parameters
-        ----------
-        func : Callable
-            Function that returns a "child to base" TransformXYZ object.
-        """
-        self.pointing_function = func
-
-    def set_pointing_UNVERIFIED(self, *args) -> None:
-        """Sets current facet ensemble canting (i.e. sets
-        self.ori.transform_child_to_base using the given arguments.
-        """
-        warn("Depricated, do not use OpticOrientation instance, use OpticOrietionAbstract.")
-        if self.pointing_function is None:
-            raise ValueError('self.pointing_function is not defined. Use self.define_pointing_function.')
-
-        self._self_to_parent_transform = self.pointing_function(*args)
-
-    @classmethod
-    def generate_az_el_UNVERIFIED(cls, facets: list[Facet]) -> 'FacetEnsemble':
-        """Generates HeliostatCantable object defined by a simple azimuth then elevation
-        canting strategy. The "pointing_function" accessed by self.set_pointing
-        has the following inputs
-            - az - float - azimuth angle (rotation about z axis) in radians
-            - el - float - elevation angle (rotation about x axis) in radians
-        """
-
-        def pointing_function(az: float, el: float) -> TransformXYZ:
-            r = Rotation.from_euler('zx', [az, el], degrees=False)
-            return TransformXYZ.from_R(r)
-
-        # Create heliostat
-        heliostat = cls(facets)
-        heliostat.define_pointing_function(pointing_function)
-
-        return heliostat
-
-    @classmethod
-    def generate_rotation_defined_UNVERIFIED(cls, facets: list[Facet]) -> 'FacetEnsemble':
-        """Generates HeliostatCantable object defined by a given scipy Rotation object.
-        The "pointing_function" accessed by self.set_pointing has the following input
-            - rotation - scipy.spatial.transform.Rotation
-        """
-
-        def pointing_function(rotation: Rotation) -> TransformXYZ:
-            return TransformXYZ.from_R(rotation)
-
-        # Create heliostat
-        heliostat = cls(facets)
-        heliostat.define_pointing_function(pointing_function)
-
-        return heliostat

opencsp/common/lib/csp/OpticOrientationAbstract.py

@@ -1,11 +1,11 @@
-from abc import abstractmethod, ABC
+from abc import abstractmethod
+import copy
 from warnings import warn
+
 from opencsp.common.lib.geometry.TransformXYZ import TransformXYZ
-from opencsp.common.lib.tool.typing_tools import strict_types
-import copy
 
 
-class OpticOrientationAbstract(ABC):
+class OpticOrientationAbstract:
     """
     Classes that extend OpticOrientationAbstract are objects that can be in different
     orientations, and can contain child objects (that also extend OpticOrientationAbstract)
@@ -45,23 +45,20 @@ class OpticOrientationAbstract(ABC):
     """
 
     def __init__(self) -> None:
-        # self._children: list['OpticOrientationAbstract'] = None
         self._parent: 'OpticOrientationAbstract' = None
         self._self_to_parent_transform: TransformXYZ = None
 
         self._set_optic_children()
 
     def no_parent_copy(self):
-        """
-        Deep copy of Optic without a parential attachement.
-        """
+        """Deep copy of Optic without a parential attachement."""
         copy_of_self = copy.deepcopy(self)
         copy_of_self._parent = None
         return copy_of_self
 
     def _set_optic_children(self) -> None:
-        """Call this function in the constructor to set the children of self
-        and set self as the parent of the children."""
+        # Call this function in the constructor to set the children of self
+        # and set self as the parent of the children.
         if self.children is not None:
             for child in self.children:
                 # print("debug child", child, "debug self", self)
@@ -77,9 +74,9 @@ def children(self) -> list['OpticOrientationAbstract']:
     @abstractmethod
     def _add_child_helper(self, new_child: 'OpticOrientationAbstract'):
         "Add child OpticOrientationAbstract object to self."
-        ...
 
     def add_child(self, new_child: 'OpticOrientationAbstract', new_child_to_self_transform=TransformXYZ.identity()):
+        """Adds a child to the current optic"""
         if new_child.parent is not None:
             raise ValueError(
                 "Cannot add a child optic if that child is already owned by a parent: \n"
@@ -104,14 +101,13 @@ def add_child(self, new_child: 'OpticOrientationAbstract', new_child_to_self_tra
 
     @property
     def parent(self) -> 'OpticOrientationAbstract':
-        """..."""
+        """The parent of the current Optic"""
         return self._parent
 
     @property
     def _children_to_self_transform(self) -> list[TransformXYZ]:
-        """The list of transformations that take each child OpticOrientationAbstract object
-        from their frames of reference into the 'self' frame.
-        """
+        # The list of transformations that take each child OpticOrientationAbstract object
+        # from their frames of reference into the 'self' frame.
         if self.children is None:
             return None
         return [child._self_to_parent_transform for child in self.children]
@@ -156,31 +152,28 @@ def get_transform_relative_to(self: 'OpticOrientationAbstract', target: 'OpticOr
         `TransformXYZ`
             The transformation that takes self to the `target` frame of reference
         """
-        warn("this function has not been verified")
         if target is self:
             return TransformXYZ.identity()
 
         # look up the tree
         searcher = self
         transform = TransformXYZ.identity()
-        while searcher._self_to_parent_transform is not None:
-            if searcher is target:
-                return transform
+        while searcher.parent is not None:
             transform = searcher._self_to_parent_transform * transform
             searcher = searcher._parent
+            if searcher is target:
+                return transform
 
         # look down the tree
         searcher = target
         transform = TransformXYZ.identity()
-        while searcher._self_to_parent_transform is not None:
-            if searcher is self:
-                return transform.inv()
+        while searcher.parent is not None:
             transform = searcher._self_to_parent_transform * transform
             searcher = searcher._parent
+            if searcher is self:
+                return transform.inv()
 
-        raise ValueError("target is not an in the parent-child" " tree of self.")
-
-        pass  # end of get_transform_relative_to
+        raise ValueError("The given 'target' is not an in the current parent-child tree.")
 
     def get_most_basic_optics(self) -> list['OpticOrientationAbstract']:
         """Return the list of the smallest optic that makes up composite optics."""