Open JobLeonard opened 7 years ago
So I figured I'd give this a try myself. First, I made a component wrapper for p5, which works fine:
https://github.com/JobLeonard/p5-react
Then I tried turning that into an idyll custom component:
https://github.com/JobLeonard/idyll-p5
However, it refuses to compile. I get a ReferenceError: window is not defined
. p5js heavily relies on the global window (as does my own code, but I could fix that).
Does this have something to do with the server-side rendering?
I love the idea of having a p5 component to allow users to easily embed processing sketches.
That error seems like an SSR issue, you can disable SSR by running idyll with the option to turn that off (idyll -r
). I'm happy to take a look at the code too but I'm getting a 404 on the idyll-p5
component link.
I agree that having some more facilities to track resizes in the components makes a lot of sense, especially for a lower level canvas component. I'm still thinking over the API for that. The list of requirements that you give above seem like completely reasonable things that anyone using canvas in this setting would want.
Ah, my bad, I forgot to set the repository to public!
Also, I just got it to work with that tip!
Give it a look, I'm sure it's still pretty rough around the edges and in need of clean-up
Oh cool - this is great work. I have a couple ideas to streamline the way that you can inject idyll variables into the sketchFunc
, can send those as a PR.
For some reason cloning that repo locally and running npm start
has the width locked to 1200px
, even on window resize
Edit - this looks like it has something to do with retina / pixel ratio, i'm taking a look at this now
I think you're right: I'm using my own logic to compensate for pixeldensity, but p5 also has it - which makes sense, because it wants to have cross-compatible sketches with as few headaches as possible. So the result is that it applies higher pixel densities twice. Easy enough to fix, but there are two options:
I think the principle of least surprise applies here, so I'll default to scaling the pixels, but allow for passing a "crisp" flag to enable the former.
EDIT: `devicePixelRatio is passed now in case someone wants to apply their own logic, but it really has no need for a crisp flag.
I have a couple ideas to streamline the way that you can inject idyll variables into the sketchFunc, can send those as a PR.
Yes please! On a related note, I just tried building and uploading the code as-is to github pages. It seems that the minificaition breaks the passing-props functionality, so hopefully your approach will fix that too.
I'm trying to add support for the updateProps
method mentioned over here, but it seems that you sit on a throne of lies there is no such method - I can't find it in the IdyllComponent if I look at the source code either. Was that feature removed and is the documentation out of date?
😂
BTW, I got WebGL mode working too :)
So at this point the main things to add would be:
updateProps
support (assuming that is still the method used by Idyll, and otherwise use whatever replaced it)npm run build
.😀
I've pushed some updates to my fork, see a live version on GH pages: https://mathisonian.github.io/idyll-p5/index.html
Things I've updated:
updateProps
support. We recently changed the way this is implemented so it gets injected on the component - a side effect of this is that it only happens on the outer component that gets returned, so I had to manually pass it down.Detecting when it enters the viewport shouldn't be too bad, since it extends Idyll component it accepts the onEnterView
prop. One easy way to implement this would be to give the SketchComponent
a default value for the onEnterView
.
Fixes window checks, so that SSR still works
Awesome, thanks
We recently changed the way this is implemented so it gets injected on the component
Thanks, I really had no idea what I was doing wrong
WIP - updating the way Idyll variables are referenced so they are available without passing a separate object. This is still a little broken.
I guess this is why you took out the "string with a Function object" approach, and replaced it with an expression that returns an arrow function? What is still broken about it, btw? Don't expressions always have access to all defined variables?
I also noticed that you replaced the large number of parameters with a single options object
sketch(p5, { width, height, devicePixelRatio: window.devicePixelRatio, updates: updateProps});
Makes sense, saves typing.
ASIDE:
class Matt extends IdyllComponent
Is this how we are going to communicate in-code? Because I'm down for that. I'll make my own Job component to match. I'll go with green :P
I pushed a few updates to fix the variables, and left one note for something that is not ideal with the updateProps
, but I think we can fix on the Idyll end.
https://mathisonian.github.io/idyll-p5/index.html
One other issue is that when you initialize a variable in Idyll it only evaluates that initial expression once, so
[var name:"mySketch" value:`() => {/*...*/}`]
[Sketch sketch:mySketch]
wont have Idyll variables respond to sliders, but this code will:
[Sketch sketch:`() => {/*...*/}`]
I'm thinking through if this is desired behavior, but for now if you really need a variable that updates like you'd have to use a derived
variable.
It also seems like code minification is the reason why you were seeing certain things not work during deployment. I turned it off for now, but am investigating why that would be.
The minification sounds like a bug to me, hope it's a trivial one.
So let's think the injecting of idyll variables through for a bit, because we also have p5
's internal state to keep in mind and stay synchronised with.
Unless I'm mistaken, lines 74-70 of sketch.js
currently don't actually do anything:
componentWillReceiveProps(nextProps) {
// pass relevant props to sketch
const { sketch, width, height } = this.state;
let { webGL, noCanvas, ratio, updateProps } = nextProps;
nextProps.sketch(sketch, { width, height, devicePixelRatio: window.devicePixelRatio, updateProps: updateProps});
}
They miss the whole wrapping setup
and unmount
functions done in lines 43 to 66 of the mountedContainer
function, and there is no new p5(_sketch, div)
, which is necessary to actually open the sketch.
Another important thing to note is that a p5
sketch is not functional: it's a stateful, mutating object. If I create a canvas through it, and change the sketch code, the only thing to do is remove the old sketch (and canvas) and open the new one. Which also would require special handling code (basically, calling sketch.unmount()
and then opening the new one). It's actually kinda easy, btw: just add some special watchedVal
code in the wrapping Sketch
component. I'll write it later.
But that raises another problem with generating the sketch as a derived variable, closing over the idyll variables: it creates new sketch code every time one of those variables changes. Causing a reset. We don't want that. (unless I misunderstand how derived variables work, of course)
So with that in mind we really have to:
Which (sadly) brings us back to the earlier receiveProps
approach. I think I might have a way to do it in a less clunky way, and I'll rename the methods to be more in line with how Idyll works, instead of using React terms.
The new version of your updatedProps example would look like this:
[var name:"clickBgColor" value:0 /]
[var name:"clickLineColor" value:255 /]
[Sketch
vars:`{ clickBgColor, clickLineColor }`
sketch:`(p5, { width, height, updateVars }, vars) => {
let size = 25;
p5.draw = () => {
p5.fill(vars.clickBgColor, 16);
p5.noStroke();
p5.rect(0, 0, width, height);
size = 40 + 10*p5.sin(p5.frameCount * p5.PI / 60);
p5.stroke(vars.clickLineColor);
p5.strokeWeight(size);
p5.line(p5.mouseX, p5.mouseY, p5.pmouseX, p5.pmouseY);
};
p5.mouseClicked = () => {
updateVars({
clickBgColor: 255 - vars.clickBgColor,
clickLineColor: 255 - vars.clickLineColor
});
}
}` /]
(note that I renamed updateProps
to updateVars
, to make the connection more explicit; we can also go the other way and rename vars
as props
, which would have the silly side-effect adding a props.props
in the React code)
In SketchComponent
, we first attach an extra function to the p5
object to handle receiving new vars:
p5._updatedVars = (_vars) => {
Object.assign(vars, _vars);
}
and we also modify componentWillReceiveProps
like so:
componentWillReceiveProps(nextProps) {
if (nextProps.vars){
// pass relevant props to sketch
const { sketch } = this.state;
sketch._updatedVars(nextProps.vars);
}
}
If I understand the way derived variables work correctly, this will not trigger compiling a new sketch whenever a passed variable changes, because it they are not directly referenced in the sketch so Idyll doesn't see it. However, it will trigger a new passing of a vars
prop, which is then handled automatically behind the senes.
Admittedly, is not as pretty as passing the variables and being defined in the sketch immediately: we have to set a separate vars
parameter in the Sketch component, and access the variables through a vars
object in the sketch.
Another downside would be that if someone accidentally writes clickBgColor
instead of vars.clickBgColor
, the code compiles just fine and we get our earlier derived-sketch-definition problem again, which probably will confuse a few people out there. In that sense, the string-based approach might have been a bit safer, if inefficient, since it would give a compiler error.
What would fix both of these issues is if we find a way to destructure an update (without asking the user to write this explicitly herself). We can already do so in the sketch definition:
(p5, { width, height, updateProps }, { clickBgColor, clickLineColor }) => { /* ... */ }
.. but the problem is that I don't see how to make that work with updating clickBgColor
and clickLineColor
through _updatedVars
, since we're outside of the scope of the sketch function there.
Still, this is already better than the previous receivedProps
approach, and I also don't really see how else to handle the problem of not wanting to trigger a sketch reset every time. Maybe you see another way of making this more elegant, but this seems like a decent compromise to me.
Of course, we'll have to test if this actually works in the first place.
@JobLeonard I think there are some other important points here that I still need to parse, but wanted to point out -
componentWillReceiveProps(nextProps) {
// pass relevant props to sketch
const { sketch, width, height } = this.state;
let { webGL, noCanvas, ratio, updateProps } = nextProps;
nextProps.sketch(sketch, { width, height, devicePixelRatio: window.devicePixelRatio, updateProps: updateProps});
}
if the nextProps.sketch
function defines a draw
function, this will essentially cause it to get swapped out e.g.
runningSketch.draw = newDrawFunction() {}
and this seems to work okay, on the next tick the sketch starts using the new draw function. That example with the two sliders is an example of this concept working
Oh, right, I see it now. I guess what confused me was my own choice to save the p5
object as sketch
instead of p5
.
There's still two ways this can break though:
setup()
runs again, which it won't until there is a resetunmount()
isn't wrapped to call p5.remove()
, so in case of unmounting we might end up with many running instances of p5 sketches (p5 uses a draw-loop, meaning it calls basically calls itself using window.requestAnimationFrame()
)Okay, that makes sense to me, in which case the solution you proposed above probably makes the most sense. I'll see if I can't get something like that working in the next few days.
I just realised there is another problem, see comments in sketch below:
(p5, { width, height, updateProps }) => {
// because this sketch is derived each time,
// frame is reset each time. This is not
// expected behaviour (from p5 users POV).
let frame = 0;
p5.draw = () => {
p5.noStroke();
p5.fill(clickBgColor, 16);
p5.rect(0, 0, width, height);
p5.fill(clickLineColor);
let size = 300 - 300*p5.cos(frame * p5.PI / 240);
p5.ellipse(width/2, height/2, size, size);
frame++;
};
p5.mouseClicked = () => {
// could not use clickBgColor and clickLineColor
// without breaking both sketches. Is this
// expected behaviour?
updateProps({
clickBgColor2: 255 - clickBgColor2,
clickLineColor2: 255 - clickLineColor2
});
}
}
I pulled in your changes and added the example, see here: https://jobleonard.github.io/idyll-p5/
Ok, even freakier: I added clickBgColor2
and clickLineColor2
because re-using the same variable made neither sketch work, but now click either sketch inverts the other, even though updateProps
is updating different variables in each sketch!
And it looks like I can click anywhere on the page and trigger a mouseClicked in any sketch. But it doesn't always have that problem.
Okay - i think a couple weird things happening -
In the second sketch you're referencing clickBgColor
where it should be clickBgColor2
, causing the two sketches to always show the same thing. Compounding this is the fact that it seems like processing is catching click events anywhere on the page. Point taken though, it definitely isn't as easy as just re-initializing the sketch code.
Oops, I forgot to update all the variables, you're right. The false mouseClicked
trigger also means updateProps
is called twice, which turns out to be the reason it didn't work before. wrapping mouseClicked
in a function that only triggers when inside the canvas fixes the problem.
I just tested mouseMoved
and it seems like we also need to add this check for every other mouse. I'll test out keyboard and touch events next.
This doesn't handle the sketch-reinitiation problem yet, but regardless of how we'll handle that we need to handle this too.
Here's what the code looks like: https://github.com/JobLeonard/idyll-p5/blob/5ecc32a505b3bc5ef62fbe5721efc21debf617cc/components/sketch.js#[L109-L184]
EDIT: I tried using hasFocus
and p5.focus but neither worked.
Keyboard events added in my branch, see https://jobleonard.github.io/idyll-p5/ for a simple demo.
By default, p5 sketches need to be clicked/tapped on firs before gaining "focus" and listening to to mouse- and keyboard-events. If you want it to always be listening to these events, pass an alwaysListening:1
to the Sketch.
Added first attempt at touch events, although they don't quite work as they're supposed to yet. Not sure what's breaking them.
The bigger issue right now is that all of these sketches are always running and listening. This really, really slows down the page when I'm viewing it on mobile.
Ah okay - that makes sense, and looks like awesome progress. I'm busy finishing a project up at work this week, but will take a stab at the props and viewport logic this weekend.
Great, hope you'll make some headway! I'm happy to join in again once you get the basics working, that'll make it easier for me to figure out how to make it work.
There's also the question of derived variables resetting the sketch - I'm almost tempted to suggest going back to the old compiled-from-string approach I started with.
Hey, I see that you've been really busy with other things. Just a quick question: would it be worthwhile to open a separate issue to discuss how to handle how Idyll handles derived variables and interaction with the rest of the document from within closures? Since that might be a more general issue than P5js's way of handling instance mode.
Hey - yes I haven't forgotten about this! Sorry for the slowness, but I want to make sure we get some other things cleaned up first.
I think that we should use this component as a guiding use case for these types of parameters. Its the first time that this has shown up (because in most other cases the component isn't responsible for so much logic). Let's get this working with the current implementation and if we can see a way forward that would make the implementation of this component way cleaner / way easier to write against then open specific issues for that, because I'm sure there are other libraries that behave in a similar way to p5 that people may want to use.
Hey, just a quick message to say that I haven't forgotten about this. I've just been too busy to work on it.
For the last months I have been focused getting the data browser I am working on ready for a public beta. It was that browser that I originally wrote the Canvas and Remount components for.
In case you're curious, you can see it in action here:
http://loom.linnarssonlab.org/
And the repo with the complete and very messy code here:
https://github.com/linnarsson-lab/loom-viewer
Anyway, I've since changed a few things in how the component draws things, the most important of which is that it's all done asynchronously, to prevent the browser from freezing when drawing on hundreds of canvas components:
https://github.com/linnarsson-lab/loom-viewer/blob/master/client/components/canvas.js https://github.com/linnarsson-lab/loom-viewer/blob/master/client/plotters/async-painter.js
I think that around the Christmas break I'll have time to update the equivalent idyll code to make use of this too :)
As mentioned in the other issue and on HN: I have been working with custom plots on canvas in React, not knowing about this project.
Maybe some of the ideas in my approach are useful, so I thought I'd share. With a bit of work, this might be turned into a blank canvas that is relatively easy to write custom drawing code for - which may be simpler than writing a custom React component.
In my use-case, the canvas is a static image that only reacts to changes in passed props, window sizes, or self-contained animations with
requestAnimationFrame
, but with a few changes this also work for more interactive stuff.(Aside: in most contexts, the approach in idyll's regl example probably works better (one fixed position full-screen behind-everything-else canvas). However, I'm dealing with scientists who often print out websites to annotate them, so having separate canvases with their own renders is a more sensible solution for me)
I needed a canvas component that:
div
to pixel-perfectionThe only thing that seems to work consistently across browsers is mounting the canvas after everything else. Basically, first mount the
divs
that will contain the canvas, using CSS to make them the appropriate size, and then after that has happened, mount the canvas at the size matching the div:There's a bit more to it than that, but that is the basic idea.
On top of that, the component is wrapped in a higher-order component that detects when the screen resizes, replacing the old canvas.
To draw on it, I pass a
paint
function as a propThe actual painting code is simply a function takes a drawing context as an argument, and knows the sizes and scales of the pixels. Which lets me more-or-less decouple the logic from the
canvas
element entirely.BTW, canvas interactivity like detecting the mouse would require a more complicated API, perhaps similar to p5js' instance mode. In fact, it's probably even easier to just wrap p5js entirely, which could make writing embedded p5js sketches relatively easy.