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

Extending PHD2 for Planetary Guiding #1187

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Eyeke2
Copy link
Contributor

@Eyeke2 Eyeke2 commented Apr 15, 2024

After several months of testing with a community of beta testers and successful real-world use for imaging solar eclipse and extended solar activity time lapses, this PR, consisting of a compact set of commits, is ready to be integrated into the main PHD2 codebase. This code has been developed to support solar guiding, enabling the creation of long time lapses of solar activity, as well as guiding on planets or the Moon. These celestial bodies typically appear larger than guiding stars and can also present as crescent shapes. The new algorithm I developed performs well in various conditions, maintaining effectiveness even under partial cloud cover or other obstructions, or when the crescent shape is very thin.

The PR includes changes to the Star Profile display and SNR evaluation, replacing HFD metrics with detected planetary radius or image sharpness measurements, which are useful for the planetary guiding workflow.

The set of commits includes updates to the Gear Simulator code, which was instrumental for testing the new Planetary Guiding tool. This updated simulator can now be used for enhanced testing, allowing users to run PHD2 calibration and guide on stars or planets. It also enables users to test the new Planetary Guiding functionality and practice without the need to connect any physical gear.

Although comprehensive documentation is still in development, I've created Wiki pages for new users and designed tooltips in the new UI to provide interactive help information and explanations. The UI for the new tool is designed to be simple and intuitive.

For planetary guiding, typical RMS values may be slightly higher than when guiding on stars. However, the very short exposure times should mitigate any issues. If necessary, performance can be further enhanced by opting for low pass guiding algorithms.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

Adding reference to Wiki pages of the project (surface feature detection has been excluded from this PR, as it needs more testing).

https://github.com/Eyeke2/phd2.planetary/wiki

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

Adding few related screenshots
image
image
image
image

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

Few sections of a presentation given at Memphis Astronomical Society (MAS) by Brent Ellis shortly after the great solar eclipse, where he talks about his experience with Planetary Guiding tool (I renamed it recently from Planetary Tracking to Guiding, as it seems more appropriate):

User experience
Short introduction

@pchev
Copy link
Contributor

pchev commented Apr 15, 2024

This look interesting, but for me it not compile on Linux because of opencv.

/home/pch/source/phd2/phd2/guider_planetary.h:37:10: fatal error: opencv/cv.h: No such file or directory
37 | #include "opencv/cv.h"

I not have cv.h anywhere in my system despite libopencv-dev is installed.
But I have /usr/include/opencv4/opencv2/opencv.hpp
Sorry I have no experience with opencv development.

But is it really required?
At the moment phd2 use it only for cam_opencv.cpp that is used only on Windows.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

I should note that this tool has been developed and compiled only for the Windows platform. I would appreciate assistance from other developers in adapting it for other platforms, including Linux and Mac, which I currently do not have access to.

@pchev
Copy link
Contributor

pchev commented Apr 15, 2024

In this case you must add conditional to compile it only on Windows and not make the phd2 compilation to crash on Linux.

@pchev
Copy link
Contributor

pchev commented Apr 15, 2024

I find a bit more on this subject.
It look like the opencv C API (cv.h) was depreciated long ago and is totally removed since the version 4 that is installed on Linux now.
This is replaced by a C++ API (opencv.hpp), but this probably need change to the code.

The easier solution for now is to compile your new code only on Windows since you can probably still use a previous version of opencv in VS.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

In this case you must add conditional to compile it only on Windows and not make the phd2 compilation to crash on Linux.

That can certainly be done, but unless it's proven that it cannot run on Linux, I'd prefer to explore how we might adapt the code to compile on different platforms, if feasible.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

@pchev answering your question - yes, opencv is required by this extension, but depending on the platform it may require some adaptation, which I'm not able to test and verify at this time.

@pchev
Copy link
Contributor

pchev commented Apr 15, 2024

OK, I also prefer if we can make it to work on Linux and Mac, but I don't know how much of the code use opencv and what effort this require.

