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

Allow primitives to have "cancel" behavior; cancel "play sound until done" on thread retire #2002

Closed

Conversation

towerofnix
Copy link
Contributor

@towerofnix towerofnix commented Feb 15, 2019

Resolves

Resolves scratchfoundation/scratch-audio#7. Closes #40 (as this PR provides an implementation).

Proposed Changes

At a high level, this PR allows classes such as Scratch3SoundBlocks to specify behavior that "cancels" an actively executing block. These functions are called when you click a script that is already running, and its active block is one of that opcode. The "play sound until done" block makes use of this; if you retire a thread where it is the active block, the sound it started will be stopped.

Internally, instances of BlockCached now keep track of their "cancel block" function alongside the main block function. This is referred to when a thread is retired; if present, it's called just like the normal block function.

Reason for Changes

When a script is executing, it is highlighted with a yellow "glow" bordering its outline. If you click a script while it's executing, it will lose this glow and stop running further blocks in the script. The implication, to the user, is that the script is stopped. Internally, this is what is happening. But, before this PR, "stopping" a thread did not also mean stopping active side-effects. Intuitively, side-effects should be stopped when a script is stopped; otherwise it does not seem so much like you are stopping the the script.

This PR doesn't (currently) implement "cancel" blocks into extensions, but it provides the basic code. Once it's implemented, blocks like "play note for beats" and "turn motor on for (N) seconds" can be changed so that their side-effects are cancelled as soon as the thread is retired.

Test Coverage

I tested this manually. I'm not certain if unit tests are wanted here? I saw that there don't seem to be any for execute.js so far, only the higher-level classes (e.g. Thread, Sequencer); this PR doesn't add much behavior to those classes.

@thisandagain
Copy link
Contributor

thisandagain commented Feb 25, 2019

@kchadha @ericrosenbaum This may be a good candidate for "co-review". :-)

@towerofnix
Copy link
Contributor Author

This also closes #706, since retireThread and so also the cancel functions of blocks, get called when the stop sign is clicked.

Fwiw, this doesn't implement stopping the motor as discussed in #706 and #40; it only adds the support for that feature. It's probably worth implementing stopping the motor in this PR, but I don't have a lego motor or any external devices to do that.

Implementing motor-stopping into this would mean adding support for this to extensions (since the motor blocks are all part of extensions), which is a good idea anyway since that looks to be the direction Scratch 3.0 is going now.

@thisandagain thisandagain modified the milestones: March 2019, April 2019 Apr 17, 2019
@thisandagain
Copy link
Contributor

thisandagain commented Apr 17, 2019

@ericrosenbaum @kchadha Bump

@ericrosenbaum
Copy link
Contributor

@towerofnix sorry it took us a long time to look at this! @kchadha and I were just looking into it, and we realized that Scratch 1 and 2 also did not stop the sound for "play sound until done" when you click the stack to stop it, so that specific thing feels less important than I originally thought when I filed that issue (scratchfoundation/scratch-audio#7). This would be a nice feature though, especially, as you point out, for the extensions that make sound or control motors. We'll investigate the implications of the change a bit further.

@ericrosenbaum
Copy link
Contributor

After some more discussion, we decided to close this PR, and revisit in the future if needed for internal design and implementation.

@towerofnix
Copy link
Contributor Author

@ericrosenbaum alright. Since I spent a while on this, even if it's not help-wanted and I didn't particularly expect it to get merged, I'd appreciate just a summary of thoughts on this PR shared here?

@towerofnix
Copy link
Contributor Author

Ping @ericrosenbaum?

@ericrosenbaum
Copy link
Contributor

Sorry I didn't provide a more detailed response. We appreciate your work on this. Our main concern is the risk of introducing bugs, due to this being a fairly complex feature. We realized that because previous versions of scratch do not have this feature, it's not itself a bug fix but a a new feature. So we need to decide as a group whether we want to add it (which we have decided to hold off on for now). If we do decide to add it in the future, due to the complexity, we'll want to do our own design and implementation of the feature within the team. Your implementation in that case would serve as a useful reference. Hope that helps!

@towerofnix
Copy link
Contributor Author

Alright, I understand! Thanks for the explanation.

@ELing-Lonely
Copy link

@towerofnix 您好,想请问这个pr当时是在scratch-vm 的那个版本基础上使用的,我可以知道吗?

@towerofnix
Copy link
Contributor Author

@ELing-Lonely Please see my email reply, or read it below:

Hello,

I created this pull request in February 2019. That is very shortly after Scratch 3.0 was first released to the public (January 2019) and since then, it has been 6 years. I do believe the pull request was based on the version of scratch-vm which, at the time, was current. I have not tested the code in any more recent version of scratch-vm. Unfortunately that means I cannot tell you if it would require only minor adaptations, or require more serious ones.

However, during those six years, Scratch has not changed very much. It looks like the VM file “src/engine/execute.js” is still basically the same as it used to be. Therefore, I expect changes would be pretty minimal. You may even be able to use “git cherry-pick” or “git rebase” without much trouble. I have not tried this out myself.

If you are interested, I do recommend trying it yourself. May I ask why you were wondering? Are you creating a modified version of Scratch which you are able to share about?

And it's no bother at all! Thank you for reaching out.

@ELing-Lonely
Copy link

@towerofnix
您好
我并没有创建一个修改版本的Scratch的打算,只是在学习使用Scratch-vm 时发现,停止功能没有停止一切的能力,查看了issues ,发现您在2019年提出的pr,可以解决此问题,但社区开发者好像没有采用,但我觉得可以解决我的问题所以有此一问,在此在次表示感谢。谢谢您的回信。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants