arch-inc / fabricjs-psbrush

Fabric.js用の感圧ブラシ実装 / A lightweight pressure-sensitive brush implementation for Fabric.js
https://arch-inc.github.io/fabricjs-psbrush/
Other
62 stars 24 forks source link

Error in Fabric 5.0 #43

Open marcohamersma opened 2 years ago

marcohamersma commented 2 years ago

I know the readme states that the library is only available for version 3 and 4, but I figured I would make an issue anyway to make sure people know that there is currently a few errors/bugs.

error in _reset

it seems in the _reset method PSBrush calls this._setBrushstyles() (a function it inherits from fabric) with no arguments, but it seems that in version 5 _setBrushStyles expects the context to be passed as the first argument:

  _setBrushStyles: function _setBrushStyles(ctx) {
    ctx.strokeStyle = this.color;
    ctx.lineWidth = this.width;
    ctx.lineCap = this.strokeLineCap;
    ctx.miterLimit = this.strokeMiterLimit;
    ctx.lineJoin = this.strokeLineJoin;
    ctx.setLineDash(this.strokeDashArray || []);
  },

I managed to monkeypatch this fix this in my code by doing the following

const baseBrush = new PSBrush(canvas);
const original = this.baseBrush._setBrushStyles;
  this.baseBrush._setBrushStyles = function _setBrushStyles(ctx) {
  if (!ctx) ctx = this.canvas.contextTop;
  original(ctx);
};

Brush color

When the brush's color is set to a color, the brush will still show up as black while drawing, only switching to the proper color after finishing the stroke

https://user-images.githubusercontent.com/577144/169871456-8da35190-108d-4cd7-a5ca-d7326893a97f.mov

Peer dependency

Additionally, it would be good if Fabricjs became a peerDependency, rather than a dependency.

arcatdmz commented 2 years ago

@marcohamersma Thanks, your suggestion all sounds great and reasonable! Let me look into Fabric 5.0, make appropriate changes, and move Fabric.js to peerDependency. (Also, please feel free to submit a PR as I might not have enough time in coming several weeks.)

r-debashis commented 2 years ago

I am also facing the same issue with fabric 5+. Would love to use this library for our next release.

marcohamersma commented 2 years ago

I've created a pull request for these fixes :)

There is however another issue that I'm having with Fabric's new-ish Eraser functionality/mixin. This among other things adds an eraser property to Fabric.Object which can store a Fabric.Eraser object if the brush stroke intersects with the eraser.

The problem occurs when running canvas.loadFromJSON; the PSBrush seems to not restore the plain "object" variation of eraser to a proper Fabric.Eraser. I'm trying to find out where the issue is exactly. My experiments in this codesandbox show that regular brush strokes are not affected by this issue.

So I think the issue is with the PSStroke.fromObject override, but I'm still trying to pinpoint it exactly (when I have some time). If anyone else has some ideas for a fix, that would be super helpful

Zhuxb-Clouds commented 9 months ago

I've created a pull request for these fixes :)

There is however another issue that I'm having with Fabric's new-ish Eraser functionality/mixin. This among other things adds an eraser property to Fabric.Object which can store a Fabric.Eraser object if the brush stroke intersects with the eraser.

The problem occurs when running canvas.loadFromJSON; the PSBrush seems to not restore the plain "object" variation of eraser to a proper Fabric.Eraser. I'm trying to find out where the issue is exactly. My experiments in this codesandbox show that regular brush strokes are not affected by this issue.

So I think the issue is with the PSStroke.fromObject override, but I'm still trying to pinpoint it exactly (when I have some time). If anyone else has some ideas for a fix, that would be super helpful

The same issue - have you already resolved it or do you have any progress to share?

marcohamersma commented 9 months ago

I have no progress on this, I refrained from updating Fabric for the time being