Multiple whiskers #62

paulsonnentag · 2018-05-16T15:01:38Z

Summary API Changes

as discussed in original issue: #49

Creation / Removal of whiskers

Currently, the API is missing a way to remove whiskers again. I would like to add a whisker only if the paper has something connected to the output. The program can change audio nodes dynamically so it should be possible to remove whiskers again if there is no longer anything connected to the sound output.

var whisker = await paper.get('whisker', {
  direction,      // "up" (default), "down", "left", "right"
  whiskerLength,  // as fraction of the side (default 0.7)
  requiredData,   // array of data fields that must be present in the other paper
  paperNumber,    // paper number to do this for (default is own paper number)
});

// destroy
whisker.destroy();

Different whisker color

If a paper has multiple whiskers it would be nice if you could change the color of the whisker to differentiate them better. Instead of a color change the "connected" state is indicated by the moving dot animation. If the paper is not connected a static line is rendered without the moving dot.

var whisker = await paper.get('whisker', {
  direction: "up",
  color: "blue", // new color attribute (default: red)
  requiredData: [ "frequency" ]
});

Separate event handlers for placement and removal of papers

I think it would be cleaner to have two separate events for when a paper is placed and when it's removed. This makes it easier to explain the API. It's not necessary to check for null and in the removed paper event handler we can also pass in the information about the paper which was removed.

whisker.on("paperAdded" ({paperNumber, paperObj}) => { /* do something */ })
whisker.on("paperRemoved", ({paperNumber, paperObj}) => { /* do something */})

- allow to create multiple whiskers - whiskers can be removed - event listeners can be attached to whiskers - paperRemoved - paperAdded - whiskerMoved

paulsonnentag · 2018-05-16T15:04:31Z

client/paper/entry.js

