Skip to content
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.

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

BF: should not fix units="pix" when drawing TargetStim during calibration #6461

Closed
wants to merge 52 commits into from

Conversation

mh105
Copy link
Contributor

@mh105 mh105 commented May 14, 2024

This was hell of a bug to hunt down...

The problem is as the following: when using the EyeTracker Calibration/Validation components in Builder, the GUI allows setting of Outer radius and Inner radius, and the Spatial units is grayed out as following the screen unit in the experiment settings. This is great and parsimonious design. But after some convoluted layers of overloaded dict() calls, we get to this part of psychopy.iohub.devices.eyetracker.calibration.procedure.BaseCalibrationProcedure. And it assumes the target_attributes to contain outer_diameter and inner_diameter in pixels.

This causes the strange behavior that the validation component works (because it correctly uses unit_type) while the calibration component fails to display any targets, giving the appearance of screen freezing. It is actually because the default value for these components under height unit setting for screen is 0.01. Since it's less than one pixel, nothing got displayed.

Fortunately the fix is simple because the unit_type got carried all the way into this function call, and we just need to set units=unit_type when using layout.Size(). Then everything works nicely and the calibration screen gets displayed.

mdcutone and others added 28 commits May 7, 2024 13:45
In a non-Builder context, the MovieStim's state is not updated, and
`.play()` would subsequently be called on every `.draw()` call.
Installed plugins were not being found on Linux/MacOS
BF: Check MovieStim's underlying player status when handling autoplay
…on-unit

Fixed eyetracker calibration target units (height -> pix)
…keys

BF: KeyboardComponent allowed keys from spreadsheet caused namespace errors
@mh105
Copy link
Contributor Author

mh105 commented May 15, 2024

Ah, looks like this was fixed by d62efd5 already, and we had the exact same fix :) Nevertheless, I think the ENH commit above is worth merging, since there's no need to keep the confusing Pix variables around when everything can be organized cleanly as arguments for the visual.TargetStim() constructor.

@peircej peircej requested a review from TEParsons May 16, 2024 13:13
@TEParsons
Copy link
Contributor

I think that code was added initially as a fix for Tobii eyetrackers, they use their own proprietary calibration code which needs to be in pixels. I'll test this branch with a Tobii next time I've got one to hand (we have one in the office), it may well be unnecessary following yours and @KirstenWilliams's other fixes but want to confirm before pulling in

@mh105
Copy link
Contributor Author

mh105 commented Jun 3, 2024

It is possible that the layout.Size and getattr() call to get the radius and innerRadius variables are still necessary. Closing this PR since the core fix of using unit_type is incorporated in release 2024.1.5.

@mh105 mh105 closed this Jun 3, 2024
@mh105 mh105 deleted the mousegaze_cali branch June 3, 2024 19:33
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.

None yet

6 participants