ManimCommunity / ManimPango

Binding for Pango, to use with Manim.
https://manimpango.manim.community
MIT License
45 stars 13 forks source link

Allow passing color to TextSettings #65

Closed marcin-serwin closed 2 years ago

marcin-serwin commented 2 years ago

This will allow to fix problem described in ManimCommunity/manim/pull/2341

PhilippImhof commented 2 years ago

Thanks for the contribution. I will have a closer look at this at latest by the end of the week. I hope you can wait until then and I apologise for probably not being able to do it sooner.

The approach you are using is similar to what MarkupText does. (Actually, that was the reason why I wrote that class roughly a year ago. I was regularly having trouble with t2c and co.) I think, it should be the final goal to merge Text and MarkupText into one single non-LaTeX text class. But I am not sure there is a consensus on that. And your contribution definitely improves the existing Text class.

PhilippImhof commented 2 years ago

Thanks for the PR. This looks good to me. Using https://gnome.pages.gitlab.gnome.org/gtk/Pango/struct.Attribute.html seems like a better idea to me rather than using PangoMarkup. I guess I will work on it later.

I think that PangoAttribute has its advantages. My concern is rather on the end-user side of it. I think it would be good to get rid of the archaic syntax we have inherited, also because it does not really allow overlapping formatting. Using PangoMarkup is an easy solution, because it offers a solution on the backend and on the frontend.

naveen521kk commented 2 years ago

I think that PangoAttribute has its advantages. My concern is rather on the end-user side of it.

Probably we should this a better API than the current one. I mentioned PangoAttribute because it would be better than creating XML kinda strings ourselves and then passing it to pango.

marcin-serwin commented 2 years ago

I've noticed another problem, this time with gradient. It is a problem for both Text and MarkupText, namely the following code

class Test1(Scene):
    def construct(self):
        self.add(
            Text(
                "gradiented office isn't perfect",
                t2g={"office": (GREEN, RED)},
            ).move_to(UP))
        self.add(
            MarkupText(
                'gradiented <gradient from="GREEN" to="RED">office</gradient> isn\'t perfect',
            ).move_to(DOWN))

when run with current main branch results in gradient main

I've changed the implementation of gradient in text to set colors individually on each letter but this exposed another problem, namely the same code now results in gradient partially fixed

As you can see the ff ligature is no longer present. This is again problem with ManimPango as text2svg draws each group of formatted letters individually. Hence this code:

class Test2(Scene):
    def construct(self):
        self.add(
            Text(
                "gradiented office isn't perfect",
                t2s={"f": NORMAL},
            ))

results in Test2_ManimCE_v0 12 0 because ManimPango draws

marcin-serwin commented 2 years ago

The tests are failing because windows is missing some fonts. I don't think it is caused by my changes, as I didn't touch fonts.

PhilippImhof commented 2 years ago

AFAICS, the error mentioned above (gradiented office) is explained in the docs for MarkupText: one can use <gradient from="RED" to="YELLOW" offset="2,1">example</gradient> to start the gradient two letters earlier and end it one letter earlier

I admit that this is only a workaround. However, as long as we do not have gradients built-in to Pango, we must do them at the SVG level and there just seems to be no way to reliably know how many glyphs you really get from a certain text.

naveen521kk commented 2 years ago

The tests are failing because windows is missing some fonts. I don't think it is caused by my changes, as I didn't touch fonts.

I think the change which deletes the SVG file on the error raised has caused the issue. I think removing that could fix the errors(at least the permission one).

marcin-serwin commented 2 years ago

AFAICS, the error mentioned above (gradiented office) is explained in the docs for MarkupText: one can use <gradient from="RED" to="YELLOW" offset="2,1">example</gradient> to start the gradient two letters earlier and end it one letter earlier

You're right, I didn't notice that. I think instead of requiring offsets, as a workaround we could set colors directly with markup in strings passed to pango. So for example we could replace <gradient from"WHITE" to="RED">ab</gradient> with <span color="WHITE">a</span><span color="RED">b</span> before sending it to pango. This will of course still be workaround since ligatures will be a single color (of the second letter I think), but as you said we can't do better without built-in gradients.

naveen521kk commented 2 years ago

I will have a look at the tests failing on windows tomorrow.

naveen521kk commented 2 years ago

I will have a look at the tests failing on windows tomorrow.

The tests failed because the order of execution of tests has changed. There is some kind of cache in Pango that gets interfered in these tests. Can you rebase on https://github.com/ManimCommunity/ManimPango/pull/67 and check if that works?

naveen521kk commented 2 years ago

@marcin-serwin Can you rebase again with main?

naveen521kk commented 2 years ago

I will release a new version after the CI is fixed #68 . Thanks.

naveen521kk commented 2 years ago

I have released 0.4.0 now, it should have this change.