Comments (6)
Hi @agilgur5 ! That's a really nice and simple PR. I'll keep an eye on it, and as soon as it's merged over there, I'll close this issue. Thank you for your work 🙂
from react-signature-canvas.
Nice investigation, thanks! Cloning the data can indeed have a performance impact. I believe that in this case, it's probably negligible (unless the signature is quite complexe 😄), but it's definitely worth having it in mind.
Do you think that a PR changing this:
signaturePad.fromData(data, { clear: false });
to this
signaturePad.fromData(data, { clear?: boolean, immutable?: boolean });
could have chances of being accepted? (It's not ideal like this, but it's a first step toward having it immutable by default for the next major release 😉)
Thanks also for [].concat(signaturePoints)
, it's indeed better to not have to install a third-party library just for this. [...signaturePoints]
is also possible.
from react-signature-canvas.
Merged! Well done!
from react-signature-canvas.
Marked this as upstream back when it was opened, but never quite responded. Thanks for providing a repro!
Upstream Bug, but might have performance implications
react-signature-canvas
is a pretty simple wrapper around signature_pad
, so this bug is upstream and not here. The one-liner code for fromData
can be found on this line.
I dug a bit deeper upstream in signature_pad
, and found this to be caused by this line: this._data = pointGroups;
. This sets the internal pointer to be the same as the data passed into fromData
.
Ideally, this should be copied over instead of using the ref so that mutations don't affect the arguments that were passed in. This is definitely an edge case though, as I've actually never come across a use-case for this. That being said, it's a simple change to fix this, so it could be contributed if there isn't a significant performance penalty to this (i.e. if the data is huge, copying could cause some lag).
Next Steps to fix
signature_pad
recently had some large changes and has released v4.0.0 (v3 never made it past beta, so it was basically v2.5.2 -> v4.0.0). It's a breaking change, so it will likely require a similar breaking change in this package due to the shared API surface.
Part of that change was this PR szimek/signature_pad#570 that changed that line a tiny bit, but did not quite fix this issue for all cases.
However, a one-liner can be done to fix it for all cases, [].concat(pointGroups)
instead of pointGroups
.
I'll try to make a PR for that soon in signature_pad
and then I'll have to get the upgrade in as well to fix this.
Given the possible perf implications and the simple workaround available for this edge case, it's possible this won't be accepted though.
Workaround Available
canvasRef.current.fromData(R.clone(signaturePoints));
Thanks for providing a workaround with your issue! Per my above paragraph, this can be simplified with raw JS so as not to require use of a library like Rambda:
canvasRef.current.fromData([].concat(signaturePoints))
Some tests for this workaround
Also did some tests to make sure that there aren't further pointer issues deeper in the data structure with signature_pad
's usage:
const signaturePoints = [
[
{ x: 54.002685546875, y: 130.68206787109375, time: 1596180140938 },
{ x: 87.002685546875, y: 86.68206787109375, time: 1596180141056 },
{ x: 114.002685546875, y: 66.68206787109375, time: 1596180141072 },
{ x: 130.002685546875, y: 60.68206787109375, time: 1596180141088 },
{ x: 143.002685546875, y: 60.68206787109375, time: 1596180141104 },
{ x: 150.002685546875, y: 61.68206787109375, time: 1596180141120 },
],
]
const data = [].concat(signaturePoints)
// push is used in signature_pad: https://github.com/szimek/signature_pad/blob/878786ef6ef38bf317d1cf674b3455d39e98d270/src/signature_pad.ts#L299
data.push([
{x: 132.002685546875, y: 110.68206787109375, time: 1596185355643 },
{x: 135.002685546875, y: 106.68206787109375, time: 1596185355725 },
{x: 146.002685546875, y: 98.68206787109375, time: 1596185355741 },
{x: 153.002685546875, y: 94.68206787109375, time: 1596185355757 },
{x: 157.002685546875, y: 91.68206787109375, time: 1596185355790 },
{x: 146.002685546875, y: 98.68206787109375, time: 1596185355741 },
])
console.log(signaturePoints)
// [
// [
// {x: 54.002685546875, y: 130.68206787109375, time: 1596180140938 },
// {x: 87.002685546875, y: 86.68206787109375, time: 1596180141056 },
// {x: 114.002685546875, y: 66.68206787109375, time: 1596180141072 },
// {x: 130.002685546875, y: 60.68206787109375, time: 1596180141088 },
// {x: 143.002685546875, y: 60.68206787109375, time: 1596180141104 },
// {x: 150.002685546875, y: 61.68206787109375, time: 1596180141120 },
// ]
// ]
from react-signature-canvas.
Cloning the data can indeed have a performance impact. I believe that in this case, it's probably negligible (unless the signature is quite complexe 😄), but it's definitely worth having it in mind.
Agreed, but figured I'd might as well test this and see to make sure. I did a quick test (see screenshot below) of cloning the full data of a signature and there was no noticeable lag on my end. Of course, this can vary per device (e.g. on a low-end mobile device vs. a beefy laptop), but given that fromData
really shouldn't be used that often anyway, I don't see many use-cases where it may make a difference (the only one I could anticipate is reading then writing back on resizes, which could already induce lag in and of itself).
could have chances of being accepted? (It's not ideal like this, but it's a first step toward having it immutable by default for the next major release 😉)
Now that I've tested the performance I'll try a PR with the clone. If not, I think that's a good alternative, though I would rename the parameter to something like cloneData
instead to be more clear ("immutable" without context makes me think "immutable what?")
from react-signature-canvas.
Linked this to #68 and a new tracking milestone as react-signature-canvas
still has to update to signature_pad
v4 (which is a non-trivial amount of work, per #68)
from react-signature-canvas.
Related Issues (20)
- issues in latest Google Chrome 117 HOT 4
- Cannot get SVG data url after `getTrimmedCanvas()` HOT 3
- working in localhost but when published not working on dev env HOT 1
- Saving and Restoring DataURL results in smaller image HOT 1
- Warning in browser console: `Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true` when calling `toDataURL` HOT 6
- Update to `signature_pad` v4 for "passive" events? HOT 4
- `time` attribute only present in last stroke
- Convert raw array data (`toData`) to image so it can be scaled? HOT 5
- React 18 support in peerDeps HOT 9
- Custom CSS properties? (for custom cursor etc) HOT 1
- `TypeError: Cannot read properties of null (reading 'off')` during mount when running tests for a wrapper component HOT 8
- `onBegin` not called when `fromData` invoked HOT 1
- `TypeError: undefined is not an object (evaluating 'this._data[this._data.length-1].push')` HOT 1
- `TypeError: sigCanvas.current.getTrimmedCanvas is not a function` HOT 1
- `TypeError: sigCanvas.current.getTrimmedCanvas is not a function` HOT 2
- Background Image lost while converting `toData` and back with `fromData` ("undo" operation) HOT 2
- support for `redrawOnResize` func HOT 2
- Weird artifacts using Next.js 13 HOT 3
- Stroke offset with Next.js prod build HOT 2
- What is the difference between SignaturePad and SignatureCanvas? HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from react-signature-canvas.