Skip to content

Conversation

@NavpreetDevpuri
Copy link
Contributor

Reopening #90
Fixed 3b1b/manim#1067
NOTE : SurroundingRectangle() only contains visible text.

class te(Scene):
    def construct(self):
        text1 = Text(" ab ", font="Consolas", size=2)
        rect1 = SurroundingRectangle(text1)
        text1.chars[1].set_color(GREEN)
        self.add(text1,rect1)

Output
te

class bug1(Scene):
    def construct(self):
        text1 = Text("  ab\ncd", font="Consolas", size=2)
        text2 = Text("ab\ngh", font="Consolas", size=2)
        rect1 = SurroundingRectangle(text1)
        rect2 = SurroundingRectangle(text2)
        self.play(Transform(remove_invisible_chars(text1), remove_invisible_chars(text2)), Transform(rect1, rect2))
        self.wait()

Output
bug1

Added new parameters background_stroke_width and background_stroke_color

class bug2andbug3(Scene):
    def construct(self):
        helloworldc = Code(
            "helloworldc.c",
            tab_width=4,
            background_stroke_width=1,
            background_stroke_color=WHITE,
            insert_line_no=True,
            style=Code.styles_list[4],
            background="window",
            language="cpp",
        )
        helloworldcpp = Code(
            "helloworldcpp.cpp",
            tab_width=4,
            background_stroke_width=1,
            background_stroke_color=WHITE,
            insert_line_no=True,
            style=Code.styles_list[15],
            background="window",
            language="cpp",
        )
        helloworldc.move_to(np.array([-3.6, 0, 0]))
        helloworldcpp.move_to(np.array([3.1, 0, 0]))
        self.add(helloworldc,helloworldcpp)

Output
bug2andbug3

Updated Paragraph() and added new methods set_line_to_initial_position() and set_all_lines_to_initial_positions()

class para(Scene):
    def construct(self):
        t = Paragraph('this is a awesome', 'paragraph', 'With \nNewlines', '\tWith Tabs', '  With Spaces',
                      'With Alignments',
                      'center', "left", "right")
        t.set_line_alignment("center", 7)
        t.set_line_alignment("right", 9)
        t.shift(2 * RIGHT)
        rect = SurroundingRectangle(t)
        self.add(t, rect)
        self.wait()
        self.play(t.set_all_lines_alignments, "right")
        self.play(t.set_line_to_initial_position, 4)
        self.play(t.set_all_lines_to_initial_positions)
        self.play(t.set_line_alignment, "center", 7)
        self.play(t.set_line_alignment, "right", 9)
        t.chars[0][5:7].set_color(GREEN)
        t[1][0:4].set_color(YELLOW)
        self.wait()

Output
ezgif-3-3dc23c79c5fa

Copy link
Member

@Aathish04 Aathish04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NavpreetDevpuri .

There have been some major changes since you last pulled from master.

  • These include changing all the imports to relative imports, and
  • Changing the main config system.
  • Cleaning up some things from constants.py.
  • Internally renaming the manimlib library to manim.

Your PR has added back some of the things that were once removed.
These include the relative imports in code_mobject.py and a lot of what was cleaned up from constants.py. Your code also makes frequent imports from manimlib which has been renamed to manim.

Please address these.

The easiest way would be to pull from ManimCommunity/Master, and simply go through your changes and re-add them.

@NavpreetDevpuri
Copy link
Contributor Author

@Aathish04 thanks for that. i did not notice that.
Now i fixed it. check it.

@safinsingh safinsingh requested a review from Aathish04 July 13, 2020 20:37
@Aathish04 Aathish04 added pr:bugfix Bug fix for use in PRs solving a specific issue:bug documentation Improvements or additions to documentation enhancement Additions and improvements in general labels Jul 18, 2020
@huguesdevimeux
Copy link
Member

I didn’t follow this PR at all. Is it ready ?

@NavpreetDevpuri
Copy link
Contributor Author

I didn’t follow this PR at all. Is it ready ?

I think it will work.
But i didn't tested it by myself. I installed new copy of windows and manim's development environment is deleted. I will test it myself.

@Aathish04
Copy link
Member

I didn’t follow this PR at all. Is it ready ?

I think it will work.
But i didn't tested it by myself. I installed new copy of windows and manim's development environment is deleted. I will test it myself.

@NavpreetDevpuri I'm so sorry for taking way too much time to respond. There are two things that I think need to be done before this is ready to merge:

a) Format the code via black,
b) Fix merge conflicts between your branch and master.

Once these are resolved, I think we'll be ready to merge.

@NavpreetDevpuri
Copy link
Contributor Author

NavpreetDevpuri commented Aug 3, 2020

I tested it and it works fine for me. But i am using 3b1b/manim
Here are some outputs

Code

class Test6(Scene):
    def construct(self):
        t = Paragraph('this is a awesome', 'paragraph', 'With \nNewlines', '\tWith Tabs', '  With Spaces',
                      'With Alignments',
                      'center', "left", "right", line_spacing=0.3, size=1, alignment="left", t2c={"Tabs": RED})
        self.play(t.shift, RIGHT)
        t.__len__()
        self.play(t.set_color, RED)
        t.set_line_alignment("left", 8)
        t.set_line_alignment("right", 9)
        self.play(t.set_all_lines_alignments, "right")
        t[0][5].set_color(GREEN)
        t[0][6].set_color(GREEN)
        t[1][0:4].set_color(YELLOW)
        rect = SurroundingRectangle(t)
        self.add(t, rect)
        self.wait()

Output

Test6

Code

class test15(Scene):
    def construct(self):
        helloworldc = Code(
            "helloworldc.c",
            tab_width=4,
            background_stroke_width=1,
            background_stroke_color=WHITE,
            insert_line_no=True,
            style=Code.styles_list[4],
            background="window",
        )
        helloworldcpp = Code(
            "helloworldcpp.cpp",
            tab_width=4,
            background_stroke_width=1,
            background_stroke_color=WHITE,
            insert_line_no=True,
            style=Code.styles_list[15],
            background="window",
        )
        helloworldc.move_to(np.array([-3.6, 0, 0]))
        helloworldcpp.move_to(np.array([3.1, 0, 0]))
        self.add(helloworldc)
        self.play(Transform(remove_invisible_chars(helloworldc),
                            remove_invisible_chars(helloworldcpp)))
        helloworldc[2][0:4].set_color(RED)
        self.play(Transform(helloworldc[0], helloworldcpp[0]),
                  Transform(helloworldc[1], helloworldcpp[1]),
                  Transform(helloworldc[2], helloworldcpp[2]))
        self.wait()

Output

test15

@NavpreetDevpuri
Copy link
Contributor Author

NavpreetDevpuri commented Aug 3, 2020

I am correcting some bugs so that it also works for manim-community.
In CodeMobject.py file, Should i check for file in assets\codes\ ? Or should i just need to ask user to specify a direct link to file ?

Code_YFF2g0itOV

Is following correct ?
AnynaCrthe

@NavpreetDevpuri
Copy link
Contributor Author

NavpreetDevpuri commented Aug 3, 2020

You guys removed conts from Constants.py !
Let me correct text_mobject.py
Code_gOda9Moy1j

@Aathish04
Copy link
Member

I am correcting some bugs so that it also works for manim-community.
In CodeMobject.py file, Should i check for file in assets\codes\ ? Or should i just need to ask user to specify a direct link to file ?

What asset is Code() looking for? In general, I think it's better to use the assets directory like you have done here, but others may have some other thoughts...

@NavpreetDevpuri
Copy link
Contributor Author

NavpreetDevpuri commented Aug 3, 2020

What asset is Code() looking for? In general, I think it's better to use the assets directory like you have done here, but others may have some other thoughts...

