Closed chinmaychinara91 closed 2 weeks ago
tutorials in GUI and tutorial scripts
Can you point me to this ?
Can you point me to this ?
@chinmaychinara91
There are many tutorials in this page, that you can follow to verify the changes are backwards compatible https://neuroimage.usc.edu/brainstorm/Tutorials/
Most tutorials can be run with scripts, located in main Brainstorm repository. Look for the tutorial_*.m
scripts:
https://github.com/brainstorm-tools/brainstorm3/tree/master/toolbox/script
There is also this private repo: https://github.com/brainstorm-tools/bst-tests
Hey guys, unfortunately, we do not have a revopoint yet. Can you provide the following to tests the code in the PR?
In addition, have you had the chance to run the digitization with Polhemus, to see that everything is fine with that?
Hey guys, unfortunately, we do not have a revopoint yet. Can you provide the following to tests the code in the PR?
- Some raw files to test the import of the new surface format
- A protocol where the sensor localization has been already performed
In addition, have you had the chance to run the digitization with Polhemus, to see that everything is fine with that?
@rcassani:
I ran it in the simulated mode without the Polhemus connected. Functionalities looked good and nothing broke. Polhemus here at USC has gone for some repair. Once it is back I will check it.
Also ran the exhaustive testing with various tutorials as you instructed above. All looked good.
In addition, have you had the chance to run the digitization with Polhemus, to see that everything is fine with that?
we can test that at USC, we have the digitizer around.
but if I understand it, this will not be an issue since the Digitizer simulation was working fine.
@chinmaychinara91, thank you for the data
but if I understand it, this will not be an issue since the Digitizer simulation was working fine.
Correct, it should not be an issue. Though we must test it before merging the PR
@chinmaychinara91, thank you for the data
but if I understand it, this will not be an issue since the Digitizer simulation was working fine.
Correct, it should not be an issue. Though we must test it before merging the PR
Yes definitely. We will test it here and let you know.
@chinmaychinara91, thank you for the data
but if I understand it, this will not be an issue since the Digitizer simulation was working fine.
Correct, it should not be an issue. Though we must test it before merging the PR
Yes definitely. We will test it here and let you know.
It may require some time to do that on the digitizer itself. we need to set it up and run it.
@Moo-Marc, can you give us a hand with the test with the Polhemus?
It may require some time to do that on the digitizer itself. we need to set it up and run it.
Will be testing the PR with Polhemus today. Will keep posted.
The Polhemus here at USC is being bit of a task as it just got back from repairs. @Moo-Marc could you help us in testing this PR out at your end ? Just want to make sure already available functionalities do not break.
Hi, not sure why but I had not received notifications for your messages. Sure I'll help test. Hopefully we don't get conflicts with my other digitizer PR.
@Moo-Marc thank you so much. Do let me know once you test it out.
d753abd commit takes into account that the New Digitize Panel changes (https://github.com/brainstorm-tools/brainstorm3/pull/682) are merged to this PR.
@rcassani this is ready for review. Both the legacy and new panel support Revopoint now. Do test it out on your end based on the tutorial in description and let me know if you face any issues.
@rcassani thank you for your comments. I have replied to them individually. Do go through them. But to summarize yes it needs another round of cleanup from my end which I will work on this week. Definitely it is not generalized well and there are some hard coded values but we are yet to receive more samples with variety of caps to get a good sense of generalizing them.
@chinmaychinara91, related to 53f43dd.
Do not patch code just to make it work. You have to understand why there was a problem, and how it can be solved from origin.
Moreover, remember to keep previous behaviours otherwise:
All the calls must be updated. E.g. the proposed changes brakes the calls brainstorm digitize
https://github.com/brainstorm-tools/brainstorm3/blob/23b805bea0cecaa09ef41de416fd95b8e65176b1/brainstorm.m#L141
If user's code rely on this they will need to change their codes.
In addition, it would need to update the usage strings for panel_digitize
and panel_digitize_2024
What about solving the bug from root like below?
This solution is compatible with former ways using 'Start' with panel_digitize
and panel_digitize_2024
--- a/toolbox/sensors/panel_digitize.m
+++ b/toolbox/sensors/panel_digitize.m
@@ -40,7 +40,7 @@ end
%#function icinterface
%% ===== START =====
-function Start(DigitizerType) %#ok<DEFNU>
+function Start(varargin) %#ok<DEFNU>
global Digitize
% ===== INITIALIZE CONNECTION =====
% Intialize global variable
@@ -68,10 +68,11 @@ function Start(DigitizerType) %#ok<DEFNU>
'trans', []));
% ===== PARSE INPUT =====
- Digitize.Type = 'Digitize';
- if nargin > 0 && ~isempty(DigitizerType)
- Digitize.Type = DigitizerType;
+ DigitizerType = 'Digitize';
+ if nargin > 0 && ~isempty(varargin{1})
+ DigitizerType = varargin{1};
end
+ Digitize.Type = DigitizerType;
switch DigitizerType
case 'Digitize'
% Do nothing
Same thing must be done for panel_digitize_2024
@rcassani
I have made all the changes based on the comments and discussions above and on Slack. Let me know if you find any errors on your side.
To summarize:
reducepath
.auto_3dscanner
now handles all the automatic detection and labelling of 3D Scanner acquired mesh. For new caps, we just need to change the getEegCapLandmarkLabels
function and that should be it.imfindcircles
with the caps we have available. Check https://github.com/brainstorm-tools/brainstorm3/pull/716/commits/3403c9ef4359220ca9b189425b14ea901ef3f079.Digitize.Transf
transformation once the fiducials are digitized.Tutorial updated partially. Should be good for your testing. Will do a final update of tutorial after it is ready to merge.
For importing an OBJ into brainstorm:
in_tess_wftobj
function in actually a slightly modified submodule of https://www.fieldtriptoolbox.org/reference/fileio/ft_read_headshape. Should we use the fieldtrip function directly or since the license allows us, just continue to do it as it is right now (and just put a reference to the fieldtrip function).Both these functions are available as part of spm12 plugin (/external/fieldtrip) and also the fieldtrip plugin. How do we go about this ?
@tmedani
In short, it does not makes too much sense to request the user to install FieldTrip as plugin and Load it to perform trivial operations and things that already in the Brainstorm source code. Moreover, not all functions in FieldTrip are available in the compiled version of Brainstorm.
- The in_tess_wftobj function in actually a slightly modified submodule of https://www.fieldtriptoolbox.org/reference/fileio/ft_read_headshape. Should we use the fieldtrip function directly or since the license allows us, just continue to do it as it is right now (and just put a reference to the fieldtrip function).
in_tess_off.m
. Do you have any trouble reading this format of file? Do we want to install full FieldTrip for this?
- We also use the https://www.fieldtriptoolbox.org/reference/forward/ft_convert_units function to determine the units of the vertices based on the shape of the head. This is a good function to have.
Same thing as the point above. Do we want to install full FieldTrip for this?
A conversion of surface from Brainstorm > FieldTrip > Brainstorm would be needed.
Choosing the right scaling for distance units is shown in the Brainstorm tutorials. This is done with a function that is already in Brainstorm, channel_fixunits
@rcassani thanks for pointing out the resources. That makes things much simpler. I don't think we will need FT now.
Code looks good, almost ready to merge.
- Most of the code duplication mentioned in the previous review has been addressed.
- @chinmaychinara91 I have addressed several of the comments in this review. Please take a look to those changes so you can improve future PRs
@rcassani thank you for the review. I will work on the comments you gave and get back.
@chinmaychinara91, @tmedani It seems this PR is ready.
Please add a warning in the tutorials and in the GUI (e.g. when clicking on the Auto button) that the Auto feature is experimental, and results must be carefully verified.
Please add a warning in the tutorials and in the GUI (e.g. when clicking on the Auto button) that the Auto feature is experimental, and results must be carefully verified.
Agreed will do
Did the tests. All look good now. It can be merged.
Tutorials updated. Found some more bugs while doing some tests and fixed them in https://github.com/brainstorm-tools/brainstorm3/pull/716/commits/53f04ca38eab3370a2b707eef5485ed7af60cd44 and https://github.com/brainstorm-tools/brainstorm3/pull/716/commits/bcacbcaea9a71017d82ecb3f112bb3f89c1a59b4. It is good to go now.
Tutorial: https://neuroimage.usc.edu/brainstorm/Tutorials/TutDigitize3dScanner#Digitize_using_Revopoint_Digitize_GUI_in_Brainstorm
Test Data: Can be downloaded from here.
Goal is to allow automatic localization and labelling of EEG caps using 3D scanned mesh of a person wearing the cap (captured using Revopoint 3D). This represents a new low-cost, fast, easy-to-use approach for digitizing heads and electrodes as compared to the traditional digitizer.
NOTE: Currently tested well with two configurations of EEG caps:
https://github.com/brainstorm-tools/brainstorm3/pull/716/commits/d40c719ede6ee167c290c7b526836d57811761cahttps://github.com/brainstorm-tools/brainstorm3/pull/716/commits/c49ebc6cc474979bfd634731f5be67dcca33e6bcProposed GUI within Brainstorm LEGACY MODE
2024 MODE
@tmedani @ajoshiusc @yashvakilna