Fixed indexing error causing broken projections with MeshMapping #35
Conversation
I'll write an issue demonstrating the bug, but currently the CreatVertexGrid function mixes up rows and cols resulting in broken quads whenever the number of rows and cols don't happen to be the same.
| { | ||
| // 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(); |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I realize it may not have been that clear what I meant earlier. This is the calibration data before running the line "sortrows()":
And this is it after that line:
This is what I meant with converting row-major to col-major. I hope that helps visualize the issue.
2 participants
Fixes #34