Fix #1039 (<span foreground=> tags)

[markromanmiller]

Motivation

In the interest of letting go of <color> tags and using the Pango-recommended <span foreground="color_spec"> tags, we need to ensure that color passed from the MarkupText object into Pango into the SVG and back to the mobject.

Overview / Explanation for Changes

One bug involved in this process was that the local style of <use> tags was entirely ignored, instead of merely being lower priority. This fix rewrites the <def> section of the SVG module to work closer to SVG standards.

@PhilippImhof will be updating the MarkupText object to work in line with expectations

Oneline Summary of Changes

- Replace <color> syntax by Pango's <span foreground> for coloring parts of MarkupText (#1039) (:pr:`1058`)
- Allow using colors for underline, overline and strikethrough in MarkupText (:pr:`1058`)

I suggest two oneline summaries, because this PR also makes other Pango features available that we could not support until now.

Testing Status

One test has been added, focusing specifically on correct inheritance of <use> element properties.

Further Comments

Acknowledgements

• I have read the Contributing Guidelines

Reviewer Checklist

• Newly added functions/classes are either private or have a docstring
• Newly added functions/classes have tests added and (optional) examples in the docs
• Newly added documentation builds, looks correctly formatted, and adds no additional build warnings
• The oneline summary has been included in the wiki

[PhilippImhof]

@MrMallIronmaker I took the liberty to change the one-line summary in order to be more specific. Feel free to change it back, if you don't like it.

[PhilippImhof]

@naveen521kk I suppose @MrMallIronmaker will still have to address the changes you requested for the SVG part; I did not want to mess around in his code.

[markromanmiller]

Changes made, as long as everything passes tests.

@PhilippImhof would it be possible to write up some visual tests for MarkupText?

[PhilippImhof]

Yes, that's planned. But that's a separate thing, because we need to bundle it with a font file. Graphical unit tests are very likely to fail, because fonts/results differ slightly from one platform to another.

[PhilippImhof]

I have just noticed that this seems to break the Write animation. (Remember my fear for that?) The characters simply appear one after the other. This is probably linked to some issue with the stroke / surrounding path.

[markromanmiller]

Bother. I won't be able to check anything in the next six-to-eight hours or so. Does Write work in the master branch (after the SVG PR)? Or is this an issue in this PR?

If it's just this PR, then my first guess is that there's an issue in how I rewrote handling the defs - it might have an added layer of VGroups or something... Idk.

[markromanmiller]

Re: tests - is there an issue for that / should there be?

[PhilippImhof]

It's this PR; the Write animation works in current master. The tests are noted here: https://github.com/ManimCommunity/manim/projects/7

I'd say no need to open an issue.

[markromanmiller]

It's this PR.

Whew, was worried for a sec. I'm still unsure why the animation would be broken; I'll have to do some digging.

[PhilippImhof]

I wouldn't have missed that in the last PR :)

[PhilippImhof]

Just to avoid an accidental merge, as this PR currently breaks the Write animation.

[markromanmiller]

After a little bit of debugging, I've found that the write animation still works if stroke opacity is set to 1 beforehand. How does Write handle objects with stroke opacity set to zero? If the stroke is fully transparent (or if it's black too? idk) it looks like it's not written.

[markromanmiller]

So objects of type Text and MathTex have, by default, a white stroke with 1.0 opacity and 0 width. In contrast, MarkupText has, by default, 0 opacity and 4 width. I made a rather arbitrary decision that when, in an SVG file, stroke=none, to set stroke opacity to 0. I think I can set stroke width to 0 instead and it would match these other text-based mobjects.

[markromanmiller]

@PhilippImhof - Write(MarkupText) should work now.

For reference, I tested with:

class Foobar(Scene):
    def construct(self):
        txt = MarkupText("foo<span foreground='#ffff00'>bar</span>")
        self.play(Write(txt))
        self.wait()

[PhilippImhof]

So objects of type Text and MathTex have, by default, a white stroke with 1.0 opacity and 0 width.

You mean Tex and MathTex? Because I copied the defaults for MarkupText from Text.

In contrast, MarkupText has, by default, 0 opacity and 4 width. I made a rather arbitrary decision that when, in an SVG file, stroke=none, to set stroke opacity to 0. I think I can set stroke width to 0 instead and it would match these other text-based mobjects.

That's strange, because of

manim/manim/mobject/svg/text_mobject.py

