chanind / hanzi-writer

Chinese character stroke order animations and practice quizzes
https://hanziwriter.org
MIT License
3.47k stars 539 forks source link

feat: Canvas & SVG renderers will no longer will invert characters #239

Open jamsch opened 3 years ago

jamsch commented 3 years ago

This is in anticipation of a major version update to the hanzi-writer-data & hanzi-writer-data-jp packages to fix a core issue with path layouts being vertically flipped. After this change, the only assumption left (in regards to character data) will be that the character's bounding box is 1024x1024.

The fix that will be applied to the characters is listed in this repository:

https://github.com/jamsch/hanzi-writer-char-fixer

BREAKING CHANGE: Users will need to update their hanzi-writer-data & hanzi-writer-data-jp character packages

codecov-commenter commented 3 years ago

Codecov Report

Base: 96.31% // Head: 96.55% // Increases project coverage by +0.24% :tada:

Coverage data is based on head (4bc8a47) compared to base (c0c08b0). Patch coverage: 96.25% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #239 +/- ## ========================================== + Coverage 96.31% 96.55% +0.24% ========================================== Files 32 31 -1 Lines 1139 1161 +22 Branches 207 215 +8 ========================================== + Hits 1097 1121 +24 + Misses 38 36 -2 Partials 4 4 ``` | [Impacted Files](https://codecov.io/gh/chanind/hanzi-writer/pull/239?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin) | Coverage Δ | | |---|---|---| | [src/characterActions.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL2NoYXJhY3RlckFjdGlvbnMudHM=) | `100.00% <ø> (ø)` | | | [src/utils.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL3V0aWxzLnRz) | `95.00% <91.66%> (-2.19%)` | :arrow_down: | | [src/models/Character.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL21vZGVscy9DaGFyYWN0ZXIudHM=) | `97.22% <97.05%> (-2.78%)` | :arrow_down: | | [src/HanziWriter.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL0hhbnppV3JpdGVyLnRz) | `95.60% <100.00%> (+1.68%)` | :arrow_up: | | [src/Mutation.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL011dGF0aW9uLnRz) | `96.62% <100.00%> (-0.08%)` | :arrow_down: | | [src/Positioner.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL1Bvc2l0aW9uZXIudHM=) | `100.00% <100.00%> (ø)` | | | [src/Quiz.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL1F1aXoudHM=) | `97.26% <100.00%> (+1.36%)` | :arrow_up: | | [src/geometry.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL2dlb21ldHJ5LnRz) | `100.00% <100.00%> (ø)` | | | [src/renderers/canvas/HanziWriterRenderer.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL3JlbmRlcmVycy9jYW52YXMvSGFuemlXcml0ZXJSZW5kZXJlci50cw==) | `100.00% <100.00%> (+3.33%)` | :arrow_up: | | [src/renderers/canvas/canvasUtils.ts](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin#diff-c3JjL3JlbmRlcmVycy9jYW52YXMvY2FudmFzVXRpbHMudHM=) | `100.00% <100.00%> (ø)` | | | ... and [3 more](https://codecov.io/gh/chanind/hanzi-writer/pull/239/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Chanin)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

chanind commented 3 years ago

What's the bug this is solving? The data is flipped because that's the format that makemeahanzi uses, although I'm not sure why it uses that format. The positioner corrects the flip and scales the contents to fit the window so the character should appear correctly on the screen. Is the character not rendering correctly in some scenarios?

jamsch commented 3 years ago

Not so much a bug fix, but the intention is to make it easier for users to add in their own custom characters without requiring them to write an algo to vertically flip the paths & medians (which has personally been a source of pain for adding in missing Kana and other writing systems). It should also make it easier to export the character data to SVG to be used outside the library (e.g. on React Native) without requiring scaling transforms.

This change comes from a project that now utilizes these normalized characters. It certainly will warrant a major version update to prevent users from loading in the old characters. I'll be happy to close this though if you don't feel that this is part of the scope of this repo.

chanind commented 3 years ago

What do you think about making the data parsing part of the code replaceable? So, instead of it being hardcoded to the makemeahanzi format, you could pass in a character parsing function as an option that would take in the character JSON in whatever format it's in and return a format that hanzi-writer can work with. By default it would use a parsing function for makemeahanzi, but you'd be able to make your own data format and just pass a formatting option so it would work. That way we wouldn't need to make a breaking change, while still allowing for flexibility in how the character data is structured.

jamsch commented 3 years ago

@chanind Yep, makes sense. Will add to this PR with your suggested changes soon.

If I'm understanding you correctly, we'd implement a default character parser/transformer (as we're assuming this is used for hanzi-writer-data / hanzi-writer-data-jp character sets) that flips the Y transform and offsets the Y medians so that it can be rendered normally on the Canvas/SVG renderers (as already done in this PR). For users that already have normalized characters they wouldn't need to go through this step unless their character spec differs from a 1024x1024 bounding box.

chanind commented 3 years ago

Yeah I think that's reasonable. For cases where the data is already in the 1024x1024 and pre-flipped format, you'd still need to pass something into the processCharData option, or whatever we want to call that param, that keep it from doing any transforms. Maybe something like processCharData: data => data as an identity function? Or maybe we could create a shorthand like processCharData: 'plain' or something? By default it would assume the data is in makemeahanzi format, and the default processCharData function would act accordingly