Skip to content

Conversation

@jackiekazil
Copy link
Member

No description provided.

@jackiekazil
Copy link
Member Author

Looking for someone to do a review.

@coveralls
Copy link

coveralls commented Nov 12, 2017

Coverage Status

Coverage increased (+0.05%) to 82.177% when pulling d1fbc82 on 410_fix_distance into 275674d on master.

@JamesArruda
Copy link
Contributor

JamesArruda commented Nov 15, 2017

Looks good. One suggestion is to put the second distance calculation and the min statement inside the if self.torus: block to avoid an excess linalg.norm call.

Does this change how get_heading will behave? Should that heading be based on the minimum distance, too?

EDIT: I was doing some testing and came to the conclusion that the modding usually works for the torus distance, but there are cases where it won't work. See this comment: #410 (comment)

@Corvince
Copy link
Contributor

Corvince commented Dec 7, 2017

Closing this in favor of #430

@Corvince Corvince closed this Dec 7, 2017
@jackiekazil jackiekazil deleted the 410_fix_distance branch January 7, 2018 21:58
@jackiekazil jackiekazil added this to the Unknown milestone milestone Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants