Skip to content

Conversation

@behackl
Copy link
Member

@behackl behackl commented Sep 24, 2020

List of Changes

Rendering long strings as Text lets the text be outside of the predefined canvas, leading to only a part of the string being converted to the svg. This PR sets the canvas width dependent on the text width.

Motivation

Resolves #469.

Testing Status

The issue is fixed, but the test I wanted to add is currently not working.

Depends on #492

Acknowledgement

@behackl
Copy link
Member Author

behackl commented Sep 24, 2020

The tests here fail (at least locally, so I'd be surprised if they would pass in the pipeline), because it seems it is not possible to call Text within a test. Is there a workaround I could use, @ManimCommunity/tests ?

@behackl behackl added pr:bugfix Bug fix for use in PRs solving a specific issue:bug question Further information/clarification is needed labels Sep 24, 2020
@behackl
Copy link
Member Author

behackl commented Sep 25, 2020

As discussed on Discord (@huguesdevimeux, @naveen521kk): if the size is too large (e.g., size=3000, which is actually ridiculously large), then the same error as in #469 occurs.

I've tried to fix it in a similar way, but I fear that this hits some other limit than the canvas size: no matter how large I set the canvas, and no matter how I place the text, the error remains.

If someone else has an idea how to fix this, suggestions are welcome. Otherwise I see two possible ways forwards:

  1. Simply ignore it, or
  2. throw a ValueError when setting size larger than 1000 with a link to this PR.

@huguesdevimeux huguesdevimeux marked this pull request as draft September 27, 2020 16:33
@huguesdevimeux
Copy link
Member

If there is no fix, I think ignoring and warning the user that there is a problem is fine IMO

@behackl
Copy link
Member Author

behackl commented Sep 27, 2020

If there is no fix, I think ignoring and warning the user that there is a problem is fine IMO

@huguesdevimeux Good idea! I'll raise a warning if a font size larger than 500 or so is used.

Another issue: the test_text.py introduced here does not work: when I run pytest from the root directory of the repository, then this fails with

E       manim.mobject.svg.text_mobject.cairo.IOError: error while writing to output stream

manim/mobject/svg/text_mobject.py:300: cairo.IOError
=== short test summary info ===
FAILED tests/test_text.py::test_long_string - manim.mobject.svg.text_mobject.cairo.IOError: error while writing to output stream

The reason seems to be that the media/texts directory does not exist (at least when I create that manually, then the test passes). Do you think it would be a good idea to make this more robust in some sense? Maybe either by letting the call to Text create a directory if it does not exist, or otherwise reverting to using an appropriate temporary directory?

@huguesdevimeux huguesdevimeux requested review from leotrs and removed request for leotrs September 28, 2020 13:47
@leotrs
Copy link
Contributor

leotrs commented Sep 29, 2020

This LGTM but all tests are failing :(

@behackl
Copy link
Member Author

behackl commented Sep 29, 2020

This LGTM but all tests are failing :(

Yes, this is what I mentioned recently on Discord. Text and Tex need to be more robust regarding directory creation, I hope to submit a separate PR for this soon, this this here depends on that.

@leotrs
Copy link
Contributor

leotrs commented Sep 29, 2020

Ah I see. Well sounds like a great opportunity to use our handy new DEP bot.

@naveen521kk naveen521kk mentioned this pull request Sep 29, 2020
4 tasks
@dep dep bot added the pr:dependent This PR or issue requires that another PR is merged first label Sep 29, 2020
@leotrs
Copy link
Contributor

leotrs commented Oct 15, 2020

@behackl This can now continue development, no?

Not trying to be pushy, just making sure :)

@behackl behackl marked this pull request as ready for review October 25, 2020 19:39
@behackl
Copy link
Member Author

behackl commented Oct 25, 2020

I've merged master and applied the same solution/warning that I've added to CairoText to PangoText as well.

PangoText and CairoText share a lot of common code, if we intend to keep CairoText around for a bit longer, it might make sense to refactor these to classes a bit. The fact that I've added the solution to both in the very same way should not make an eventual refactor more complicated, but I'd like to not do it here immediately but rather observe how the situation with PangoText evolves.

@leotrs
Copy link
Contributor

leotrs commented Oct 25, 2020

The plan for Pango/CairoText is to start using the former instead of the latter (#609). Once that is merged, we will probably keep CairoText around for a bit until we are sure Pango is working well. Then, CairoText is going away completely.

So I wouldn't worry too much about keeping them aligned. As long as #609 is merged soon, we should be able to completely forget about CairoText soon as well.

@leotrs
Copy link
Contributor

leotrs commented Oct 28, 2020

@behackl don't stress about pango/cairo for this one ;)

also, would it be possible to add a test so we don't regress in the future?

@behackl
Copy link
Member Author

behackl commented Oct 29, 2020

I've already added a test here to check that. I am not entirely sure, however, why the tests on this PR fail only for windows; I need to investigate a bit more.

@behackl
Copy link
Member Author

behackl commented Nov 11, 2020

The situation here has not become any clearer, and I have absolutely no clue why changing the canvas size dynamically (in particular: making it larger makes test fail on Windows.

Overall, the Text class is very messy. I propose to get back to this PR once we have managed to refactor Text a little.

@behackl behackl closed this Nov 11, 2020
@behackl behackl mentioned this pull request Dec 1, 2020
1 task
@behackl behackl deleted the text-canvas-size branch May 16, 2022 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:bugfix Bug fix for use in PRs solving a specific issue:bug pr:dependent This PR or issue requires that another PR is merged first question Further information/clarification is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IndexError: list index out of range in text_mobject

4 participants