Because I don't know anything about opencv I start to read this page about VS:
https://docs.opencv.org/4.x/dd/d6e/tutorial_windows_visual_studio_opencv.html

There is an example code in the section "Test it!"
I put this code in a file test.cpp, create a cmake based on the gcc tutorial of this same site.
It compile and work fine.

So making the same opencv code to work on Windows and Linux is not a problem, the problem is only the removal of the C API you use in opencv version higher than 4.

If you can upgrade your code to use the C++ API, like the example I mention above is using, it will work on any platform that use opencv version 4.
This is probably a good change also for Windows, in the case the old version you actually use get no more supported.

@d33psky
Copy link
Member

d33psky commented Apr 15, 2024

Hey this is great work !
I assume the best way forward is for Eyeke2 to upgrade OpenCV to use the newer opencv.hpp too.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

I may have been premature in creating this PR; it would have been more appropriate as a code review or a request for assistance to ensure compilation and functionality under Linux and OSX. I must acknowledge my oversight in not consulting the build instructions for Linux or OSX. I incorrectly assumed, without verification, that these platforms would use the same OpenCV 2.4 version as Windows. While updating to newer versions of OpenCV is an option, it would necessitate changes to the source code due to the evolution of the OpenCV API since version 2.4. If feasible, deploying the OpenCV 2.4 package on Linux or OSX would be my preferred approach.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

I should clarify that I was building on the existing OpenCV 2.4 library already integrated into PHD2 for Windows. I haven't introduced a new library; I simply utilized the existing framework.

@pchev
Copy link
Contributor

pchev commented Apr 15, 2024

It is not possible to include an old version of opencv with phd2 on Linux because the build is different on every distribution version depending on the opencv own requirement.

Can you try if the example code I mention above work with opencv 2.4 in your environment?
In this case this will greatly simplify the update.

Normally the C++ API is available in version 2 and deprecated after version 2.4.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

@pchev , thank you for the suggestion. I can attempt to remove including "opencv/cv.h" and transition from C-style to the C++ API while continuing to use OpenCV 2.4. However, I'm not sure I fully understand your point. If you're saying that OpenCV 2.4 isn't available for all Linux distributions and versions, why should we continue using it?

@agalasso
Copy link
Contributor

agalasso commented Apr 15, 2024

I would be in favor of updating the Windows OpenCV to to opencv4 so that all the platforms are up to date with the most recent opencv major version. Ideally this would be a separate PR that would merge to master before this pr.

@Eyeke2 would you like to work on this or would you like me to do it? I would not mind doing it, but if you want to take it that is fine too.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

@agalasso I'm not very familiar with the PHD2 build system, which involves git cloning and integrating external components. I would greatly appreciate it if you could update PHD2 with the latest version of OpenCV. Once updated, I'll take it from there, rewriting some sections of the planetary guiding code to adapt it to the latest OpenCV API.

@pchev
Copy link
Contributor

pchev commented Apr 15, 2024

@agalasso , upgrading to opencv4 would be ideal, but I fear this break cam_opencv that use the old API.
I don't know if it can be fixed.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

@pchev @agalasso I assume that PHD2 used OpenCV solely because of cam_opencv. Does this setup compile under Linux/OS X, or is it only functional on Windows? If cam_opencv cannot be updated to the newer version of OpenCV, we might consider using two different versions of OpenCV simultaneously, since the binary library filenames differ and the include files can be installed in separate directories.

@agalasso
Copy link
Contributor

let's not go there -- better to keep all platforms synchronized as much as possible. I'll put up a pr shortly upgrading to opencv4 (including whatever changes are needed for cam_opencv)

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 15, 2024

@agalasso Certainly, that's a preferable approach. Using two different versions simultaneously can be challenging because of potential namespace collisions.

@agalasso
Copy link
Contributor

#1188 updates opencv

Copy link
Contributor

@agalasso agalasso left a comment

Choose a reason for hiding this comment

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

