-
Notifications
You must be signed in to change notification settings - Fork 118
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
New class for camera selector button & use in Profile Wizard #1286
base: master
Are you sure you want to change the base?
New class for camera selector button & use in Profile Wizard #1286
Conversation
af40f01
to
358042f
Compare
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.
If you intend to incorporate this into the ProfileWizard, you need to be sure it works in all contexts. For example, the profile may be constructed with no gear attached at all. Further, the existing camera-selector code presumes use of a profile but there won't be one in the context of the wizard - or it will be the wrong one. So that will need some other form of dictionary-like temporary storage so that the user can move back and forward in the Wizard, change his mind, but not lose work. This isn't really a copy and paste kind of project and has so-far demanded more time and attention than either Andy or I wanted to invest in it -especially since there is an easy work-around. It can surely be done and would be a nice addition, but it's going to take some attention to details.
This is why I am hoping to have the new class reviewed and merged first instead of lumping it all in one PR. Separating the button into a new class introduced issues in the gear dialog that needed to be resolved first (i.e. selected camera model not changing when the driver changed, so it could try connecting to a non-existent Player One ASI120MM or ZWO Ceres 462M). I believe they are all ironed out. Working on the profile wizard part on profile-wizard-cam-select-btn. To briefly summarize, it seems to work as intended and not affect other profiles so far. Still testing various scenarios and may have to make further changes if any are made to the new button class during this review. |
In your newly created class, it seems to me that any reference to "pConfig->Profile" is a problem if the class is going to be used by the Profile Wizard. That reference is to the already loaded profile not to the profile you are trying to build. There is no "new profile" at this point. It's not a trivial problem. Andy and I are about to do another dev release around the end of the week, so I'd like to see this wizard-related stuff put on the shelf until we can get through with that and until a better approach can be worked out. FWIW, I think it will generally work better if you make contact ahead of time and discuss proposed changes before plunging into coding. For example, if I had known this was in the pipeline, I wouldn't have just spent the time to rework the Help content for using multiple cameras from the same manufacturer. |
I am back to working on this. Since the discussion is revolving around the end goal of using the button in the Profile Wizard, I've merged the work for that into this branch rather than opening a separate PR after this one. The button works for me as-is in Profile Wizard since the AutoTempProfile used by it becomes pConfig while the wizard is open. The AutoTempProfile constructor has an init parameter which determines whether the temp profile is set to pConfig. As far as I can tell, this is always true in practice. I can move the code for setting the camera model from the button's class to the parent dialogs if desired. This is how it was originally being implemented it until realizing it wasn't necessary to work, so for now I believe the end result would be duplicate code. |
I’ll try to take a look at this soon, maybe over the weekend.
Bruce
From: Ethan Chappel ***@***.***>
Sent: Wednesday, January 22, 2025 3:18 PM
To: OpenPHDGuiding/phd2 ***@***.***>
Cc: bwdev01 ***@***.***>; Comment ***@***.***>
Subject: Re: [OpenPHDGuiding/phd2] New class for camera selector button & use in Profile Wizard (PR #1286)
I am back to working on this.
Since the discussion is revolving around the end goal of using the button in the Profile Wizard, I've merged the work for that into this branch rather than opening a separate PR after this one.
The button works for me as-is in Profile Wizard since the AutoTempProfile used by it becomes pConfig while the wizard is open. The AutoTempProfile constructor <https://github.com/OpenPHDGuiding/phd2/blob/0927de6c8943fae7161457008b989bf72a05c638/src/phdconfig.cpp#L901> has an init parameter which determines whether the temp profile is set to pConfig. As far as I can tell, this is always true in practice.
I can move the code for setting the camera model from the button's class to the parent dialogs if desired. This is how it was originally being implemented it until realizing it wasn't necessary to work, so for now I believe the end result would be duplicate code.
—
Reply to this email directly, view it on GitHub <#1286 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADDHSV7FMNMUKPOCP35OPK32MARLTAVCNFSM6AAAAABUU7LAC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMBYGQ3DKMBTGM> .
You are receiving this because you commented. <https://github.com/notifications/beacon/ADDHSV5NNHBHFBPVBZBUL232MARLTA5CNFSM6AAAAABUU7LAC6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTU3PICIS.gif> Message ID: ***@***.*** ***@***.***> >
|
I just cloned your github branch and I can't get the project to build. I've confirmed that the source tree from the PHD2 repo builds on this system so the dev environment is correct. The build for the clone of your branch is failing on the ogma camera sdk code which is a recent addition to the master branch. Could you please update your branch and confirm that it will build correctly from the run_cmake.bat step. Thanks. |
Moves much of the code for the camera selector button out of the gear dialog into a new class. Does not change anything from the user's perspective, but would like to use this new class to add the button to Profile Wizard so we no longer have to hope the right camera is selected if multiple using the same driver are attached to the PC. Most of the work is done there too, but I plan to finish and submit after this pull request is finalized and merged.
Tested on Windows 11 with one ZWO and two Player One cameras.