Update :class:`~.Text` to use new `ManimPango` color setting #2341

marcin-serwin · 2021-11-29T18:05:41Z

Overview: What does this pull request change?

:class:~.Text class now uses color setting introduced in ManimPango 0.4.0 for color and gradient.

Motivation and Explanation: Why and how do your changes improve the library?

Fixes #2330

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

The PR title is descriptive enough for the changelog, and the PR is labeled correctly
If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
If applicable: newly added functions and classes are tested

naveen521kk · 2021-11-30T08:04:15Z

Did you check disable_ligature flag?

marcin-serwin · 2021-11-30T18:28:53Z

Did you check disable_ligature flag?

I haven't and there is indeed an issue with it, but easily fixable. What's harder to fix are the ligatures themselves, since when they are present there is no way for manim to know which glyph corresponds to which letter(s) of the text. E.g. this code

class Test(Scene):
    def construct(self):
        self.add(Text("fitext",
                      t2c={"fi": RED},
                      disable_ligatures=False))

results in

because method Text.find_indexes doesn't know that fi is a single glyph. This can be fixed by passing color information directly to pango instead of setting it later, but it requires changing ManimPango to support it. I'll try to open a PR there soon, and set this to DRAFT until it is finished.

naveen521kk · 2021-12-13T16:52:15Z

Do you know any reason why macOS tests hangs https://github.com/ManimCommunity/manim/runs/4499349852?check_suite_focus=true#step:18:23? Is it something related to ManimPango?

marcin-serwin · 2021-12-13T16:57:35Z

Do you know any reason why macOS tests hangs https://github.com/ManimCommunity/manim/runs/4499349852?check_suite_focus=true#step:18:23? Is it something related to ManimPango?

I don't know. I don't own a Mac and the github CI message is cryptic. I've tried removing test I added, but it shouldn't break it, since the test is not even run on MacOS.

naveen521kk · 2021-12-14T11:02:04Z

@ManimCommunity/macos Can anyone confirm whether this PR works on macOS and the recent version of ManimPango works correctly/doesn't crash?

PhilippImhof · 2021-12-14T14:36:33Z

Just tried it on my machine (Python 3.9.7, ManimPango 0.4.0, Manim Community v0.13.1 from this PR).

Result:

marcin-serwin · 2021-12-14T16:29:01Z

@PhilippImhof Do tests pass?

PhilippImhof · 2021-12-14T16:57:05Z

Just tried. First, I had a crash, similar to the tests in CI. Checking the output, I modified /manim/tests/test_scene_rendering/opengl/test_opengl_renderer.py:

@pytest.mark.skip(msg="Temporarily skip due to failing in Mac CI")
def test_force_window_opengl_render_with_format(
    using_temp_opengl_config,
    force_window_config_pngs,
    disabling_caching,
):
    """force_window creates window when format is set"""
    scene = SquareToCircle()
    renderer = scene.renderer
    renderer.update_frame = Mock(wraps=renderer.update_frame)
    scene.render()
    assert renderer.window is not None
    renderer.window.close()

Now I have

FAILED tests/test_graphical_units/test_opengl.py::test_Circle - AssertionError: 
FAILED tests/test_graphical_units/test_threed.py::test_Axes - AssertionError: 
 2 failed, 557 passed, 3 skipped, 7 xfailed, 2 xpassed, 224 warnings in 476.46s (0:07:56)

So no failing tests related to this PR and the crash is due to OpenGL, but I cannot say more, because I am absolutely not into that.

naveen521kk · 2021-12-14T17:06:36Z

So no failing tests related to this PR and the crash is due to OpenGL, but I cannot say more, because I am absolutely not into that.

Hmm, does that mean the recent update in ManimPango (at least macOS binary wheels) caused this? Can you confirm that by building manimpango from source and installing it in your venv and then running tests? (run poetry run pip install ManimPango==0.4.0 --force --no-binary :all: --no-cache).

naveen521kk · 2021-12-14T17:07:53Z

Also, do you reproduce this test failure/crash when running in the main branch?

PhilippImhof · 2021-12-14T20:29:15Z

Running the tests on main does not cause any errors, except for the same two tests that fail. (But no problems with OpenGL renderer). That is with ManimPango 0.3.0.

Doing poetry run pip install ManimPango==0.4.0 --force --no-binary :all: --no-cache and running the tests again, I get the same result. But I am not sure it is correct, because poetry show says manimpango 0.3.0, but pip says Successfully installed ManimPango-0.4.0. @naveen521kk: ~~Does poetry show actually check the installed version or does it just repeat what is written in the lock file?~~ I checked with pip show manimpango and it confirms version 0.4.0 is installed.

