Skip to content

Conversation

cjsha
Copy link
Member

@cjsha cjsha commented Aug 1, 2025

This workflow works:
image

This workflow doesn't work:
image

The second workflow encapsulates TS4231V1PositionData and TS4231V1SpatialTransform into a single node. It yields the following error when trying to open the GUI:
image

I'm trying to encapsulate TS4231PositionData and TS4231V1SpatialTransform into a single node because the correctness of the SpatialTransform depends on persistent P & Q values. When we discussed this, we said we want whatever TS4231V1TransformedPositionData node we come up to have no external dependencies e.g. to be a pure function.

I suspect the error is happening because the underlying bonsai infrastructure that I'm using to pipe TS4231V1PositionDataFrames into the GUI is looking for an input node, and it no longer considers TS4231V1PositionData as an input node once it's in a GroupWorkflow or IncludeWorkflow (however you make the encapsulated workflow).

Besides trying to fix this error, I think it would be helpful to refactor this branch of commits bc I took quite a circuitous path here (like making edits to TS4231PositionData and then reverting all those changes).


Fix #427

cjsha and others added 12 commits August 1, 2025 14:50
- Consolidate win form event handlers
- Initialize SpatialTransformMatrix property to indentity matrix
- Refactor some functions here and there
- This is a more elegant than using the `WinForm.Control.Invoke` pattern to avoid cross-threading errors
- I think the the sptial transform should be another property within
  TS4231PositionData that allows the user to map the base-station-based
  coordinate system into an external coordinate system
- Defaulting to identity matrix means that this does nothing by default.
- add cancel button
- add timeout (probably only need timeout or cancel button)
- check workflow is running before opening GUI
- leave text boxes blank
- correctly populate ts4231V1CoordinatesMatrix
- give persistent scope to subscriptions so they can be disposed
- simplify conditional statement for checking if user input is valid
- minor edit to top-level label to be consistent with changes
…Dialog

- This should recover the raw position values from P and Q alone without
  the M that is currently being edited.
jonnew's feedback:
- Add status strip
- Use "OK", "Cancel" button paradigms

additionally:
- Improve resizing
- Address all VS messages
  - PascalCase methods
  - Make "using" syntax more concise
  - Add/Edit some XML comments
- Instead of transforming every Vector3 and then averaging, average all Vector3s and then take the transform
- Inline/remove the GetData() function
- The operator is now an included workflow comprising of ts4231
  source node and a spatial transform node
- The property that's set by the dialog is a struct containing
  pre-transform coordinates, post-transform coordinates and spatial
  transform matrix instead of just the matrix
- Add workflows to ItemGroup in Onix1.csproj
- Revert TS4231V1PositionData
- Dialog changes:
  - Add X, Y, Z labels
  - Add a textbox for each component of each coordinate instead of
    one textbox
  - Add a textbox & label for displaying Spatial Transform Matrix
  - Change status messages TextBox to RichTextBox which allows
    changing font color and using newline characters instead of
    environment.newline.
  - Automatically calculate transform matrix when inputs are valid
    (avoids decoupling sets of pre-transform & post-transform
    coordinates and the spatial transform)
  - Simplify bottom toolstrip behavior
  - Move event handlers to top and helper methods underneath
  - Change instructions in top label according to the above changes
@cjsha
Copy link
Member Author

cjsha commented Aug 4, 2025

For context, I already tried combining these two TS4231V1PositionData and TS4231V1SpatialTransformPositionData nodes into one, but then we wouldn't have an input source of data to pipe into the GUI. We'd instead be taking measurements based on the output of this source node which is not ideal because the output of this combined source node undergoes a spatial transform. This would require performing an inverse spatial transform to the data in order to do a proper calculation of a new spatial transformation matrix.

One workaround could be to make a new dataframe that outputs position data and transformed position data, if for some reason Bonsai has real limitations.

cjsha added 3 commits August 18, 2025 15:20
- Add cancel button
- Add timeout (probably only need timeout or cancel button)
- Check workflow is running before opening GUI
- Add status strip
- Use "OK", "Cancel" button paradigms

Also:

- Give persistent scope to subscriptions so they can be disposed
- Simplify conditional statement for checking if user input is valid
- Minor edit to top-level label to be consistent with changes
- Improve resizing
- PascalCase methods
- Make "using" syntax more concise
- Add/Edit some XML comments
- Instead of transforming every Vector3 and then averaging, average all Vector3s and then take the transform
- Inline/remove the GetData() function
- Add X, Y, Z labels
- Add a textbox for each component of each coordinate instead of
  one textbox
- Add a textbox & label for displaying Spatial Transform Matrix
- Change status messages TextBox to RichTextBox which allows
  changing font color and using newline characters instead of
  environment.newline.
- Automatically calculate transform matrix when inputs are valid
  (avoids decoupling sets of pre-transform & post-transform
  coordinates and the spatial transform)
- Simplify bottom toolstrip behavior
- Move event handlers to top and helper methods underneath
- Change instructions in top label according to the above changes
- Change some text in the confirmation dialog
- Remove private access modifiers where they're not necessary
@cjsha cjsha force-pushed the issue-427_rebased branch from 89b1c78 to cc4eee6 Compare August 18, 2025 20:46
@cjsha cjsha marked this pull request as ready for review August 18, 2025 20:46
@cjsha
Copy link
Member Author

cjsha commented Aug 18, 2025

Previous discussion on this branch can be found here #432

@cjsha
Copy link
Member Author

cjsha commented Aug 18, 2025

I rebased 1) to catch up this branch with main and 2) to remove some of the commits that added extraneous files or no-longer-needed edits (because I took a roundabout path to get this branch to where it is now). I'm wondering if it makes more sense to just squash and merge though.

@jonnew
Copy link
Member

jonnew commented Aug 22, 2025

Looking very good.

Since you are providing instruction in the text box, might be nice to be even more explicit:

  1. Determine a set of 4, well separated in X, Y, and Z position in the space in which the headstage will move. These position should explore a large region of the territory the headstage will explore and not be confined to a particular plane.
  2. For each coordinate in the table below, place the headstage at each of the positions defined in step 1 and click measure on the GUI. After the TS4231 coordinate is obtained from the headstage, enter the known User coordinates in the X, Y, and Z text boxes to provide your spatial mapping. Repeat this process for all coordinates.
  3. "

@jonnew
Copy link
Member

jonnew commented Aug 22, 2025

Lets provide a link here?

image

@jonnew
Copy link
Member

jonnew commented Aug 22, 2025

Ill need to finish the review on monday when I have access to the hardware for testing

- Change instructions label to rich text box for improved formatting
- Use some text from jonnew in PR comment as basis for the content of the rich text box
- rich text box detects URLs to make clickable hyperlinks if desired
- remove private access modifiers where not necessary
@cjsha
Copy link
Member Author

cjsha commented Aug 22, 2025

I'm hesitant to add a link in the GUI. With that said, the instructions at top are now a rich text box (instead of a basic label) as of my last commit which detects URLs and turns them to clickable hyperlinks so it's easy to add if that's what we decide.

@jonnew
Copy link
Member

jonnew commented Aug 25, 2025

This is looking really good. I think something that we need to make abundantly clear in the documentation, and that i found during my testing, is that this will work best if the X,Y, and Z measurements, create a box that covers the area of interest. I found that if I e.g. used a small Z and then placed the headstage outside of the calibration area in Z, large errors in X and Y could accumulate. In the case below, the 4 yellow dots are the calibration points. When the headstage is placed at a Z well outside this area with X and Y at zero, X and Y accumulate errors:

image Screenshot 2025-08-25 092455

If I repeat this but use a Z calibration that is at the star in the image above, the these errors largely go away.

@jonnew
Copy link
Member

jonnew commented Aug 28, 2025

Can we make a function that does a better job formatting the Matrix to show in the GUI. Here is one:

        static string ToPrettyString(Matrix4x4 matrix, int decimals = 2, int padding = 7)
        {
            
            string format = $"F{decimals}";

            string[,] elements = new string[4, 4]
            {
            { matrix.M11.ToString(format).PadLeft(padding),
              matrix.M12.ToString(format).PadLeft(padding),
              matrix.M13.ToString(format).PadLeft(padding),
              matrix.M14.ToString(format).PadLeft(padding) },
            { matrix.M21.ToString(format).PadLeft(padding),
              matrix.M22.ToString(format).PadLeft(padding),
              matrix.M23.ToString(format).PadLeft(padding),
              matrix.M24.ToString(format).PadLeft(padding) },
            { matrix.M31.ToString(format).PadLeft(padding),
              matrix.M32.ToString(format).PadLeft(padding),
              matrix.M33.ToString(format).PadLeft(padding),
              matrix.M34.ToString(format).PadLeft(padding) },
            { matrix.M41.ToString(format).PadLeft(padding),
              matrix.M42.ToString(format).PadLeft(padding),
              matrix.M43.ToString(format).PadLeft(padding),
              matrix.M44.ToString(format).PadLeft(padding) }
            };

            // Build the formatted matrix string with brackets and commas
            var sb = new StringBuilder();
            sb.Append("[[");

            for (int row = 0; row < 4; row++)
            {
                for (int col = 0; col < 4; col++)
                {
                    sb.Append(elements[row, col]);
                    if (col < 3) sb.Append(",");
                }
                sb.Append("]");

                if (row < 3) {
                    sb.Append(",");
                    sb.AppendLine();
                    sb.Append(" [");
                }  else
                {
                    sb.Append("]");
                }
            }
            return sb.ToString();
        }

If used with a monospaced font like courier new, it looks good. I didnt commit this because I messed around with the GUI docking settings and I think I probably messed some things up. Please just make it so that the matrix is fully visible on the default size like it is here:

image

Also fix instructions at top
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to apply a spatial transform matrix to ts4231 data in the data operator
2 participants