110 lesehest toggle zoom #121
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
currently toggles zoom in and out, had to change the implementation of toggle zoom inside recorder.py and rewrite the function. what it used to do in recorder.py will need to go in recorder_picamera.py because i had to have it just call the rewritten class in recorder_picamera2.py because picamera and picamera2 do zooming differently. also the user experience is a little different than before, with this one you see it zoom smoothly in and it takes about 1-2 seconds, but i think it makes it clearer what you're zooming in on, so i think its an improvement, but could also be changed to go back to just immediately zooming in and out.
had to change how recorder.py called toggle zoom because picamera and picamera2 zoom in different ways, and the current toggle zoom in recorder.py only worked with picamera. so the call was simplified in recorder.py, and recorder_picamera2.py and recorder_picamera.py had to have function definitions of their own that would toggle the zoom when called by recorder.py. i had made the function within recorder_picamera2.py but then to ensure recorder_picamera.py work I had to add back the code that used to make it toggle zoom in recorder.py. Hasnt yet been tested on old dencam so will need to be tested prior to a merge
need to test on lesehest again after changes. had to change how it got called in recorder_picamera.py because it wasn't redefining the toggle zoom function properly, I believe this will cause an issue with the lesehest version but that should be easily fixed by moving the zoom_toggle function to the class definition
I had to change the code slightly for it to work with the old code which broke the lesehest application for the toggle zoom so that is now fixed with the changes needed to make it run on fenrir staying untouched
ignored an unused argument in recorder_picamera2.py because it had to be passed from recorder.py as-is to not have to change recorder.py as much, but that means it goes unused in picamera2 for now. it's a config that picamera needs for recording but not picamera2 Resolves #110
it looked weird it shouldn't have had the extra -4 shift
it seems that the error that catches a camera not connected on fenrir is a runtime error, but on lesehest its an indexerror when this error was introduced, it crashed the program and the fix is to catch the specific error lesehest throws when the camera isn't connected, it now shows the proper error screen when the camera is disconnected on lesehest Resolves #128
This was
linked to
issues
Feb 3, 2025
In both of the zooming out implementation and the zooming in implementation there were a lot of the same function calls and it made sense to refactor this so that it is clearer what is common and what is different about the actions taken in the two states. Also, gave a magic number a name. Decided for now not to make this a module constant but just variable local to the method.
Refactor implementation of zoom toggling method in `recorder_picamera2.py` so that change to `recorder.py` is no longer necessary.
The removed method had been added when `recorder.py` had been modified to implement the zoom toggling differently. In the last commit, the zoom toggling in `recorder_picamera2.py` was implemented differently so the changes to `recorder.py` were no longer necessary hence this change to `recorder_picamera.py` is also no longer necessary.
…_zoom Refactor zoom toggling in `recorder_picamera2.py` to require fewer changes to other modules.
iingram
reviewed
Feb 5, 2025
| if not self.zoom_on: | ||
| size = [int(s * 0.95) for s in size] | ||
| else: | ||
| size = [int(s * 1.05) for s in size] |
There was a problem hiding this comment.
Since 1.05 is a little off from the true inverse of .95, the zooming back out doesn't quite make it to full resolution. This is addressed by the if block at line 98. And it is also true the zooming looks fine to the user but the higher num_crop_steps is the larger that last jump from where this zoom out ends and the final full resolution display. Writing 1.05 out to some more digits, like 1.0526, is one way. A better way might be to write 1 / 0.95 instead. An even better way might be to declare the "each-step scale factor" as a variable at the top of the method and use that variable both in the zooming in and in the zooming out (i.e. here that would be 1/scale_factor_each_step) as then the relationship is made much clearer and if a developer decides to change that scale factor they only have to change it in one place and the necessary inverse for the zooming out is computed for them.
iingram
approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this change allows picamera2 version lesehest able to toggle zoom when you press the function button. The value for zoom can be changed in the code to give more or less zoom, and it does it slowly so you actually see where its zooming in on gradually rather than a jump in and out. It touched recorder.py because within toggle_zoom previously it ran the code that will make the fenrir/picamera version toggle zoom. but those functions inherent to picamera do not exist in picamera2, so the call to this toggle zoom had to be placed within recorder_picamera.py and recorder_picamera2.py respectively with the broader toggle_zoom in recorder.py calling it generally. I tested it on both a lesehest and fenrir operating system and this version works on both.
#110