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

Add a 'defer' option to custom block args #22855

Merged
merged 9 commits into from Jun 7, 2018
Merged

Conversation

balderdash
Copy link
Contributor

@balderdash balderdash commented Jun 1, 2018

Given an event handler like:
image

Instead of generating code that just grabs the value of mySprite and other at the beginning of the program:

whenTouching(mySprite, other, function () {
  removeAllBehaviors(mySprite);
});

We can replace the arguments with getters that can return the current values of mySprite and other:

whenTouching(function () { return mySprite; }, function () { return other; }, function () {
  removeAllBehaviors(mySprite);
});

This is done by setting the defer option on the event block's arguments.

The draw loop can then call those getters every tick, so that when the value of mySprite changes, the event handler reacts accordingly.

@balderdash
Copy link
Contributor Author

image

Since this change means we rerun the input blocks every tick, something like this would create thousands of sprites per minute. Fortunately we're moving away from this type of block, so I don't think this will be an issue.

@balderdash
Copy link
Contributor Author

@balderdash balderdash changed the title Fix events on dynamically created sprites Add a 'defer' option to custom block args Jun 5, 2018
Ram Kandasamy added 2 commits June 5, 2018 15:48
I'm not sure why you'd want to, but no need to clutter the code
with this extra check.
@balderdash balderdash requested a review from joshlory June 6, 2018 02:47
@joshlory
Copy link
Contributor

joshlory commented Jun 6, 2018

Hmm, expected that these deferred accessor function are visible in the "Show Code" dialog?

whenTouching(function () {
  return mySprite;
}, function () {
  return other;
}, function () {
  });

@balderdash
Copy link
Contributor Author

It's expected, but that doesn't make it any less ugly.

An alternative would be to move those functions up by the variable declarations, so that it would look more like:

var mySprite;
function mySpriteGetter() {
  return mySprite;
}

...
whenTouching(mySpriteGetter, otherGetter, function() {
  ...
});

@joshlory
Copy link
Contributor

joshlory commented Jun 7, 2018

What does codegen look like when we eventually add where() selectors? Can the "symbol" selector be a special case, following the same general pattern?

Something like:

whenTouching({symbol: 'mySprite'}, {costume: 'bear'}, function () {
  // ...
}

@joshlory
Copy link
Contributor

joshlory commented Jun 7, 2018

Now that these are dynamic references, how are we handling the case where the ref changes?

image

Right now it looks like we snap to the newest-created sprite. Want to make sure that's an intentional decision, not just how it happens to work 😄.

@balderdash
Copy link
Contributor Author

What it would look like with defer is something like this:

whenTouching(function () {
  return mySprite;
}, function () {
  return where({costume: 'bear'});
}, function() {
  ...
});

This doesn't look as pretty, but defer is significantly more flexible. You can apply it to any input, so it could dynamically look up the value of any expression, not just sprite variables and where clauses. This lets us build blocks like:
image

@@ -11,7 +11,8 @@
"args": [
{
"name": "SPRITE",
"type": "Sprite"
"type": "Sprite",
"defer": true
Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that requiring Block Editors to add the defer attribute opens us up to possible bugs. In most cases it'll just work, but in specific cases the missing defer will cause errors that depend on the order of the blocks in the workspace.

Can we defend against this, or make it easier to understand when defer should be required? Should "Sprite" type args always have "defer": true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could make defer: true the default for event/eventLoop block inputs, and you'd have to explicitly set defer: false to turn it off. (This PR originally deferred all such inputs, there was no explicit defer option, but I thought it should be configurable).

Copy link
Contributor

Choose a reason for hiding this comment

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

When might you want the defer: false behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not super important, but if you have a field input (e.g. 'when {DIR} key pressed'), then deferring it would needlessly clutter the generated code.

@joshlory
Copy link
Contributor

joshlory commented Jun 7, 2018

What does the code generation look like for the "when" block above?

@balderdash
Copy link
Contributor Author

This is what the when block's codegen looks like:

when(function () {
  return score > 10;
}, function () {
  showTitleScreen('You win!', '');
});

Here's the block config and helper code too:

{
  "func": "when",
  "blockText": "when {CONDITION}",
  "args": [
    {
      "name": "CONDITION",
      "defer": true
    }
  ],
  "eventBlock": true
}
whenEvents = []
function when(condition, handler) {
  whenEvents.push({
    condition: condition,
    handler: handler
  });
}
forever(function () {
  whenEvents.forEach(function(event) {
    if (event.condition()) {
      event.handler();
    }
  });
});

Copy link
Contributor

@joshlory joshlory left a comment

Choose a reason for hiding this comment

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

LGTM to make the breaking change for June 11. We should revisit after the deadline to explore how we want to enable the where selector.

@balderdash balderdash merged commit eeb51da into staging Jun 7, 2018
@balderdash balderdash deleted the dynamic-events branch June 11, 2018 22:58
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

2 participants