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 Grid-Aligned behavior to Playlab #17009

Merged
merged 8 commits into from Aug 14, 2017
Merged

Add Grid-Aligned behavior to Playlab #17009

merged 8 commits into from Aug 14, 2017

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Aug 10, 2017

For our BB-8 levels; this is primarily to fix the behavior where BB-8 turns to face the null direction (south) between each movement step:

forward

Behavior that is particularly funky when the solution involves using relative movement:

turning

To fix this, I added dedicated methods for turning BB-8 (and animating the turn) so that we can highlight and animate the block, and added a new behavior which we can apply when using these blocks so that BB-8 can avoid being faced south and being compelled to move, resulting in an animation that looks like this:

turningafter

@@ -5916,6 +5974,7 @@ Studio.moveSingle = function (opts) {
}

if (skin.gridAlignedMovement) {
sprite.setActivity(constants.BEHAVIOR_GRID_ALIGNED);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd love to solicit some opinions on whether or not this is the correct place to set this behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems good to me. It's actually weirder that I don't see a corresponding setActivity call in the else case.

Copy link
Contributor 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 the case that the else case expects BEHAVIOR_STOP, which is the default. If I'm correct then I could add that corresponding call for cleanliness, but if I'm not I have no idea what all could break

@Hamms Hamms changed the base branch from clean-sprite-overrides to staging August 11, 2017 00:30
Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Very nice!

sprite.addAction(new GridTurn(opts.dir, skin.slowExecutionFactor));
Studio.yieldGridAlignedTicks();
} else {
throw new TypeError("Studio.turnSingle is only valid in grid-aligned mode");
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: You could check this condition and throw as an early-out at the top of this method, which would make it more obvious that the rest of the method is a simple command list with no branches.

Studio.turnSingle = function (opts) {
  if (!skin.gridAlignedMovement) {
    throw new TypeError("Studio.turnSingle is only valid in grid-aligned mode");
  }

  const sprite = Studio.sprite[opts.spriteIndex];
  sprite.lastMove = Studio.tickCount;
  sprite.setActivity(constants.BEHAVIOR_GRID_ALIGNED);
  sprite.addAction(new GridTurn(opts.dir, skin.slowExecutionFactor));
  Studio.yieldGridAlignedTicks();
  Studio.lastMoveSingleDir = opts.dir;
};

@@ -5916,6 +5974,7 @@ Studio.moveSingle = function (opts) {
}

if (skin.gridAlignedMovement) {
sprite.setActivity(constants.BEHAVIOR_GRID_ALIGNED);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems good to me. It's actually weirder that I don't see a corresponding setActivity call in the else case.

let item;

describe('update', () => {
describe('if destination is set', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary nested describe blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sort of unnecessary, but it is the case that update can have some pretty different behavior based on when in the "lifecycle" of destination determination it's called, and I wanted to make absolutely clear which case we're in

@Hamms Hamms merged commit 67a5b2c into staging Aug 14, 2017
@Hamms Hamms deleted the add-grid-aligned-behavior branch August 22, 2017 23:31
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

3 participants