-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fixed Arrow3D.put_start_and_end_on()
to use the actual end of the arrow
#3706
Merged
chopan050
merged 11 commits into
ManimCommunity:main
from
SirJamesClarkMaxwell:Arrow-3D-put-start-and-end
Jun 26, 2024
Merged
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
ef4fbc1
my test is not passing, i need to add a little bit of docs. except th…
SirJamesClarkMaxwell 50bef62
fixed the issue #3655
SirJamesClarkMaxwell 9ff95fd
removed comments
SirJamesClarkMaxwell da8fb2b
fix: 3706 original issue, without adding unnecessary dot
SirJamesClarkMaxwell 90fd3ad
Changes that way how end point of Arrow3D is calculated.
SirJamesClarkMaxwell 39042ca
Merge branch 'main' into Arrow-3D-put-start-and-end
SirJamesClarkMaxwell 13e31c2
I've improved the methods get_start and get_end for the Cone class, a…
SirJamesClarkMaxwell 85941b4
Merge branch 'main' into Arrow-3D-put-start-and-end
SirJamesClarkMaxwell ddcd511
Finished! Replaced VMobject by VectorizedPoint as Ben suggested while…
SirJamesClarkMaxwell 1b678b1
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] bbd0fa4
Merge branch 'main' into Arrow-3D-put-start-and-end
chopan050 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can the tripled reference to
start
andend
cause any harm here? (Modifying any of the entries will modify all three of them in the same way.)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.
I think that it must be the same point three times. Otherwise we will get a 3d curve wich would be visible, and we don't want this. My solution based on merge #3358 or #3718. I am not sure why we might have problem here, because we are calculating everything internally based on user start and end points. Maybe I missunderstood you question
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.
I believe he is talking about this behavior
Which results in
Because
y
simply stores a list of 3 "pointers" tox
.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.
Exactly, this is what I was referring to!
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.
Okay, but should we care about it? They are local variables, as long as direction, get_center and new_height will return propper types we will not have a problem here. I could add type checking but I am not sure that we need it here. Why? Because if direction won't have a proper type cone and cilinder will through the error same for height, if get_center will have invalid type all manim will be broken. I could add it but IMO if the type of the variables used in this method other part of the code will through the error ealier.
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.
I will push today the VectorizedPoint version.
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.
Is there any update on this? It would be nice to have it merged before the v0.19.0 release :)
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.
I had problems with my laptop (and I was finishing my 4th physic semester)
I will push tuday the newest version of the code
Sorry
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.
i just switched to new PC, i will install manim tomorrow and then i will add changes -> push.|
For some reason the changes which i made, wasn't saved, i have no idea why
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.
I have seen the changes, thanks!