Add operator to spatial transform ts4231 data #432

cjsha · 2025-04-16T13:29:11Z

Resolve #427

bparks13

Overall looks good, I did not test the functionality but did leave some comments on some potential restructuring of the code, and the need to add XML comments before we can merge this.

OpenEphys.Onix1.Design/SpatialTransformMatrixDialog.cs

OpenEphys.Onix1.Design/SpatialTransformMatrixEditor.cs

OpenEphys.Onix1/SpatialTransform.cs

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

jonnew

This is an interesting idea but we need to do some things to make it useful generally. I've committed some changes that start to move in this direction. Other refinements are

The user needs to be able to cancel a measurement and start over. Or a timeout needs to be set that will cancel a measurement if some time has elapsed before lighthouse data is received.
Ideally the editor should not display unless the workflow is running. I think there are ways to check the run state of a workflow when an editor is opened.

- 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

cjsha · 2025-04-23T00:09:53Z

I made changes and did some cursory testing to make sure it looks ok and still works. I didn't have time to extensively test this latest commit.

There is one problem with the current implementation which is that the calculated matrix spatial transform is only correct if we are already starting with an identity matrix.

jonnew · 2025-05-06T20:45:29Z

It seems like the user supplied coordinates are verified in real-time for the correct format. It would be nice to have a status text indicating if the input is valid or not (e.g. "input coordinates invalid vs. input coordinates valid". You can a tool strip for this (see e.g. Rhs2116StimulusSequenceDialog.cs:

jonnew · 2025-05-06T20:49:13Z

Instead of this:

use the standard "OK, Cancel" pattern:

See Rhs2116StimulusSequenceEditor for an example.

…Dialog - This should recover the raw position values from P and Q alone without the M that is currently being edited.

jonnew

Almost there. I added some comments and I think i took care of the issue with non-default values of M.

Please do some verification with real headstage in known space and wacky values for initial value of M.

cjsha · 2025-05-07T12:59:15Z

@jonnew Re:

It would be nice to have a status text indicating if the input is valid or not (e.g. "input coordinates invalid vs. input coordinates valid". You can a tool strip for this (see e.g. Rhs2116StimulusSequenceDialog.cs

I'm wondering if it would be worth indicating which coordinate is invalid. like having green checks or red x-es for each row or each cell (probably instead of the status strip) as indicated by this screenshot:

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

cjsha · 2025-05-20T00:02:23Z

I addressed feedback (look at commit message for more details). I tested calculating a new M from a pre-set M. This seems to work OK.

jonnew · 2025-05-30T15:28:44Z

If I click measure for a coordinate with data in it, and the measurement is invalid, it doesnt get rid of the old measurement

Desired?

jonnew · 2025-05-30T15:52:02Z

Here are some changes I think would be helpful to the UI

In addition, please put more line breaks between items in the instructions. Its very compact and hard to read.
Also, make sure Tab works as expected for the entered values (goes in row major order)

jonnew · 2025-05-30T16:02:36Z

It is possible to create a transform that collapses dimensions by providing zero for a given dimension. For instance if all my calibration data is in a single X,Y plane, this is the result:

This should not be permitted.

jonnew · 2025-05-30T16:08:14Z

Several overall architectural comments:

The property M should be replaced with a custom class that holds both M and all the measurements and corresponding user coordinates used to create it. This way, when the GUI is invariably reopened, the user can make incremental edits instead of needing to start over.
The need to invert M to reopen the GUI needs to go away. To do this, I suggest that we turn this into a transform called TS4231SpatialTransform. TS4231PositionData remains the uncalibrated position data its always been. A new, EmbeddedWorkflow is created called TS4231CalibratedPositionData that combines TS4231PositionData with the TS4231SpatialTransform. This way the transform always has access to untransformed data an inversion of M is not required.

jonnew

Please have a look at my comments in the PR

- 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

jonnew

The behavior of this is now correct. In terms of implementation I've provided some comments below. Additionally you should perform a git rebase main on this to bring it up to date. I did this and there was one trivial conflict in the csproj file in which you can accept main's version in its entirety.

jonnew · 2025-07-24T13:25:49Z

OpenEphys.Onix1/SpatialTransformProperties.cs

+        /// <summary>
+        /// The set of coordinates before undergoing a spatial transform.
+        /// </summary>
+        public Vector3[] Pre { get; set; }


Ultimately this and Post ends up being a Matrix4x4 when used in the inversion operation to find M. It seems like they should be stored as such as private variables with controlled access via setting functions that accept input types that are more amenable to interaction with the GUI elements. (e.g. Vector3 and index). Currently there is nothing here that controls the length of these vectors since they have public sets.

jonnew · 2025-07-24T13:27:07Z

OpenEphys.Onix1/SpatialTransformProperties.cs

+            Array.Copy(other.Post, Post, 4);
+            M = other.M;
+        }
+    }


This class should probably be called SpatialTransform3D or something and actually contain the math for performing the inversion operation whenever a correspondence (pre, post) is added.

jonnew · 2025-07-24T13:27:57Z

OpenEphys.Onix1/SpatialTransformProperties.cs

+        /// The spatial transform matrix calculated from <see cref="Pre"/> and
+        /// <see cref="Post"/>.
+        /// </summary>
+        public Matrix4x4? M { get; set; }


M should have private set and be updated as a function of whatever function adds pre/post data. This way its a function of pre and post rather than requiring manual update and public setting.

jonnew · 2025-07-24T13:30:44Z

OpenEphys.Onix1.Design/SpatialTransformMatrixDialog.cs

+
+        private void CalculatePrintMatrix()
+        {
+            SpatialTransform.M = null;


Move all of this logic the the SpatialTransformProperties class. If M is a function of pre and post there, none of this checking is required.

Further, never put logic like this in a GUI. The GUI's logic should pertain to handling of GUI elements and perhaps setting the state of the object(s) the GUI represents via getters and setters.

True. I think I achieved this now. In particular, this GUI is meant to provide the user a way of setting or viewing data in the SpatialTransform3D class which is subsequently used to transform spatial transform data.

cjsha · 2025-08-01T21:23:45Z

I rebased and pushed into another branch. I propose closing this PR and then making #477 the active one for this issue.

cjsha · 2025-08-18T20:51:31Z

I don't think it's particularly important to port my changes in issue-427_rebased back into this branch. I'm going to close this PR and we can finish our work on for the PR #477 corresponding to issue-427_rebased.

Add operator to spatial transform ts4231 data

e1ea84e

cjsha requested a review from bparks13 April 16, 2025 13:29

Bump version to 0.5.0

e43429f

cjsha requested a review from jonnew April 16, 2025 13:33

bparks13 reviewed Apr 16, 2025

View reviewed changes

cjsha added 2 commits April 16, 2025 16:07

Address (some of) bparks feedback

74c21b0

- Consolidate win form event handlers - Initialize SpatialTransformMatrix property to indentity matrix - Refactor some functions here and there

Use ControlScheduler

c3e951d

- This is a more elegant than using the `WinForm.Control.Invoke` pattern to avoid cross-threading errors

cjsha added this to the 0.5.0 milestone Apr 18, 2025

jonnew requested changes Apr 21, 2025

View reviewed changes

cjsha requested a review from jonnew April 23, 2025 00:04

Invert spatial transform of positions entering SpatialTransformMatrix…

1a1c61e

…Dialog - This should recover the raw position values from P and Q alone without the M that is currently being edited.

jonnew requested changes May 6, 2025

View reviewed changes

cjsha requested a review from jonnew May 20, 2025 00:02

ChucklesOnGitHub self-assigned this May 22, 2025

cjsha unassigned ChucklesOnGitHub May 30, 2025

jonnew requested changes May 30, 2025

View reviewed changes

jonnew modified the milestones: 0.5.0, 0.5.1 Jun 2, 2025

cjsha added 5 commits June 11, 2025 15:57

Update version prefix

a9136bc

Revert TS4231V1PositionData

f39e988

Remove System.Numerics from TS4231V1PositionData

0dd7784

Change SpatialTransformProperties struct to class

946e203

ChucklesOnGitHub requested a review from jonnew June 23, 2025 15:52

jonnew requested changes Jul 24, 2025

View reviewed changes

cjsha mentioned this pull request Aug 18, 2025

Add operator for transforming TS4231 position data #477

Merged

cjsha closed this Aug 18, 2025

Add operator to spatial transform ts4231 data #432

Add operator to spatial transform ts4231 data #432

Uh oh!

Conversation

cjsha commented Apr 16, 2025

Uh oh!

bparks13 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonnew left a comment

Choose a reason for hiding this comment

Uh oh!

cjsha commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonnew commented May 6, 2025

Uh oh!

jonnew commented May 6, 2025

Uh oh!

jonnew left a comment

Choose a reason for hiding this comment

Uh oh!

cjsha commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjsha commented May 20, 2025

Uh oh!

jonnew commented May 30, 2025

Uh oh!

jonnew commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonnew commented May 30, 2025

Uh oh!

jonnew commented May 30, 2025

Uh oh!

jonnew left a comment

Choose a reason for hiding this comment

Uh oh!

jonnew left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonnew Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

jonnew Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

jonnew Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

jonnew Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

cjsha Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

cjsha commented Aug 1, 2025

Uh oh!

cjsha commented Aug 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cjsha commented Apr 23, 2025 •

edited

Loading

cjsha commented May 7, 2025 •

edited

Loading

jonnew commented May 30, 2025 •

edited

Loading

jonnew left a comment •

edited

Loading