Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing a broken stroke width for ZoomedScene #1073

Closed
wants to merge 1 commit into from

Conversation

ksofiyuk
Copy link

@ksofiyuk ksofiyuk commented May 15, 2020

When using ZoomedScene, incorrect stroke width behavior is observed.

I found that a special multiplier self.get_frame_width() / FRAME_WIDTH was added, which is supposed to keep the right stroke width. In fact, it introduces an incorrect behavior. See the example below:

before
after

Reproducible example:

class Test(ZoomedScene):
    CONFIG = {
        "zoom_factor": 0.4,
        "zoomed_display_height": 5,
        "zoomed_display_width": 5
    }

    def construct(self):
        zoomed_camera = self.zoomed_camera
        zoomed_display = self.zoomed_display

        title = TextMobject("Before").scale(2.5).to_corner(LEFT + UP)

        zoomed_display.display_frame.set_color(BLUE)
        zoomed_display.to_corner(RIGHT + UP, buff=0)

        circle1 = Circle(stroke_width=6).scale(0.5)
        circle2 = Circle(stroke_width=3, color=BLUE).scale(0.25)
        rect = Rectangle(height=1.25, width=2.0, color=YELLOW, stroke_width=2).next_to(circle2, LEFT, buff=0)
        dot = Dot(fill_color=BLUE, color=WHITE, stroke_width=4).scale(2).next_to(circle1, LEFT, buff=0)
        zoomed_camera.frame.next_to(rect, LEFT)

        self.add(circle1, circle2, rect, dot, title)
        self.activate_zooming()
        self.play(self.get_zoomed_display_pop_out_animation())

        zoomed_camera.frame.generate_target().move_to(circle1)
        self.play(
            MoveToTarget(zoomed_camera.frame),
            run_time=6
        )
        self.wait(2)

I understand that the author of this code fragment originally wanted to keep the thickness of the lines in ZoomedScene the same as in the original scene. First of all, I think that this is an unexpected behavior as it is normal to expect a thickening of lines when zooming objects. Secondly, the originally proposed hack does not work, because the width and height of the scenes in pixels are different, so simple multiplication by self.get_frame_width() / FRAME_WIDTH does not restore the original stroke width. If someone wants to keep this behavior, it should be made optional.

@ksofiyuk
Copy link
Author

It fixes this issue #675

@ksofiyuk
Copy link
Author

ksofiyuk commented May 16, 2020

Additional clarifications:
The old multiplier self.get_frame_width() / FRAME_WIDTH is superfluous because the zoom scale is already reflected in the get_cairo_context method by setting transformation matrix in L297 with (pw/fw, ph/fh) scale.

I understand that the author of this code fragment originally wanted to keep the thickness of the lines in ZoomedScene the same as in the original scene. First of all, I think that this is an unexpected behavior as it is normal to expect a thickening of lines when zooming objects. Secondly, the originally proposed hack does not work, because the width and height of the scenes in pixels are different, so simple multiplication by self.get_frame_width() / FRAME_WIDTH does not restore the original stroke width. If someone wants to keep this behavior, it should be made optional.

P.S. I would also like to point out that line 296 with ctx.scale(pw, ph) is redundant, as the line following it sets a new transformation matrix, resetting the previous scale.

@NavpreetDevpuri
Copy link
Contributor

NavpreetDevpuri commented May 16, 2020

I suggest, make it optional something like if constant_line_width == True then ensures lines have constant width otherwise make lines looks normal.

@NavpreetDevpuri
Copy link
Contributor

NavpreetDevpuri commented May 16, 2020

May be add constant_line_width in CONFIG
and set its default value to False

@ksofiyuk
Copy link
Author

ksofiyuk commented May 16, 2020

This would be good, however, the current design does not allow this behavior to be easily implemented (with constant line width).

Let's take a look at the reproducible example (rendered with a low quality config):

  1. Main camera has pixel_height = 480 and frame_height = 7.994.
  2. Zoomed display camera has pixel_height = 300 and frame_height = 2.
  3. Transformation matrix height scale for the main camera is 480 / 7.994 = 60.04.
  4. Transformation matrix height scale for the zoomed camera is 300 / 2 = 150.
  5. If we multiply zoomed camera width by self.get_frame_height() / FRAME_HEIGHT = 2 / 7.994 = 0.063, we'll get a new width scale 150 * 0.063 = 9.49, which is completely wrong.

The main problem is that the zoomed display camera does not know the width and height of the main camera in pixels. So the ratio of pixel_height/frame_height is completely different for the main camera and the zoomed camera.

@ksofiyuk
Copy link
Author

The current solution, in which the stroke width is multiplied by self.get_frame_height() / FRAME_HEIGHT, is wrong and does not provide width consistency due to the reasons described above.

Width constancy can be implemented. The correct solution is to multiply stroke width in zoomed camera by zoom_factor, but to do so we have to throw zoom_factor into the zoomed camera class, which will require a change in the interface of the camera class.

@TonyCrane
Copy link
Collaborator

Sorry, but this seems outdated, so closed this.

@TonyCrane TonyCrane closed this Feb 10, 2021
eulertour pushed a commit to eulertour/manim-3b1b that referenced this pull request Mar 16, 2021
* Update PULL_REQUEST_TEMPLATE.md

* Improved intro section

* ENH: Model acronyms after NumPy

* MNT: Fixed tense of thank you message

* Removed mention of acronyms in PR titles

* Update .github/PULL_REQUEST_TEMPLATE.md

Co-authored-by: Benjamin Hackl <[email protected]>

* Update .github/PULL_REQUEST_TEMPLATE.md

Co-authored-by: Benjamin Hackl <[email protected]>

* removed wikilink from changelog.rst

Co-authored-by: Jason Villanueva <[email protected]>
Co-authored-by: Benjamin Hackl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants