Skip to content

Conversation

@vfeskov
Copy link
Contributor

@vfeskov vfeskov commented Nov 9, 2017

Closes #32

DOM source object exposes element() stream, which emits the DOM element as soon as it appears, so if you combine it with canvas rendering instructions you don't get any delay.

Using DOM source to access canvas element is pretty flexible too, there's no need to invent new configuration to support events or additional attributes:

const drivers = {
  DOM: makeDOMDriver('#root'),
  Canvas: canvasDriver,
  viewport: ...
}

function main (sources) {
  // dynamic width and height of the canvas element
  const vdom$ = sources.viewport.size$.map(({width, height}) => 
    <div>
      <canvas width={width} height={height}></canvas>
    </div>
  )

  // event handling of the canvas element
  const canvas = sources.DOM.select('canvas')
  const counter$ = canvas.events('click').startWith(0).scan(counter => counter + 1)
  const renderingInstructions$ = counter$.map(counter => rect({children: [text({x: 15, y: 25, value: `Canvas was clicked ${counter} times`, font: '18pt Arial', draw: [{fill: 'black'}]})]}))

  const sinks = {
    DOM: vdom$,
    Canvas: Observable
      .combineLatest(canvas.element(), renderingInstructions$)
      .map(([hostCanvas, rootElement]) => ({hostCanvas, rootElement}))
  }
  return sinks
}

run(main, drivers);

@grozen
Copy link
Collaborator

grozen commented Nov 12, 2017

While the effort is commendable, I have to admit I am personally not a huge fan of this approach. I really do prefer when a driver hooks into something and allows you to manipulate it by feeding in a stream, but maybe I am just being narrow minded.

This is the main reason I bothered with creating that driver I mentioned in #32, which makes me all the more impartial and so I would rather others weigh in on this.

@vfeskov
Copy link
Contributor Author

vfeskov commented Nov 12, 2017

@grozen i did consider the delayed driver, and while its internal mechanics are basically the same as ones of this approach (using additional element stream from DOM driver), the surface API of the delayed driver is more complicated, requiring another third party package, extra operator, delaying initial rendering instructions and using canvas selector outside of the component that defines it.

The suggested approach is more straightforward, having less caveats, and it is more flexible too by supporting multiple canvases at once. A case when canvas gets removed from DOM can also be covered pretty easily with a minor change. I will include it in this PR

@vfeskov
Copy link
Contributor Author

vfeskov commented Nov 12, 2017

It comes down to comparing these two (shared dependencies omitted):

First - 28 lines:

const drivers = {
  DOM: makeDOMDriver('#root'),
  Canvas: canvasDriver,
  viewport: ...
}

function main (sources) {
  // dynamic width and height of the canvas element
  const vdom$ = sources.viewport.size$.map(({width, height}) => 
    <div>
      <canvas width={width} height={height}></canvas>
    </div>
  )

  // event handling of the canvas element
  const canvas = sources.DOM.select('canvas')
  const counter$ = canvas.events('click').startWith(0).fold(counter => counter + 1)
  const renderingInstructions$ = counter$.map(counter => rect({children: [text({x: 15, y: 25, value: `Canvas was clicked ${counter} times`, font: '18pt Arial', draw: [{fill: 'black'}]})]}))

  const sinks = {
    DOM: vdom$,
    Canvas: xs.combine(canvas.element(), renderingInstructions$)
      .map(([hostCanvas, rootElement]) => ({hostCanvas, rootElement}))
  }
  return sinks
}

run(main, drivers)

Second - 40 lines:

import xsConcat from 'xstream/extra/concat';
import xsDelay from 'xstream/extra/delay';
import {makeDelayedDriver} from 'cycle-delayed-driver';

const drivers = {
  DOM: makeDOMDriver('#root'),
  delayedCanvas: makeDelayedDriver(canvasDriverOnTarget),
  viewport: ...
}

function canvasDriverOnTarget(elements) {
  if (Array.isArray(elements) && elements.some(e => e.id == 'target')) {
    return makeCanvasDriver('canvas#target', {width: 400, height: 300})
  }
  return null
}

function main({ DOM, delayedCanvas, viewport }) {
  // dynamic width and height of the canvas element
  const vdom$ = viewport.size$.map(({width, height}) => 
    <div>
      <canvas id="target" width={width} height={height}></canvas>
    </div>
  )
  
  // event handling of the canvas element
  const canvas = DOM.select('canvas#target')
  const targetCanvas$ = canvas.elements().endWhen(delayedCanvas.driverCreatedSteam());
  const counter$ = canvas.events('click').startWith(0).fold(counter => counter + 1)
  const renderingInstructions$ = counter$.map(counter => rect({children: [text({x: 15, y: 25, value: `Canvas was clicked ${counter} times`, font: '18pt Arial', draw: [{fill: 'black'}]})]}))
    .compose(xsDelay(0))

  const sinks = {
    DOM: vdom$,
    delayedCanvas: xsConcat(targetCanvas$, canvas$)
  }
  return sinks
}

run(main, drivers)

@grozen
Copy link
Collaborator

grozen commented Nov 12, 2017

Oh I don't doubt that using the delayed driver is more cumbersome (and possibly baffling), but if we really wanted we could hide all that inside this driver and end up with another factory that is much more similar to what is already in place.

I ask myself, however, is this really a core issue for this driver? Is it such a widespread scenario that we need to embed this functionality in the driver?

I have my own opinions, but they aren't necessarily better than anyone else's. Except for @Widdershin, whose opinions regarding this driver are the best.

@vfeskov
Copy link
Contributor Author

vfeskov commented Nov 12, 2017

in my case, which i think isn't that rare, i needed width and height of the canvas to dynamically change to fit inside client's viewport, it proved to be a performance boost compared to scaling canvas with CSS.

also to improve performance it is recommended to use multiple canvases: move stable figures to a background canvas, draw some shapes on another canvas only once and use the result pixelmap on the main canvas.

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.

2 participants