feat: allow checking backwards strokes #252
Conversation
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
==========================================
+ Coverage 96.16% 96.26% +0.10%
==========================================
Files 32 32
Lines 1121 1126 +5
Branches 202 205 +3
==========================================
+ Hits 1078 1084 +6
+ Misses 39 38 -1
Partials 4 4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This is such a great idea! Thank you for submitting this! Some minor thoughts:
- I think it's fine to always check whether the stroke is backwards, it shouldn't hurt anything to add an
isStrokeBackwards: booleanintoonMistakeandisCorrectcallbacks. - Likewise, IMO it should be fine to just add an option
allowBackwardsStrokes: booleanor something similar to the options for quizzing, rather than inputting a 3-valued string
src/strokeMatches.ts
Outdated
| @@ -20,34 +20,44 @@ const START_AND_END_DIST_THRESHOLD = 250; // bigger = more lenient | |||
| const FRECHET_THRESHOLD = 0.4; // bigger = more lenient | |||
| const MIN_LEN_THRESHOLD = 0.35; // smaller = more lenient | |||
|
|
|||
| export type StrokeMatchType = 'miss' | 'match' | 'backwards-match'; | |||
There was a problem hiding this comment.
nit: I think it would be better to return an object here instead of a string, so that in the future we can enhance it further. I can imagine it might be useful to know why the stroke didn't match beyond only the direction. What do you think about returning something like the following:
type StrokeMatchResult {
match: boolean; // whether or not the match is successful
failureReasons?: Array<'direction' | 'position' | 'length' | 'shape'>
// in the future we could add more details here about the match result if needed
}There was a problem hiding this comment.
Cool, I'm happy to change allowBackwardsStrokes to boolean and to return an object from strokeMatches.
I like the idea of failureReasons, but are the *Match variables independent enough that they can be used to convey that level of specificity about failure? This may be my unfamiliarity with the code, but when I was playing around with it, I wasn't able to determine whether a stroke was valid aside from being backward just by looking at the existing vars. For instance, with the identifies backwards stroke when allowed test, the initial check has shapeMatch: false and startAndEndMatch: true which both seem incorrect (or at least unintuitive) to me. This is why I went the the approach of recursing with the reversed points array.
There was a problem hiding this comment.
Yeah you're doing it right I think. The shape match is using Frechet Distance, which is dependent on direction already. This is a good point, it's hard to isolate a single reason why a stroke failed, because it could fail for multiple reasons, and the reasons sort of overlap (Frechet distance is distance + shape + direction, for example).
Maybe something like this in that case?
type StrokeMatchResult {
match: boolean; // whether or not the match is successful
meta: {
strokeIsBackwards: boolean // if the stroke is correct in all ways except it's backwards
// in the future we can add more stuff here if we want
}
}
Because it is recursive, it requires an explicit return type annotation.
|
🎉 This PR is included in version 3.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The use case for this is to allow applications to notify a user when they make a backwards stroke (one which is correct except for the direction in which in which it's made) on a quiz. In such a case, the application will likely want to show some messaging to the user indicating the stroke was made in the wrong direction. The application may want to treat backwards strokes as correct or incorrect.
This change includes two API changes to fulfill the above use case:
checkBackwardsStrokesternary quiz option (defaulted off)onCorrectStrokeandonMistakequiz handlers receive anisBackwardsboolean property on the stroke data.checkBackwardsStrokesis ternary to allow avoiding the perf hit of checking backwards strokes if the application does not care about them.If
checkBackwardsStrokesisfalse, the handlers will always receiveisBackwards: falsesince the backwards check is skipped. The alternative here is to return e.g.nullif the check was skipped. I opted against this to keep the API a bit simpler with a consistent type forisBackwards.