open-cogsci / OpenSesame

Graphical experiment builder for the social sciences
http://osdoc.cogsci.nl/
GNU General Public License v3.0
234 stars 111 forks source link

Update psycho.py #784

Closed egaudrain closed 1 year ago

egaudrain commented 1 year ago

PsychoPy made changes to their Sound object: https://github.com/psychopy/psychopy/commit/50a730b7be0bb1a219d5666d657bad4c8cc121d1. The status property has been removed, and replaced with an isPlaying property. The proposed change covers both cases depending on the version of PsychoPy that is being used.

Note that on Linux with Python 3.10, I had to upgrade to some more recent version of PsychoPy to get OpenSesame to work. But I noticed that the Sampler item was not properly waiting for the sound to be finished like it should... and I believe this is where it is coming from. I haven't tried to patch my version of OpenSesame to see if that solves the issue though.

smathot commented 1 year ago

Thanks for this!

I haven't tried to patch my version of OpenSesame to see if that solves the issue though.

Did you test the patch at all?

egaudrain commented 1 year ago

I tried it now and I confirm it solves the sound issue with (a patched version of) PsychoPy 2023.1.0.

In the long run, I don't know if this is the most elegant solution. I don't know what is the most efficient way to write version specific code in wrappers. Since isPlaying() may be called a lot, it might be better to specify the function once and for all when the object is created instead of checking if the attribute exists every time we need it:

class Psycho(Sampler):
   #[...]

    def __init__(self, experiment, src, **playback_args):
       #[...]
        self._sound = Sound(
                self._data,
                sampleRate=self._samplerate,
                **kwargs
            )
        if hasattr(self._sound, 'isPlaying'):
            self.isPlaying = self._isPlaying
        else:
            # For PsychoPy version before commit 50a730b
            self.isPlaying = self._isPlaying_pre50a730b

    #[...]

    def _isPlaying(self):
        return self._sound.isPlaying

    def _isPlaying_pre50a730b(self):
        return self._sound.status == PLAYING

    def wait(self):
        while self.isPlaying():
            self._keyboard.flush()

That seems to work too (although the sound sometimes breaks up... even with relatively long buffers, but not sure it is coming from this yet...).

Also note that, at the moment, psychopy itself needs patching to work properly because this commit https://github.com/psychopy/psychopy/commit/45ed546b8e0a25ddb87156ef400687aaf31baf39 broke the possibility to pass numpy arrays as value for Sound().

smathot commented 1 year ago

Thanks for this :+1: I implemented a fix very similar to yours. In the process, I also came across the same issue that you also did:

For now I implemented a hack in OpenSesame to work around this. As soon as it's resolved upstream I'll probably remove that hack again (although it's harmless).