Leo, nice work on this feature!
It will be great to get this merged, but this pr needs to be split into more digestable pieces that can be reviewed and merged separately. There's too much code change in this pr for us to review with confidence in a reasonable amount of time.
Some possible pieces that could be separated out into individual prs:

  • whitespace cleanup -- make a dedicated pr for that; that alone will reduce the number of files in this pr
  • cleanup of spelling errors in comments
  • adding comments to existing code
  • ascom changes
  • simulator changes
  • lots of independent changes like the change in camera.cpp could be standalone prs

If you need more detailed suggestions on what could or should be a good standalone pr, let me know and we can discuss further. Ideally each pr should be small and focsued enough that the other developers can look at it in a short amount of time (minutes, not hours) and conclude that it is a good, safe code improvement or extension. You can find articles online that talk about how to make small, reviewable prs (and why it is important); I can also elaborate if you need more info.

* Copyright (c) 2006-2010 Craig Stark
* Copyright (c) 2015-2018 Andy Galasso
* Copyright (c) 2023-2024 Leo Shatz
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not doing copyrights for individual developers any more. All new code should be

Suggested change
* Copyright (c) 2023-2024 Leo Shatz
* Copyright (c) 2024 openphdguiding.org

We want to give developers due credit and always add developers to the list of contributors in Help>About
But having specific copyright notices from every contributor is not scalable and makes it very difficult to follow the BSD 3-clause license which requires the copyright statement be repeated in all software distributions.
I have slowly been converting all the copyright statements that have my name to the form above (openphdguiding.org), and requesting that contributors do the same.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 16, 2024

@agalasso Thanks! I'm ok with creating several PRs for minor updates, but for the core algorithm used in detection and its UI, I need to know if I should also divide them into smaller parts. Deconstructing them could be challenging. My plan was to make a single commit that includes the complete code of the new modules with minimal integration into the existing PHD2 framework, followed by a series of commits that fully integrate the new tool with the rest of the system. Regarding copyright, I want to ensure that intellectual property is maximally protected under the current BSD license, especially for the core module containing the detection algorithm.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 18, 2024

@agalasso thanks for upgrading to opencv4. I've converted the planetary code to use the new API while I'm still working on more internal testing and code refactoring, so new PRs are few days away. So far looks good.

@pchev
Copy link
Contributor

pchev commented Apr 18, 2024

@Eyeke2 , please tell me when you have a branch I can try with Linux.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 19, 2024

@pchev I have rebased this branch to latest master with opencv4 and updated planetary code to use the C++ API. You can try it now with Linux. I still have to break it into smaller PRs as Andy had suggested.

@pchev
Copy link
Contributor

pchev commented Apr 20, 2024

Thank you! it work on Linux now.

I have to make two small change to complete the compilation.
I include the patch for the two change here, but tell me if you prefer a PR in your branch.

  • In guider_planetary the macro ARRAYSIZE is not defined with my compiler.
    A solution can be to use a constant=2000 for the size of gaussianWeight because it not change.
    Or add the macro definition to guider_planetary.h :

diff --git a/guider_planetary.h b/guider_planetary.h
index 03a0e5cf..4aaef998 100644
--- a/guider_planetary.h
+++ b/guider_planetary.h
@@ -37,6 +37,10 @@
#include "opencv2/highgui.hpp"
#include "opencv2/imgproc.hpp"

+#if !defined(ARRAYSIZE)
+#define ARRAYSIZE(array) (sizeof(array) / sizeof(*array))
+#endif
+
// Planetary guiding/tracking state and control class
class GuiderPlanet
{

