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

Add 3 pixels to the guide star selection edge padding #244

Merged
merged 4 commits into from
Feb 7, 2019
Merged

Conversation

jeanconn
Copy link
Contributor

Add 3 pixels to the guide star selection edge padding

@jeanconn
Copy link
Contributor Author

The different question is if we want to include the stars in the 3pix region in the candidates table (so they will go through the other checks). I don't know if it would be helpful to have debug information on them if we get desperate when looking for stars and trying to add one manually.

If we want to go that way, it seems like we'd leave the out-of-bounds check limits the way they were and then exclude the stars near the edge with a new check + logging of rejection reason etc.

@taldcroft
Copy link
Member

For now keep it simple. I almost even regret making the comment that resulted in the last commit. This patch needs to be pretty tight. And it should be against 4.3.x as a bug fix. We can do something similar against master but are a little less constrained.

@taldcroft taldcroft added this to the 4.4 milestone Feb 6, 2019
@taldcroft
Copy link
Member

... if we want to include the stars in the 3pix region in the candidates table

With a broader perspective now, I think the answer is a firm no. If there is a problem then the intent is for roll optimization to pick them up.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

@jeanconn - looks good to merge to me. If you have no further reservations then let's get this into master.

@jeanconn
Copy link
Contributor Author

jeanconn commented Feb 7, 2019

I just double-checked that this actually passes tests (it had been a while) and I think this is good.

@jeanconn jeanconn merged commit 69148da into master Feb 7, 2019
@jeanconn jeanconn deleted the guide_edges branch February 7, 2019 18:42
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.

2 participants