So just to be sure, I changed the dependencies for ManimPango to 0.4.0 and updated the packages. Now poetry show gives me manimpango 0.4.0 and the tests run fine, except for test_opengl.py::test_Circle and test_threed.py::test_Axes again.

Thus, I would say that ManimPango is not the culprit.

PhilippImhof · 2021-12-15T06:27:49Z

I can see CI tests are still failing for Mac OS. However, on my machine all tests are passing, except for the unrelated test_opengl.py::test_Circle and test_threed.py::test_Axes.

marcin-serwin · 2021-12-24T00:44:53Z

@naveen521kk It took a while, but I've managed to pinpoint the problem. It's the wheels of ManimPango 0.4.0 for macos. As you can see after modifying files to build ManimPango from sources the macos tests pass (windows fails but this is unimportant). I don't have any experience with wheels, do you have any idea what could cause the problem?

naveen521kk · 2021-12-26T12:25:54Z

It's the wheels of ManimPango 0.4.0 for macos. As you can see after modifying files to build ManimPango from sources the macos tests pass (windows fails but this is unimportant). I don't have any experience with wheels, do you have any idea what could cause the problem?

Ah, I see. I thought the same. On ManimCommunity/ManimPango#68 I have changed the macOS to statically link with Pango and for some reason, it crashes here. I think reverting that change would be easier than actually debugging why it crashes here. I work on doing that soon. Thanks for investigating.

PhilippImhof · 2022-01-01T12:41:36Z

With @naveen521kk's changes from ManimCommunity/ManimPango#71 the problem will most certainly be solved. Well done!

naveen521kk · 2022-01-01T16:15:16Z

A new version is released(0.4.0.post0) and you can test it by upgrading to it. If that works, I would like this to be included in this months release #2411. Sorry for the delay!

PhilippImhof · 2022-01-01T16:52:04Z

Tests are now working fine on Mac, problem solved. IMHO this should now be marked as ready for review. Thanks for the work!

Oh and...Happy New Year!

PhilippImhof · 2022-01-01T16:56:19Z

Any ideas why there is an error in the Linux tests now? IIRC all tests passed before, except for Mac where they crashed...

marcin-serwin · 2022-01-01T16:57:46Z

Any ideas why there is an error in the Linux tests now? IIRC all tests passed before, except for Mac where they crashed...

I think some new tests were added that exposed a bug in my current implementation. I'm trying to fix it now.

naveen521kk · 2022-01-03T11:17:48Z

manim/mobject/svg/text_mobject.py

@@ -594,55 +590,137 @@ def set_color_by_t2c(self, t2c=None):
        """Internally used function. Sets color for specified strings."""
        t2c = t2c if t2c else self.t2c
        for word, color in list(t2c.items()):
-            for start, end in self.find_indexes(word, self.original_text):
+            for start, end in self.find_indexes(word, self.text):
                self.chars[start:end].set_color(color)

    def set_color_by_t2g(self, t2g=None):


Is this function used anywhere else? If not you can deprecate it or rather remove it. Same for set_color_by_t2c

naveen521kk

LGTM other than the comment above

naveen521kk

lgtm now

marcin-serwin force-pushed the t2c-bug branch from befa878 to 13c01d7 Compare November 29, 2021 19:28

marcin-serwin marked this pull request as draft November 30, 2021 18:29

marcin-serwin mentioned this pull request Nov 30, 2021

Allow passing color to TextSettings ManimCommunity/ManimPango#65

Merged

marcin-serwin force-pushed the t2c-bug branch 2 times, most recently from 53c6c65 to 87ee54f Compare December 12, 2021 15:49

marcin-serwin force-pushed the t2c-bug branch from 51787c1 to e774bdd Compare December 14, 2021 21:09

marcin-serwin force-pushed the t2c-bug branch 6 times, most recently from f299c24 to b4d6c34 Compare December 24, 2021 00:24

This was referenced Dec 31, 2021

macOS: Wheels in v0.4.0 isn't working ManimCommunity/ManimPango#70

Closed

macOS: build a shared library in wheels ManimCommunity/ManimPango#71

Merged

marcin-serwin force-pushed the t2c-bug branch 2 times, most recently from 3df473f to df3b73d Compare January 1, 2022 16:38

marcin-serwin changed the title ~~Find indexes in stripped text, not original text~~ Update Text to use new ManimPango color setting Jan 1, 2022

marcin-serwin marked this pull request as ready for review January 1, 2022 17:31

naveen521kk reviewed Jan 3, 2022