  • In planetary_tool.cpp it require to explicitly include tooltip.h :

diff --git a/planetary_tool.cpp b/planetary_tool.cpp
index 8c9c81f7..e663b470 100644
--- a/planetary_tool.cpp
+++ b/planetary_tool.cpp
@@ -36,6 +36,7 @@
#include "planetary_tool.h"

#include <wx/valnum.h>
+#include <wx/tooltip.h>

static bool pauseAlert = false;

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 20, 2024

@pchev Thank you for the fixes! It seems easier than I anticipated! We will need someone to test the planetary guiding on Linux. I'm not sure if the camera simulator is available for Linux, but if it is, it could be used for testing in simulator mode by loading an image of the Sun or Moon.

@pchev
Copy link
Contributor

pchev commented Apr 20, 2024

I tested with the simulator using different solar and planetary images.
I also tested with the INDI simulator using a large defocused star to simulated a planet.
In all the case it work without issue.
I am impressed how good it is to find the center of the partially eclipsed Sun!

The only thing that not work, but this is expected, is to modify the tracking rate from the planetary tool.
This is probably possible to add later using the corresponding INDI properties, but this is not critical because with INDI all the mount properties can already be modified by using the Settings button.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 20, 2024

@pchev that's wonderful to know. I haven't implemented tracking and its speeds for INDI mounts, as I'm less familiar with its APIs. There are really two options here - either disable these elements for Linux/MacOS platforms, or implement the relevant methods, as you have suggested. The idea of bringing these controls to planetary guiding tool is to streamline the user experience.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Apr 20, 2024

A good starting point might be to disable the mount control group for Linux/OSX in the initial release and consider adding it later, after merging with the master branch.

@pchev
Copy link
Contributor

pchev commented Apr 20, 2024

I compiled with your last change and it work fine.

At the moment the tracking rate option is disabled when connected to an Indi mount:
image

I think this is OK for now and we can add that later. This will not be a problem because Indi include a TELESCOPE_TRACK_RATE property with Sidereal, Lunar, Solar rates.

@Eyeke2 Eyeke2 force-pushed the planetary branch 2 times, most recently from 00b4ff1 to 7b8d4e8 Compare April 21, 2024 21:09
…comments

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
…bled

for the detection of large celestial bodies such as the Sun, Moon, and planets.
This feature detects them as either full light disks or crescent shapes
occurring during eclipses or various Moon phases. This commit lays the
foundation for subsequent updates needed to fully integrate this feature
into PHD2 and ensure its functionality.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
following changes to its elements, such as adding or removing icons.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
…ng mode

and integrate the tool with the PHD2 framework. Use the central point of the
detected planetary disk as a guide star. Update status messages for Planetary
Guiding mode.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
Planetary Guiding mode and replace HFD star metrics with planet radius
or sharpness, which are more relevant for planetary guiding.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
improper sizing of message alerts when the user changes the application
window size.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
Changes include making the simulator control dialog box non-modal, which
was essential for testing Planetary Guiding, and replacing hardcoded
simulation models with dropdown menus. These menus allow users to choose
between a star field generator or using file images in JPEG, TIFF, PNG,
or FIT formats. Additionally, a hardcoded option remains available under
DEVELOPER_MODE, while preserving all other legacy hardcoded options.
Other updates include adding a 'Simulate Mount Dynamics' mode, which enables
the use of PHD2 for calibration and guiding when selected while using
'Simulator' as the guiding camera. Drift simulation has also been added
for both RA and DEC, with an increased range of possible values and a
method to shift the input images by fractional number of pixels, aiding
in testing and evaluating the planetary guiding tool with images of
planets or other solar system bodies.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
…ighter.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
planetary guiding. These metrics include time spent in detecting the light
disk, circle matching score on a scale of 0-1, and the accuracy of the
detected position versus the actual one in simulator mode. This addition is
useful for testing and potentially tuning the planetary detection algorithm.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
…ll as

tracking speeds, including sidereal, solar, and lunar.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
…state and

select tracking speeds. This feature is currently limited to ASCOM mounts only.

Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
Signed-off-by: Leo Shatz <leonid.shatz@gmail.com>
@Yeqzids
Copy link

Yeqzids commented Sep 1, 2024

@Eyeke2 It seems v2.6.13dev5-planet.rc3 (on the v2.6.13-planetary branch) includes the depreciated cv.h again, so I cannot compile it under Linux. The planetary branch (rc1) still works.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Sep 1, 2024

I'm not sure why it's happening, but I haven't changed anything related to opencv since original move to OpenCV 4. Please try cloning the current branch head from https://github.com/Eyeke2/phd2.planetary.git and let me know if it works for you.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Sep 1, 2024

I suspect that the following commit may have an impact on Linux build, but I'm not sure: 193e8bc (OGMA cameras: update SDK to version 57.26291.20240811 (#1235)).

@Yeqzids
Copy link

Yeqzids commented Sep 1, 2024

I'm not sure why it's happening, but I haven't changed anything related to opencv since original move to OpenCV 4. Please try cloning the current branch head from https://github.com/Eyeke2/phd2.planetary.git and let me know if it works for you.

The master branch compiles without an issue but it's not the planetary version. I think the issue is that the v2.6.13-planetary branch calls cv.h https://github.com/Eyeke2/phd2.planetary/blob/d8f0a1d887713e1f1ed0c30beb3a699f1f1e26ed/planetary.h#L37 while the old planetary branch does not.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Sep 1, 2024

I'm not sure why it's happening, but I haven't changed anything related to opencv since original move to OpenCV 4. Please try cloning the current branch head from https://github.com/Eyeke2/phd2.planetary.git and let me know if it works for you.

The master branch compiles without an issue but I think it's not the planetary version. It seems the problematic file is planetary.h (which calls cv.h) which is in all active branches (rc2, rc3, v2.6.13-planetary) but not the outdated and staled planetary branch.

I'm definitely not referring to the master branch, it's irrelevant to this discussion. Please try cloning the current head branch (v2.6.13-planetary) and if that doesn't work, drop the suspected commit with OGMA SDK update, as I have stated earlier. I haven't changed any opencv headers for months.

@Yeqzids
Copy link

Yeqzids commented Sep 1, 2024

I'm not sure why it's happening, but I haven't changed anything related to opencv since original move to OpenCV 4. Please try cloning the current branch head from https://github.com/Eyeke2/phd2.planetary.git and let me know if it works for you.

The master branch compiles without an issue but I think it's not the planetary version. It seems the problematic file is planetary.h (which calls cv.h) which is in all active branches (rc2, rc3, v2.6.13-planetary) but not the outdated and staled planetary branch.

I'm definitely not referring to the master branch, it's irrelevant to this discussion. Please try cloning the current head branch (v2.6.13-planetary) and if that doesn't work, drop the suspected commit with OGMA SDK update, as I have stated earlier. I haven't changed any opencv headers for months.

No, neither of them works. (I updated my comment for clarity since I first posted it.) The error messages are identical to what @pchev had earlier:

[..]/phd2.planetary/planetary.h:37:10: fatal error: opencv/cv.h: No such file or directory
   37 | #include "opencv/cv.h"
      |          ^~~~~~~~~~~~~
compilation terminated.
make[2]: *** [CMakeFiles/phd2.dir/build.make:76: CMakeFiles/phd2.dir/mount.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:982: CMakeFiles/phd2.dir/all] Error 2
make: *** [Makefile:166: all] Error 2

I have installed libopencv-dev.

@Eyeke2
Copy link
Contributor Author

Eyeke2 commented Sep 1, 2024

I suppose there is a misunderstanding here. The branch 'v2.6.13-planetary' hasn't yet been adapted to use OpenCV 4, it's still using OpenCV 2, since parts of planetary detection algorithms are still using the old OpenCV API. In the meantime you can use the most recent branch with planetary tracking (without surface detection), which is 'solar.rc3'. I was mislead by your first comment, assuming that you've successfully compiled v2.6.13dev5-planet.rc1 under Linux, which is most probably not correct.

@Yeqzids
Copy link

Yeqzids commented Sep 1, 2024

Thanks @Eyeke2 , it makes sense now, and I can indeed get solar.rc3 to work.

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.

5 participants