Skip to content

Conversation

hydrobeam
Copy link
Member

@hydrobeam hydrobeam commented Oct 3, 2021

Overview: What does this pull request change?

  • Additional keyword arguments for :meth:~.Vector.coordinate_label are passed to the constructed matrix.
  • :class:~.Matrix now accepts a bracket_config keyword argument.

Motivation and Explanation: Why and how do your changes improve the library?

I noticed the docs weren't showing up for this method due to missing colons: ::. Also, the example wouldn't even render if it were available due to a typo, so that was fixed too.

I also decided to update the colour is set so that it's more compatible with #2075 if someone were to set Mobject.set_default(color=BLACK) for example. Also, more configuration was added for Matrix so that every part of the mobject can be modified as desired on creation. (Improved docs + shifted typing outside of docs too)

Links to added or changed documentation pages

Before: https://docs.manim.community/en/latest/reference/manim.mobject.geometry.Vector.html#manim.mobject.geometry.Vector.coordinate_label
After: https://manimce--2138.org.readthedocs.build/en/2138/reference/manim.mobject.geometry.Vector.html#vectorcoordinatelabel

Further Information and Comments

I think we should try to make a lot more of the library compatible with #2075, in particular by removing needlessly setting defaults like WHITE in child classes/methods, and instead letting the base `Mobject/VMobject handle those.

This does not apply for mobs with non-WHITE defaults.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

@hydrobeam hydrobeam added documentation Improvements or additions to documentation enhancement Additions and improvements in general labels Oct 3, 2021
@icedcoffeeee
Copy link
Collaborator

Just a comment: this was previously done in #1988 (there hasn't seem to be any further progress) but there was a problem with the coloring of the background rectangle. Based on how this is implemented, i'd assume it'd give the same issue.

@hydrobeam
Copy link
Member Author

Ah it seems like I missed that PR. It wasn't really the intended purpose to specifically allow for a background rectangle to be passed, but I guess this PR also handles that.

IMO, the way Matrix handles background rectangles is kind of weird. I think it'd make more sense to handle it explictly using its method, as opposed to using a parameter which then just calls the add_background_rectangle method. I think it'd be better if these parameters were removed tbh, since the method approach also allows more customization without introducing needless bloat. So even though this current approach doesn't mesh well with include_background_rectangle, that parameter doesn't need to exist, but it's not within the scope of this PR to remove.

Copy link
Collaborator

@icedcoffeeee icedcoffeeee left a comment

Choose a reason for hiding this comment

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

The Vector.coordinate_label lgtm, some minor comments of typehints of Matrix.

@hydrobeam hydrobeam changed the title Show example for :meth:~.Vector.coordinate_label and add more customization. Show example for :meth:~.Vector.coordinate_label and add more customization for :class:~.Matrix. Nov 1, 2021
@hydrobeam hydrobeam merged commit d7e7d82 into ManimCommunity:main Nov 1, 2021
@hydrobeam hydrobeam deleted the rework_coord_label branch November 1, 2021 17:05
@behackl behackl changed the title Show example for :meth:~.Vector.coordinate_label and add more customization for :class:~.Matrix. Fixed example for :meth:~.Vector.coordinate_label and added more customization for :class:~.Matrix Nov 1, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants