API updates to allow for custom frame additions #24

caleybrock · 2017-01-25T23:57:18Z

Used by: code-dot-org/code-dot-org#12866

PiskelApi changes

loadAdditionalFrames - used to append an animation to an existing animation
addBlankFrame - used to add a blank frame
onAddFrame - register a callback for when the user clicks "Add new frame" button

I also disabled adding a blank frame when the user clicks add new frame and instead created an event that gets fired so that gamelab can control better what happens. I made a comment about this inline.

caleybrock · 2017-01-27T19:13:03Z

src/js/controller/FramesListController.js

+      $.publish(Events.ADD_NEW_FRAME_CLICKED);
+      // Disable adding a blank frame and idead use the PiskelApi
+      // to cause an action.
+      //this.piskelController.addFrame();


This means that code-dot-org/piskel isn't super usable unless it's connected with gamelab. Does this matter?

I think it's fine for now - we already have a set of changes we don't merge upstream. 👍 leaving comment in so we know in the future that this change is intentional and should be preserved.

A couple of alternatives:

This could do the old behavior if no custom newFrameClicked handler has been attached.

We could do more refactoring here to make the behavior more configurable - perhaps have a default controller that normally listens for and handles ADD_NEW_FRAME_CLICKED and then skip loading that controller and have our own controller handle it in our particular use case.

Both are probably overkill for now though.

The more I think about this, the more I'd like to see it be more flexible; as if the service should offer each attached client an opportunity to handle "new frame" and proceed to its own logic if all clients refuse. The 'old behavior if no custom handler' version is probably the MVP for this, and would be cheap-ish to implement now. Thoughts?

caleybrock · 2017-01-27T19:18:21Z

src/js/service/ImportService.js

