OpenPHDGuiding / phd2

PHD2 Guiding
https://openphdguiding.org
BSD 3-Clause "New" or "Revised" License
249 stars 114 forks source link

Bug fixes and minor optimizations #1160

Closed Eyeke2 closed 8 months ago

Eyeke2 commented 8 months ago

A set of small changes designed to fix issues with Star Profile display, improve image rescale speed, fix issues resulting from uninitialized variables, and minor speed optimization. Also adding protection against inadvertent change of cell size by the user in stats grids.

Eyeke2 commented 8 months ago

Looks good to me, minus having code in statswindow.h, but I suspect that others here would prefer this PR to be split up in smaller per-topic PRs.

Do you mean creating a PR for each commit, or trying to combine them by the subject? I'm working on a separate branch where I add planetary/surface tracking features, where I fixed few general issues and decided in the meantime to derive a smaller branch with miscellaneous fixes for immediate contribution. As for the stats Window - it's only an improvement and prevents cases when someone by accident moves a mouse pointer and drags a border of internal grid line, which leaves UI in an ugly state (with added side scrollbars). The fix is designed to keep the grid fixed after its initial layout.

Eyeke2 commented 8 months ago

I may consider to split into those into the following separate PRs:

d33psky commented 8 months ago

I meant by subject, yes. For easier tracking. But that's just my idea. Let's wait for others here what they think about per-subject PRs. Your fixes and code all make sense to me, so thanks for that. The only thing I'd change would be moving the code out of statswindow.h.

Eyeke2 commented 8 months ago

I meant by subject, yes. For easier tracking. But that's just my idea. Let's wait for others here what they think about per-subject PRs. Your fixes and code all make sense to me, so thanks for that. The only thing I'd change would be moving the code out of statswindow.h.

I got you, You probably meant to include it but to move the callback function to statswindow.cpp.

Eyeke2 commented 8 months ago

Regarding changes to Star Profile window as a stand alone PR - would you recommend to squash all relevant commits together or keep them as separate commits? Each of them are addressing different issues related to the Star Profile code.

Eyeke2 commented 8 months ago

@d33psky I have pushed the fix for keeping stats grids constant, as you have requested - moving the code of include file.

Eyeke2 commented 8 months ago

N.B. I just added a code for correct tuning of the scaling factor used for HFD information displayed in the Star Profile. Will consider adding it to PR.

bwdev01 commented 8 months ago

@Eyeke2. I haven't looked at the changes in detail but would like to be sure that whatever re-scaling you're doing in Star Profile doesn't break the "very large HFD display" behavior when the window is enlarged. People use that a lot when they're trying to focus the guide camera. Thanks.

Eyeke2 commented 8 months ago

@Eyeke2. I haven't looked at the changes in detail but would like to be sure that whatever re-scaling you're doing in Star Profile doesn't break the "very large HFD display" behavior when the window is enlarged. People use that a lot when they're trying to focus the guide camera. Thanks.

@agalasso, @bwdev01 I will split this PR into separate PRs, but try to keep series of commits in each PR to make code review easier. As for the HFD display - I haven't yet added this code to the suggested PR, it appears in my other branch but I will add it later to Star Profile PR. I also share your view about keeping HFD display as large as it was before the fix - as an old user of PHD2 I know exactly what you are referring to. I will provide more details and screen captures comparing before and after, following the new PRs I hope to release soon. Btw, one of my other commits fixes an annoying issue when HFD digits were sometimes getting "behind" the star display in the the profile window.

Eyeke2 commented 8 months ago

We can close this PR, as I just have created a series of topic-focused ones as you have suggested. Thank you all for the previous comments.