search

sugarlabs / memorize-activity

A memory game, not only to play but also to create you own games, for the Sugar learning environment
GNU General Public License v2.0
1 stars 15 forks source link

Port from speak/espeak shim to sugar3.speech #25

Closed nswarup14 closed 4 years ago

nswarup14 commented 4 years ago

Fixes #8

Changes 1) createcardpanel.py 2) face.py 3) speak/face.py 4) speak/voice.py 5) speak/espeak.py (deleted) 6) cardlist.py 7) activity.py 8) card.py 9) cardtable.py 10) widgets.py

Updates The activity used a shim speak/espeak to make use of the gst-plugin-espeak for voice synthesis i.e from text to sound. The sugar3 toolkit has this feature in sugar3.speech, hence the patch removes the use of the shim and uses sugar3.speech instead.

Testing Upon testing, the activity ran as expected and there were sounds as well.

Thanks!

quozl commented 4 years ago

Tested e8aed38 on Ubuntu 19.04. I heard appropriate sound, as soon as I asked for it. My test method was to click on the gear icon to Edit game, then click on a tile, then on the face speak toggle button. Or right-click and use the play button in the palette.

You can test the gst-espeak-plugin package like this;

gst-launch-1.0 espeak text="hello world" ! autoaudiosink

If you were using virtual machine instead of native testing, the virtualisation software may be negotiating access to the host sound device.

Reviewed as at e8aed38;

nswarup14 commented 4 years ago

@quozl Yes, I am using a VM to test my changes. The could explain the delay in the sounds. I shall test it according to the procedure you have described as well.

could it be that _peding should be _pending?

I'm not sure. The variable is not used anywhere else apart from speak/face.py. the update() method also does shed light on what the variable could be. I feel we can let it be as it is.

did these changes arise from some other patch in another activity?

No, I referred to my earlier work itself which is a closed PR. I did not use anyone else's work.

SpeechManager is instantiated twice; once should be plenty, as it has many system resources at play

So the SpeechManager is instantiated in two different modules, speak/face.py and speak/voice.py. In the latter, the SpeechManager is instantiated inside a module function that is used elsewhere in the activity. How can I eliminate one of the SpeechManager instances?

Thanks!

quozl commented 4 years ago

could it be that _peding should be _pending?

I'm not sure. The variable is not used anywhere else apart from speak/face.py. the update() method also does shed light on what the variable could be. I feel we can let it be as it is.

Perhaps you might look at which commit it arrived in? Use the blame view, or git blame, and review the commits responsible.

did these changes arise from some other patch in another activity?

No, I referred to my earlier work itself which is a closed PR. I did not use anyone else's work.

Okay, thanks. I remember doing espeak to sugar3.speech conversions last year, and wondered how similar the code would be.

SpeechManager is instantiated twice; once should be plenty, as it has many system resources at play

So the SpeechManager is instantiated in two different modules, speak/face.py and speak/voice.py. In the latter, the SpeechManager is instantiated inside a module function that is used elsewhere in the activity. How can I eliminate one of the SpeechManager instances?

Instantiate it only in one module, then use that instance by passing it to the other modules somehow. A suitable module might be the Activity class initialiser.

nswarup14 commented 4 years ago

@quozl The commit that introduces self._peding is https://github.com/sugarlabs/memorize-activity/commit/57c2df2c5617d8f6c65e66ad2980cda681181011. However, the commit does not shed light on whether the variable was named correctly or if it was supposed to be self._pending.

I have removed the two instances of SpeechManager and replaced it with just one, that is instantiated at the beginning of the activity and passed along to the modules that require it.

I was able to reproduce the sounds instantly this time, following the procedure outlined in comment. I edited my initial comment accordingly.

Thanks

quozl commented 4 years ago

Thanks. It was from Speak activity. https://github.com/sugarlabs/speak/commit/7a27c93ba07e707d21a8d35f73aa9733fb51eda8 From that context, it was a timing issue, and so "pending" seems likely. I think Aleksey did not have English as first language, and was mostly active between 2009 and 2012, with a brief resurgence in 2017.

quozl commented 4 years ago

https://github.com/sugarlabs/memorize-activity/pull/25/commits/5f54462bfdbd0598288fc5c6d69a672b9281b9dd is an invasive change, and adds complexity. I suggest you revert it, and instead make a speechmanager.py which holds a global variable which is returned by a get_speechmanager function. We've several examples of that design pattern in Sugar.

nswarup14 commented 4 years ago

@quozl @chimosky I have included sugar3.speech as discussed above in the comments. Please review.

quozl commented 4 years ago

Thanks.

Some review comments added to https://github.com/quozl/memorize-activity/commits/pr-25

I've not finished review, but that's how far I got in time available.

Please also rebase your pull request branch against master branch.

nswarup14 commented 4 years ago

@quozl I have rebased against master I reviewed you the commits you added to your fork and subsequently pull them to my branch.

I have added some more commits. Tested on Ubuntu 18.04 and Sugar v0.114. No errors in the logs and the activity runs alright.

Thanks

chimosky commented 4 years ago

Tested. Works as expected, still reviewing though.

Also it'll be great to change the play button icon to a pause icon while speech is playing.

quozl commented 4 years ago

Thanks. Tested 586b1d3.

It seems the purpose of right-click menu on face icon is to select the voice language, in turn so that memorisation games can be created for bilingual learning. Code already in voice.py is very complex, and I'm not sure why.

nswarup14 commented 4 years ago

@quozl

Code already in voice.py is very complex, and I'm not sure why.

I agree. I cant think of a way to simplify it. It uses regex to compare languages, which a complex task.

rhl-bthr commented 4 years ago

I cant think of a way to simplify it. It uses regex to compare languages, which a complex task.

If the code cannot be simplified, and if you are interested, then add docstrings and comments to explain the flow

quozl commented 4 years ago

The complex code in voice.py predates the simplification done for adding sugar3.speech. I agree with @pro-panda, a good way to prove if code is needed is to document it fully, as this informs you what it does in great detail. I've often written wonderful documentation which has let me discard both the code and documentation and replace it with something much simpler. Remember the value of the work is in the understanding and simplifying change without removing functionality ... and not in the preservation and enlargement of unnecessary code.

quozl commented 4 years ago

(I've not yet understood the code in voice.py, and we have an outstanding request for help from Paraguay, see mailing list post.)

nswarup14 commented 4 years ago

@quozl @chimosky I am trying to understand the code in voice.py and following @pro-panda 's suggestion, I shall add some documentation based on what I understand. I wanted to know how I can change my language from English to Spanish, so that I can reproduce the error as mentioned in the mailing list post I shall test with both master as well as with https://github.com/sugarlabs/memorize-activity/pull/25/commits/e71e3be42dec84409af16f8b3318809598aeb19e

Thanks.

quozl commented 4 years ago

Before starting the activity, set the LANG environment variable to es_PY.utf, see issue #26 title.

quozl commented 4 years ago

Ah, my mistake. I had reproduced the error reported by Aldo by installing a fresh Ubuntu virtual machine in Spanish language mode. Something during the installation sequence removes support for other languages, and I think this may be the cause of the error.

nswarup14 commented 4 years ago

@quozl Does this mean the issue is not with the activity but rather in the installation process?

quozl commented 4 years ago

No, it means it is an issue with multiple causes, and we must then choose which cause to fix and justify it. Do you really think that when Ubuntu is installed in Spanish that it should not delete English files? I think that would be difficult to justify.

nswarup14 commented 4 years ago

@quozl @chimosky How do I proceed with the PR and ensure it's fixed correctly? Thanks

quozl commented 4 years ago

We need a developer who can speak Spanish and English, and neither of us is that developer, so we'll leave that problem to them for now.