View reviewed changes

naveen521kk approved these changes Jan 3, 2022

View reviewed changes

marcin-serwin added 11 commits January 3, 2022 13:07

Find indexes in stripped text, not original text

1b1312f

Add regression test

5b032a2

Only run the test in linux environement

2bd859a

Rewrite text2settings in Text to set text color via pango

3b01783

Make gradient in Text use pango coloring

f242086

Bump manimpango to newest version

31011b3

Update test to use new frames_comparison

fe0103e

Don't remove svg file on exception

b90bd3c

Bump manimpango

6f27c8b

Fix pre-commit errors

e77d7a6

Fix index bug

178324e

marcin-serwin force-pushed the t2c-bug branch from 93b35f4 to c8b5f5b Compare January 3, 2022 12:13

marcin-serwin added 3 commits January 3, 2022 13:18

Deprecate no longer used functions set_color_by_t2x

167206f

Remove old commented out code

57f5e6a

Update poetry.lock

36303cb

marcin-serwin force-pushed the t2c-bug branch from c8b5f5b to 36303cb Compare January 3, 2022 12:18

naveen521kk approved these changes Jan 3, 2022

View reviewed changes

naveen521kk merged commit 540dc70 into ManimCommunity:main Jan 3, 2022

naveen521kk added the new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) label Jan 3, 2022

behackl changed the title ~~Update Text to use new ManimPango color setting~~ Update :class:~.Text to use new ManimPango color setting Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update :class:`~.Text` to use new `ManimPango` color setting #2341

Update :class:`~.Text` to use new `ManimPango` color setting #2341

marcin-serwin commented Nov 29, 2021 •

edited by behackl

Loading

naveen521kk commented Nov 30, 2021

marcin-serwin commented Nov 30, 2021

naveen521kk commented Dec 13, 2021

marcin-serwin commented Dec 13, 2021

naveen521kk commented Dec 14, 2021

PhilippImhof commented Dec 14, 2021

marcin-serwin commented Dec 14, 2021 •

edited

Loading

PhilippImhof commented Dec 14, 2021

naveen521kk commented Dec 14, 2021

naveen521kk commented Dec 14, 2021

PhilippImhof commented Dec 14, 2021 •

edited

Loading

PhilippImhof commented Dec 15, 2021

marcin-serwin commented Dec 24, 2021 •

edited

Loading

naveen521kk commented Dec 26, 2021

PhilippImhof commented Jan 1, 2022

naveen521kk commented Jan 1, 2022

PhilippImhof commented Jan 1, 2022

PhilippImhof commented Jan 1, 2022

marcin-serwin commented Jan 1, 2022

naveen521kk Jan 3, 2022

naveen521kk left a comment

naveen521kk left a comment

Update :class:~.Text to use new ManimPango color setting #2341

Update :class:~.Text to use new ManimPango color setting #2341

Conversation

marcin-serwin commented Nov 29, 2021 • edited by behackl Loading

Overview: What does this pull request change?

Motivation and Explanation: Why and how do your changes improve the library?

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

naveen521kk commented Nov 30, 2021

marcin-serwin commented Nov 30, 2021

naveen521kk commented Dec 13, 2021

marcin-serwin commented Dec 13, 2021

naveen521kk commented Dec 14, 2021

PhilippImhof commented Dec 14, 2021

marcin-serwin commented Dec 14, 2021 • edited Loading

PhilippImhof commented Dec 14, 2021

naveen521kk commented Dec 14, 2021

naveen521kk commented Dec 14, 2021

PhilippImhof commented Dec 14, 2021 • edited Loading

PhilippImhof commented Dec 15, 2021

marcin-serwin commented Dec 24, 2021 • edited Loading

naveen521kk commented Dec 26, 2021

PhilippImhof commented Jan 1, 2022

naveen521kk commented Jan 1, 2022

PhilippImhof commented Jan 1, 2022

PhilippImhof commented Jan 1, 2022

marcin-serwin commented Jan 1, 2022

naveen521kk Jan 3, 2022

Choose a reason for hiding this comment

naveen521kk left a comment

Choose a reason for hiding this comment

naveen521kk left a comment

Choose a reason for hiding this comment

Update :class:`~.Text` to use new `ManimPango` color setting #2341

Update :class:`~.Text` to use new `ManimPango` color setting #2341

marcin-serwin commented Nov 29, 2021 •

edited by behackl

Loading

marcin-serwin commented Dec 14, 2021 •

edited

Loading

PhilippImhof commented Dec 14, 2021 •

edited

Loading

marcin-serwin commented Dec 24, 2021 •

edited

Loading