@@ -119,6 +136,9 @@ import { fillQuadTex, fillTriTex } from './canvasUtils';
    callback,
    whiskerPointCallback,
  } = {}) => {
+    // eslint-disable-next-line no-console
+    console.warn('paper.whenPointsAt is deprecated, use the new whisker api instead');


I don't know how we should handle deprecation of features. I've kept the original whiskers for now, but long term I think we should remove paper.whenPointsAt to avoid confusion with the new whisker api.

Can we remove the code below and call get('whisker') when calling this? So we don't have any duplicate code at least?

The console.warn makes a lot of sense, let's keep that.

paulsonnentag · 2018-05-16T15:06:11Z

client/paper/whisker.js

+    ctx.lineTo(whiskerEnd.x, whiskerEnd.y);
+    ctx.stroke();
+
+    const dotFraction = Math.abs(Math.sin(Date.now() / 600));


I've changed the dot animation to alternate direction. I wanted to avoid implying a direction of the dataflow which depends on what the whisker does and instead just visualize connectedness.

paulsonnentag · 2018-05-16T15:08:24Z

client/paper/whisker.js

@@ -0,0 +1,171 @@
+import EventEmitter from 'events';


I've used the node EventEmitter because the browser event system adds unnecessary complication like event bubbling and dealing with Event objects instead of plain JS objects.

That makes sense, on the other hand this does add a non-standard API into Paper Programs. I think I would slightly prefer to use EventTarget anyway because it's an accepted browser standard. What do you think?

I can see your point. I would like to make Paper Programs more accessible to people who are new to programming and don't know the web platforms. In this case, I don't see a lot of benefits or more powerful things you could do with the native browser events other than being compliant with the standard.

Maybe we should also have a more general discussion what the future goals of Paper Programs are. In the future, I would like to explore different programming models to make it easier to build things with Paper Programs. I probably should write down my thoughts about this sometime.

Yeah that makes sense too. I'm mostly concerned about future compatibility, and bringing in concepts from Node feels a bit heavy in that respect. I don't have a super strong opinion though, so also happy to keep it this way.

And yeah, that would be useful! I don't really have goals for Paper Programs other than where you and other contributors want to take it. :)

janpaul123 · 2018-05-17T06:07:57Z

docs/api.md

+whisker.destroy()
+```
+
+*deprecated* old api:


Maybe just delete this entirely?

are you referring to the part below "deprecated old api:" ?

janpaul123 · 2018-05-17T06:10:02Z

client/paper/whisker.js

+    }
+
+    // render whisker with dot animation
+    ctx.clearRect(0, 0, ctx.width, ctx.height);


Won't this wipe out the other whiskers too?

good catch this does nothing, because ctx has no width and height

janpaul123 · 2018-05-17T06:11:31Z

client/paper/whisker.js

@@ -0,0 +1,171 @@
+import EventEmitter from 'events';
+
+export default class WhiskerFactory {


Not sure what the point is of this factory? The code might be simpler by just giving each whisker a unique identifier and getting a unique canvas for each whisker. Then you can keep everything contained in class Whisker. Doesn't matter all that much, just thinking it might be simpler.

I wanted to have just a single interval, but I agree that it would be simpler for each whisker to create its own interval. I'll change that.

janpaul123

Some small feedback here and there. Feel free to push back on points. 😄 Thanks for the PR!!

janpaul123 · 2018-05-17T16:03:06Z

Let me know when you're happy with the PR, then I'll merge it. 👍

paulsonnentag · 2018-05-23T13:46:15Z

Ready to merge

janpaul123 · 2018-05-28T04:02:28Z

Sorry for the late response 😞; and now it looks like there's a merge conflict. Let me know when it's resolved and I'll merge!

paulsonnentag · 2018-06-15T15:42:00Z

I've resolved the merge conflicts now

janpaul123

Awesome thanks!!

paulsonnentag added 3 commits May 15, 2018 11:50

Add new whisker api

f317194

- allow to create multiple whiskers - whiskers can be removed - event listeners can be attached to whiskers - paperRemoved - paperAdded - whiskerMoved

Add color attribute for whisker

6e1f93c

Add docs for new whisker api

2202eb8

paulsonnentag commented May 16, 2018

View reviewed changes

janpaul123 reviewed May 17, 2018

View reviewed changes

janpaul123 requested changes May 17, 2018

View reviewed changes

paulsonnentag force-pushed the multiple-whiskers branch from 79309bc to 785fb5f Compare May 23, 2018 13:43

Simplify implementation, reuse new api to remove old implementation

fdb3894

paulsonnentag force-pushed the multiple-whiskers branch from 785fb5f to fdb3894 Compare May 23, 2018 18:10

Merge branch 'master' into multiple-whiskers

4725e71

janpaul123 approved these changes Jun 15, 2018

View reviewed changes

janpaul123 merged commit 55bd3eb into janpaul123:master Jun 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple whiskers #62

Multiple whiskers #62

paulsonnentag commented May 16, 2018

paulsonnentag May 16, 2018

janpaul123 May 17, 2018

paulsonnentag May 16, 2018

paulsonnentag May 16, 2018

janpaul123 May 17, 2018

paulsonnentag May 17, 2018

janpaul123 May 17, 2018

janpaul123 May 17, 2018

paulsonnentag May 17, 2018

janpaul123 May 17, 2018

janpaul123 May 17, 2018

paulsonnentag May 17, 2018

janpaul123 May 17, 2018

paulsonnentag May 17, 2018

janpaul123 left a comment

janpaul123 commented May 17, 2018

paulsonnentag commented May 23, 2018

janpaul123 commented May 28, 2018

paulsonnentag commented Jun 15, 2018

janpaul123 left a comment

		@@ -0,0 +1,171 @@
		import EventEmitter from 'events';

		export default class WhiskerFactory {

Multiple whiskers #62

Multiple whiskers #62

Conversation

paulsonnentag commented May 16, 2018

Summary API Changes

Creation / Removal of whiskers

Different whisker color

Separate event handlers for placement and removal of papers

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

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

janpaul123 left a comment

Choose a reason for hiding this comment

janpaul123 commented May 17, 2018

paulsonnentag commented May 23, 2018

janpaul123 commented May 28, 2018

paulsonnentag commented Jun 15, 2018

janpaul123 left a comment

Choose a reason for hiding this comment