Its looking for specified file helloworldc = Code("helloworldc.c",...etc. The way i did that so, we can specify a direct link to file or just a name of the file that we already copied in assets\codes

@NavpreetDevpuri
Copy link
Contributor Author

NavpreetDevpuri commented Aug 3, 2020

Now tested with manim-community and it works fine 😃

@leotrs
Copy link
Contributor

leotrs commented Aug 3, 2020

@NavpreetDevpuri thank you for the update. The tests are failing. Could you please check to see what's the problem?

@NavpreetDevpuri
Copy link
Contributor Author

NavpreetDevpuri commented Sep 2, 2020

I reformated all files locally and when I run command git status it shows no differences!

@NavpreetDevpuri
Copy link
Contributor Author

NavpreetDevpuri commented Sep 2, 2020

I tried to reformate this file with black in visual studio and it does not change according to what these checks suggesting
and I cross-checked it's black formatter
chrome_Xckxq7IBg1

@leotrs
Copy link
Contributor

leotrs commented Sep 2, 2020

I tried to reformate this file with black in visual studio and it does not change according to what these checks suggesting

Hi @NavpreetDevpuri thanks for looking into this again :) Have you made sure to update your version of black? It recently changed from version 19 to version 20. Also, don't worry too much about the black check, it has been failing for a few days now.

If you do find you have to update your black version PLEASE do not run black on the whole codebase and push those changes on this PR. If necessary, run black only on the files you are editing on this PR.

@Aathish04 @huguesdevimeux please merge this even without black check passing :)

@huguesdevimeux
Copy link
Member

Yes, black tests failing are ok for now as this will be changed soon.

Let me know when you think you're done with this PR. We are just waiting @PgBiel aproval, and then we can merge !

@NavpreetDevpuri
Copy link
Contributor Author

NavpreetDevpuri commented Sep 2, 2020

I did my best. I checked/tested/changed/rechecked/tested... until I feel That's almost perfect

@Aathish04 Aathish04 marked this pull request as ready for review September 3, 2020 03:26
@Aathish04
Copy link
Member

Marking as ready for review!

@Aathish04 Aathish04 dismissed their stale review September 3, 2020 05:58

Review Outdated.

Copy link
Member

@PgBiel PgBiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still a long way to go regarding docs. Please read the guidelines in our Wiki.

from pygments.formatters.html import HtmlFormatter
from pygments.styles import get_all_styles

"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be in Code's documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I added it to the documentation but I did not, because this seems like Extra?

@huguesdevimeux huguesdevimeux added Changes requested and not yet done and removed test requested Implementation of tests are requested labels Sep 9, 2020
@leotrs
Copy link
Contributor

leotrs commented Sep 15, 2020

This has been open for 2 months and the only thing that seems to be blocking the final merge is documentation. @NavpreetDevpuri do you think you have time to work on the documentation suggestions left by @PgBiel ?

Otherwise, we should just merge as is and worry about documenting this module later.

@NavpreetDevpuri
Copy link
Contributor Author

NavpreetDevpuri commented Sep 15, 2020

@NavpreetDevpuri do you think you have time to work on the documentation suggestions left by @PgBiel ?

I was busy with something. I am a little weak in the documentation, I mean it takes more time for me to do simple documentations

let me try.

Copy link
Contributor Author

@NavpreetDevpuri NavpreetDevpuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been open for 2 months and the only thing that seems to be blocking the final merge is documentation. @NavpreetDevpuri do you think you have time to work on the documentation suggestions left by @PgBiel ?

Otherwise, we should just merge as-is and worry about documenting this module later.

I did almost what I can do. so, we should merge it after a little review. we can improve documentation in other PR. this PR created mainly for fixing very big bugs.

from pygments.formatters.html import HtmlFormatter
from pygments.styles import get_all_styles

"""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I added it to the documentation but I did not, because this seems like Extra?

@leotrs
Copy link
Contributor

leotrs commented Sep 15, 2020

Thanks @NavpreetDevpuri for doing what you can about the documentation. There is just one instance that I would like to have sorted out and then we can merge this. See my comments above.

@leotrs
Copy link
Contributor

leotrs commented Sep 15, 2020

I see you removed that last thing - is there anything else you wish to do before we merge this? @NavpreetDevpuri

@NavpreetDevpuri
Copy link
Contributor Author

@leotrs sir, all clear for now.

@leotrs leotrs merged commit 1f02ae0 into ManimCommunity:master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement Additions and improvements in general pr:bugfix Bug fix for use in PRs solving a specific issue:bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some bugs of code_mobject.py

6 participants