Improved text shaping #3106
Open
Improved text shaping #3106
+7,574
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is BEFORE we discussed it with Florin and found lots of improvements to make
* Create npm script to run demo server * Add config and npm script for doing a lighter dev build
One line text is not perfect Multiline text is now showing (Work in progress)
Also fixed the line height issue.
(actually, in paths)
Currently, the arabic text shows in the wrong order. However, this version turns off arabic ligatures and this is the right approach. We need to add another shaping pass to establish the correct text directions.
(stashed in the 'arabic-wip' branch) This reverts commit bdc8209.
Also update the text sample page to show side-by-side examples.
Needs refactoring right away - the code is not clean
Opens with 0.0.0.0:8000/demo/text/skottie/ First test is a single Skript test for debugging The rest comes from the skottie resources
bodymovin
reviewed
Sep 3, 2024
player/js/utils/text/SkriptShaper.js
Outdated
| if (goingLeft) { | ||
| return TextDirection.Mixed; | ||
| } | ||
| console.assert(goingRight); |
There was a problem hiding this comment.
I removed the assert but kept the logging since these are significant problems and I don't want them to go unnoticed at least for now when the code is not well tested.
There was a problem hiding this comment.
I updated all logging to be disabled (no-op) by default. We can enable for testing using a window flag. @bodymovin PTAL.
bodymovin
reviewed
Sep 3, 2024
player/js/utils/text/SkriptShaper.js
Outdated
| span = document.getElementById(this.coloredId); | ||
| rect = document.getElementById(this.measurementId); | ||
| } else { | ||
| this.coloredId = 'undefined'; |
There was a problem hiding this comment.
are quotes around undefined intentional?
There was a problem hiding this comment.
This code should go away -- it's just a leftovers of an old way of testing.
bodymovin
reviewed
Sep 3, 2024
| @@ -0,0 +1,121 @@ | |||
| import { nodeResolve } from '@rollup/plugin-node-resolve'; | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a new/optional text shaper implementation, with improved internationalization (RTL, BiDi) support. The new shaping engine can be selected via animation
rendererSettings: