-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Added BraceBetweenPoints and some documentation #693
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
Conversation
leotrs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few non-blocking nitpicks.
As always, a quick graphic test here would be great.
Co-authored-by: Leo Torres <[email protected]>
Co-authored-by: Benjamin Hackl <[email protected]>
|
NumperPlane... oof. I'll look into tests for this after I tried sleeping, again. |
|
@leotrs I only now noticed that your suggestion sneakily killed off the wait statement I put into the example. I'm not sure if we have a policy on ending wait statements or not, but I'm personally not a fan of not having them. Whether I make examples or program something new, I find myself more often than not wanting to have a good look at the final state. The examples also just seem noisy when they don't stop briefly at what they're actually meant to show. Although at least the video player in the docs seems to allow easily enough to capture that last frame without a wait statement, so I suppose it's not that much of an issue here. |
|
Oh I didn't mean to be sneaky 😅 ! We don't have a consensus on this, I was just doing it out of personal preference, so I will lead the final decision to you. I think I originally deleted it because I thought the example's output was a single frame, in which case the wait is unnecessary. |
Well the other example has a wait statement too, and they're both animated, albeit very briefly. But yes, if it would've been a single frame then there definitely shouldn't be a wait statement. I'll re-add it then. Edit: With that said, something about the documentation is still failing. I reckon my attempt at chaining union and optional isn't quite working out. Not sure what to do about that. |
What do you mean? All RTD build processes were successful since 1e177cd. |
This garbled mess: From this code To be seen here: https://manimce--693.org.readthedocs.build/en/693/reference/manim.mobject.svg.brace.BraceBetweeenPoints.html#manim.mobject.svg.brace.BraceBetweeenPoints |
If I had to guess, the correct notation should be If you would like help by your IDE with writing these kind of things, you might be interested in writing it as a type hint, i.e., from typing import Union, Optional
def __init__(self, point_1: Union[list, np.array], point_2: Union[list, np.array], direction: Optional[Union[list, np.array]] = ORIGIN, **kwargs):Then (theoretically, but I am somewhat sure) the types also show up in the documentation (and you can leave out the part starting at the colon from the (But again, I might be wrong that it works like that automatically. If so, this would still be the desired behavior, but we should try to get it to work on a separate PR.) |
behackl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor suggestions to improve formatting, then it can be merged.
Co-authored-by: Benjamin Hackl <[email protected]>
Co-authored-by: Benjamin Hackl <[email protected]>
|
I found a pretty terrible typo myself as well. I do suppose this is ready for merging now. Since leotrs said the test issue is non blocking I will take care of the tests needed for my submissions later in one swoop. |

Partial implementation of #210
Added BraceBetweenPoints to brace.py
Also added documentation and examples both to the new BraceBetweenPoints as well as the old Brace.