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

Make full screen optional #283

Merged

Conversation

JohnGriffiths
Copy link
Collaborator

Following idea from @pellet - this is a tiny PR to make the use of full screen optional.

This is particularly useful for debugging, because it allows to both see and switch to a terminal while the psychopy window is up, and allows to retain control via the terminal upon a crash, for e.g. to view the stack trace or enter a debugger.

This is done by providing a use_fullscr Bool argument in the experiment init, and setting it to a modifiable attribute of the Experiment object.

A modifiable attribute is also added for window_size, although this is not added as an input argument upon init.

I think it would probably be a good idea to move some other parameters such as the monitor type into modifiable object attributes in the future also.

Example usage:

from eegnb.experiments.visual_n170.n170 import VisualN170
expt = VisualN170(duration=10, use_fullscr = False)
exp.window_size = [1200,500]
exp.run()

@pellet
Copy link
Contributor

pellet commented Dec 3, 2024

Looks good @JohnGriffiths.

I previously achieved this by passing in the entire window object like so:
window: Union[visual.Window, rift.Rift] = None link - this allowed for the monitor or vr display to be initialized with the correct parameters upon creation.
Downside to this approach is that it is tightly coupled to PsychoPy and relies on the user to learn how to create the PsychoPy/PsychXR objects.

A third option could be to encapsulate the window settings in a new EEG-ExPy window class which could be passed in to the BaseExperiment constructor or accessed via exp.window.use_fullscr for example. Something similar could be done for the trial parameters to keep them all separately in one place and reduce the number of parameters passed directly into the BaseExperiment constructor. Downside would be needing to refactor the way the BaseExperiment is currently initialized in all of the actual Experiment sub-classes.

These are just some ideas for further iterations, I think your PR looks good to merge.

@@ -50,6 +50,8 @@ def __init__(self, exp_name, duration, eeg, save_fn, n_trials: int, iti: float,
self.soa = soa
self.jitter = jitter
self.use_vr = use_vr
self.use_fullscr = use_fullscr
self.window_size = [1600,800]
Copy link
Contributor

Choose a reason for hiding this comment

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

The size was previously [1600,900]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's fine.

@JohnGriffiths
Copy link
Collaborator Author

thanks Ben.

As discussed, I think these ideas are all good ones to consider as we go, in the context of concrete use cases and improvements.

For now the current PR is a very simple and concrete one, so let's keep it as-is.

Merging.

@JohnGriffiths JohnGriffiths merged commit 06b1843 into NeuroTechX:master Dec 5, 2024
4 of 6 checks passed
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.

3 participants