+   * @param {function} [onComplete]
+   */
+  ns.ImportService.prototype.importFramesFromImage = function (image, options, onComplete) {
+    onComplete = onComplete || Constants.EMPTY_FUNCTION;


This logic is mostly borrowed from ns.ImportService.prototype.newPiskelFromImage
Wasn't sure, since we try not to make piskel too hard to upgrade in the future, if I should refactor out some of the logic, or just copy and add my own new function. In the past I've been trying to only add code to piskel, in hopes that merging might be easier.

ImportService.js was added by us - refactor away. I'd strongly consider submitting a generic version of this change upstream too.

caleybrock · 2017-01-27T23:04:33Z

@islemaster you're welcome to take a look at these two PRs too. Added Brent as a reviewer, since we spent a bunch of time discussing, but you probably have more context within Piskel.

islemaster · 2017-01-27T23:26:10Z

src/js/PiskelApi.js

+    ADD_NEW_FRAME_CLICKED: 'ADD_NEW_FRAME_CLICKED',
+
+    // Add a set of new frames to the current piskel
+    // Arguments: uri, frameSizeX, frameSizeY, frameRate


Does frameRate make sense as an argument for this event? How does it interact with the existing animation frameRate?

islemaster · 2017-01-27T23:27:24Z

src/js/PiskelApi.js

@@ -133,6 +145,50 @@ var PiskelApi = (function (module) {
  };

  /**
+   * Tell Piskel to load an spritesheet for editing, merging with whatever document


s/an/a, and maybe 'appending to' would be more specific than 'merging with'.

islemaster · 2017-01-27T23:36:25Z

src/js/service/ImportService.js

+          frameSizeY, smoothing);
+      var mergedPiskel = this.mergePiskels(this.piskelController_.getPiskel(), piskel);
+      this.piskelController_.setPiskel(mergedPiskel);
+      this.previewController_.setFPS(frameRate);


Looks like the new framerate clobbers the old framerate - and from Piskel's point of view I suppose this is fine since it's only a preview setting. Still, it seems like an odd choice for this API that adds frames to an existing animation.

islemaster · 2017-01-27T23:37:15Z

src/js/service/ImportService.js

+      var piskel = this.createPiskelFromImages_(frameImages, frameSizeX,
+          frameSizeY, smoothing);
+      var mergedPiskel = this.mergePiskels(this.piskelController_.getPiskel(), piskel);
+      this.piskelController_.setPiskel(mergedPiskel);


I think we've had undo issues in the past with operations that set a new piskel (like the built-in resize tools) - does that affect us here?

I'm able to upload a different size frame, change other things, then undo to before I added the frame and re-sized so I think it's okay. I'll keep an eye out though.

islemaster · 2017-01-27T23:39:03Z

src/js/service/ImportService.js

+          setPiskelFromFrameImages(frameImages);
+        }
+        onComplete();
+      }.bind(this)


Agreed, extracting an async getFrameImagesFromSpriteSheet would be much cleaner here.

Bjvanminnen · 2017-01-27T23:46:11Z

src/js/PiskelApi.js

+      this.removeCallback_(PiskelApi.MessageType.FRAMES_LOADED, callback);
+      onComplete();
+    }.bind(this);
+    this.addCallback_(PiskelApi.MessageType.FRAMES_LOADED, callback);


It looks like this pattern is used in a couple places (add a callback that gets called once, then removed). I wonder if it's worth adding a helper for it, so that you can just do something like this.callBackOnce(FRAMES_LOADED, callback)

islemaster · 2017-01-27T23:46:12Z

src/js/service/PiskelApiService.js

+   * Add a blank frame to the end of the current piskel.
+   */
+  ns.PiskelApiService.prototype.addBlankFrame = function () {
+    this.piskelController_.addFrame();


I think it'd be fine to inline this method above.

islemaster

Nice work! I think a little refactoring would be good, but overall structure is great.

Bjvanminnen · 2017-01-27T23:52:19Z

src/js/service/PiskelApiService.js

@@ -178,6 +185,41 @@
  };

  /**
+   * @param {!string} uri


Might be worth a function level comment saying what it means to load frames? (i.e. does it essentially just mean to download the frames?)

I prefer append to loadAdditional here, would recommend a rename.

caleybrock · 2017-01-30T23:03:37Z

I think pushing this upstream will be my tech debt item for the sprint, which will include looking at the best way to handle how flexible "adding a frame" should be.

It looks like almost none of this is currently is tested, including the PiskelApi and ImportService. For my last PR I added tests for the function that merges piskels, but that's it. Suggestions on what is reasonable to go about testing here?

caleybrock force-pushed the add-custom-frame branch 2 times, most recently from 32a2301 to bf74f50 Compare January 26, 2017 00:27

Custom frame event works, but saving does not

d2c1ff2

caleybrock force-pushed the add-custom-frame branch from bf74f50 to d2c1ff2 Compare January 26, 2017 00:36

Add blank frame to piskel api

a19af76

caleybrock force-pushed the add-custom-frame branch 3 times, most recently from a016c53 to fcff069 Compare January 27, 2017 19:05

Fix uploading callbacks

53d91b6

caleybrock force-pushed the add-custom-frame branch from fcff069 to 53d91b6 Compare January 27, 2017 19:06

caleybrock commented Jan 27, 2017

View reviewed changes

caleybrock mentioned this pull request Jan 27, 2017

Add custom animation frames to existing animations in Gamelab code-dot-org/code-dot-org#12866

Merged

caleybrock commented Jan 27, 2017

View reviewed changes

caleybrock changed the title ~~WIP: Add custom frame~~ API updates to allow for custom frame additions Jan 27, 2017

caleybrock requested a review from Bjvanminnen January 27, 2017 23:01

islemaster reviewed Jan 27, 2017

View reviewed changes

Bjvanminnen reviewed Jan 27, 2017

View reviewed changes

islemaster reviewed Jan 27, 2017

View reviewed changes

islemaster approved these changes Jan 27, 2017

View reviewed changes

Bjvanminnen reviewed Jan 27, 2017

View reviewed changes

Code review refactoring for adding frames in piskel

7dada8e

caleybrock force-pushed the add-custom-frame branch from 4457fa1 to 7dada8e Compare January 30, 2017 22:59

caleybrock merged commit af834f1 into master Feb 1, 2017

caleybrock deleted the add-custom-frame branch February 1, 2017 00:48

molly-moen mentioned this pull request Jun 20, 2023

Fix animation blinking issue #58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API updates to allow for custom frame additions #24

API updates to allow for custom frame additions #24

caleybrock commented Jan 25, 2017 •

edited

Loading

caleybrock Jan 27, 2017

islemaster Jan 27, 2017

islemaster Jan 27, 2017

islemaster Jan 27, 2017

caleybrock Jan 27, 2017

islemaster Jan 27, 2017

caleybrock commented Jan 27, 2017

islemaster Jan 27, 2017

islemaster Jan 27, 2017

islemaster Jan 27, 2017

islemaster Jan 27, 2017

caleybrock Jan 30, 2017

islemaster Jan 27, 2017

Bjvanminnen Jan 27, 2017

islemaster Jan 27, 2017

islemaster left a comment

Bjvanminnen Jan 27, 2017

islemaster Jan 27, 2017

caleybrock commented Jan 30, 2017

API updates to allow for custom frame additions #24

API updates to allow for custom frame additions #24

Conversation

caleybrock commented Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleybrock commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleybrock commented Jan 30, 2017

caleybrock commented Jan 25, 2017 •

edited

Loading