Lines 1187 to 1206 in 4ca1c15

	def __init__(
	self,
	text: str,
	fill_opacity: int = 1,
	stroke_width: int = 0,
	color: str = WHITE,
	size: int = 1,
	line_spacing: int = -1,
	font: str = "",
	slant: str = NORMAL,
	weight: str = NORMAL,
	gradient: tuple = None,
	tab_width: int = 4,
	height: int = None,
	width: int = None,
	should_center: bool = True,
	unpack_groups: bool = True,
	disable_ligatures: bool = False,
	**kwargs,
	):

Compared to

manim/manim/mobject/svg/tex_mobject.py

Lines 207 to 220 in 4ca1c15

	def __init__(
	self,
	tex_string,
	stroke_width=0,
	fill_opacity=1.0,
	background_stroke_width=0,
	background_stroke_color=BLACK,
	should_center=True,
	height=None,
	organize_left_to_right=False,
	tex_environment="align*",
	tex_template=None,
	**kwargs,
	):

So MarkupText and MathTex both have the same default. However, I can confirm that your last commit fixes the problem.

The negative review was to prevent accidental merging. (There was already one approval.) As I also contributed to this PR, I cannot give a positive review myself. Therefore I simply revoke this one.

[naveen521kk]

lgtm

[markromanmiller]

Before I dive in to this wall of text, I should say the PR looks good to me.

---

You mean Tex and MathTex? Because I copied the defaults for MarkupText from Text.

Well, technically Tex, Text, and MathTex.

I see what you're referencing - and the Manim object defaults are still the same. However, none of fill-opacity, stroke, and stroke-opacity are passed from the manim object to the SVG file. (Fill is, through the wrapping foreground color call). Then, the individual VMobjects that compose the SVG VMobject are specified either by the SVG file itself, or by SVG defaults, which means most default styling kwargs don't have any effect, e.g:

class Foobar(Scene):
    def construct(self):
        # txt = MathTex("3ab", "\\cdot uv")
        txt = MarkupText("foo<span foreground='#ffff00'>bar</span>", stroke_color=RED)
        self.play(Write(txt))
        self.wait()

completely ignores stroke_color=RED.

Each individual issue like this one can be 'patched,' but I think this is a larger problem with specifying color and style properties that I outlined in #1023 (but I do admit the problem description I gave is unclear). In short, the code is (to my knowledge) unclear about which style defaults and arguments take priority, both in terms of SVGs and regular old groups, and even the cases when it is clear, it's really easy to put a line of code in the wrong place so that the style priorities are out of order.

Note that this problem only tangentially relates to this PR; I do think this PR itself is good to go.

[PhilippImhof]

Great. I'd like to wait for @jsonvillanueva's review (although we have one approval), because they were also reviewing your SVG rewrite.

[naveen521kk]

completely ignores stroke_color=RED.

does this happen for Text also?

[markromanmiller]

I don't think so, because Text doesn't use the SVGMobject defaults, right? I can't check atm, and probably can't until much later today.

…

On Sun, Feb 28, 2021, 09:19 Naveen M K ***@***.***> wrote: completely ignores stroke_color=RED. does this happen for Text also? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1058 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPG4TU555BEO3S6AWEEMB3TBJ3J3ANCNFSM4YIZ4KPQ> .

[markromanmiller]

@naveen521kk Got a chance to check - this works with Text. Of course, with the default stroke width being 0, the stroke fades out after writing.

(I spelled "foobar" wrong lol)

from manim import *

class Foobar(Scene):
    def construct(self):
        txt = Text("fbooar", stroke_color=RED)
        self.play(Write(txt))
        self.wait()

[PhilippImhof]

So do you suggest I change something in the MarkupText class?

[markromanmiller]

I don't think there's anything to change for this PR. If there were to be something to change, it would have to be in something in SVGMobject, or even Mobject more broadly.

Happy to discuss why SVGs are parsed that way (ignoring style kwargs) if that is helpful, but I don't want to add noise to the conversation if not.

[jsonvillanueva]

LGTM as far as processing style sheets in increasing order of precedence; although, I can't speak much about the write animation fix.

I would ask to improve the test case to demonstrate the inheritance ability: specifically having fill:rgb(x%,y%,z%) inside the <use>' style tag... but this is non-blocking in the interest of time (involves regenerating the .npz control data); however, if you do have time to update the test, please do so. Otherwise, it works and this would be nice to include in the next release!

[naveen521kk]

Is it ok to merge @jsonvillanueva