Skip to content

Fixed indexing error causing broken projections with MeshMapping #35

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixed indexing error causing broken projections with MeshMapping #35

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion BonVision/CreateVertexGrid.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public IObservable<Matrix2x3[]> Process(IObservable<float[]> source)
return source.ToArray().Select(value =>
{
// The number of rows in the grid is equal to the number of distinct values of 'v'
var rows = value.Select(row => row[3]).Distinct().Count();
var rows = value.Select(row => row[2]).Distinct().Count();
Copy link
Contributor

@glopesdev glopesdev May 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NabbefeldG great find, thanks for the fix!

Just to clarify, does this mean we mixed up u and v in the mesh mapping calibration files? Usually in texture mapping land, v is the vertical dimension, but looks like here the fix was to have the number of rows extracted from the number of distinct values of u?

Is everything else with u and v correct? We should probably clarify the meaning of the different file columns in the source code comments (e.g. the comment immediately above the code refers to distinct values of 'v' but this seems to be no longer the case since we now point to u). Updating the comments would make it easier to think about any future changes and help readability.

Copy link
Author

@NabbefeldG NabbefeldG May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure at first. The matrix that is generated in Bonsai has the structure (x, y, u, v). This is read in by the provided Matlab script, interpolated and then saved again in the same shape with the additional filed for intensity (x, y, u, v, i). However, there is one line in the Matlab script that reorders the entire matrix:
(Line 44) MeshMapping = sortrows(MeshMapping,3);
This changes the ordering of the values. While the "MeshMapping_generate2023.bonsai" script generates and saves data represented along the horizontal and then moving through the columns (Row-Major), the Matlab script flips this and turns this matrix into Column-Major. There likely was a reason for this, as Matlab represents matrices as Col-Major instead.
So, the original version of "CreateVertexGrid.cs" with index 3 is correct after all, but the for-loops assume Row-Major, even though the Matlab script converted it to Col-Major.
I think there are two ways to solve this. Either the CreateVertexGrid() needs to sort its inputs to enforce the Row-Major assumption, or you allow for both represenations and give the CreateVertexGrid() the additional input to select "Row-Major" or "Column-Major" (and a possible third with: "Auto" that sorts the input). While the first feels like the more straight forward fix, the second would allow users to decide to keep there current stimuli. I can image that there are a number of experimentalists out there currently using this function with settings that are just very slighly off and we don't just want to break there stimuli with an update.
What do you think?

Copy link
Author

@NabbefeldG NabbefeldG May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize it may not have been that clear what I meant earlier. This is the calibration data before running the line "sortrows()":
Before_sortrows

And this is it after that line:
After_sortrows

This is what I meant with converting row-major to col-major. I hope that helps visualize the issue.

var cols = value.Length / rows;

var i = 0;
Expand Down