Add new capture methods capture_2d/capture_3d/capture_2d_3d #296
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
eskaur
reviewed
Nov 28, 2024
modules/zivid/camera.py
Outdated
| @@ -47,9 +47,127 @@ def __str__(self): | |||
| def __eq__(self, other): | |||
| return self.__impl == other._Camera__impl | |||
|
|
|||
| def capture2d3d(self, settings): | |||
There was a problem hiding this comment.
Should we change the "samples" directory and README snippets to use these new functions?
Merged
johningve
force-pushed
the
ZIVID-9207-wrap-new-capture-methods
branch
2 times, most recently
from
4943a0f to
e08ec7d
Compare
vawale
previously approved these changes
Dec 6, 2024
johningve
force-pushed
the
ZIVID-9207-wrap-new-capture-methods
branch
2 times, most recently
from
d1baa82 to
d32bde4
Compare
apartridge
reviewed
Dec 6, 2024
modules/zivid/camera.py
Outdated
| TypeError: If the settings argument is not a Settings2D or a Settings | ||
| """ | ||
| if isinstance(settings, Settings2D): | ||
| return Frame2D(self.__impl.capture2D(_to_internal_settings2d(settings))) |
There was a problem hiding this comment.
why using different casing (2D) in the impl method?
There was a problem hiding this comment.
Ah, I'll change it
johningve
force-pushed
the
ZIVID-9207-wrap-new-capture-methods
branch
3 times, most recently
from
20957a5 to
9bb6f87
Compare
johningve
changed the title
johningve
force-pushed
the
ZIVID-9207-wrap-new-capture-methods
branch
from
9bb6f87 to
b3cf317
Compare
apartridge
reviewed
Dec 9, 2024
modules/zivid/camera.py
Outdated
Comment on lines
139
to
142
| This overload is provided for convenience. Note that only the Settings2D part under `Settings.color` will be | ||
| used for the capture. The other parts of the settings will be ignored. | ||
|
|
||
| This method will throw if `Settings.color` is not set. |
There was a problem hiding this comment.
This method is used for capture_2d(settings) and capture_2d(settings_2d) it seems, this part of the docstring applies only for the former and not the latter. This could be clarified a bit in the docstring?
apartridge
reviewed
Dec 9, 2024
Comment on lines
+17
to
+22
| settings.color = Settings2D() | ||
| settings.color.acquisitions.append(Settings2D.Acquisition()) | ||
| settings.color.acquisitions[0].aperture = 5.6 | ||
| settings.color.acquisitions[0].exposure_time = datetime.timedelta( | ||
| microseconds=8333 | ||
| ) |
There was a problem hiding this comment.
Did you check that we dont have same bug here as we had in .NET where we didnt mutate the actual settings.color but instead a copy?
There was a problem hiding this comment.
Yeah. Added a commit to test this
apartridge
reviewed
Dec 9, 2024
|
|
||
| with shared_file_camera.capture_2d_3d(settings) as frame: | ||
| assert frame | ||
| assert isinstance(frame, zivid.frame.Frame) |
There was a problem hiding this comment.
Could in this file test a bit more, such that the frame.settings matches what is set above, which would reduce chances of bugs in this code.
johningve
force-pushed
the
ZIVID-9207-wrap-new-capture-methods
branch
3 times, most recently
from
bc01afb to
558b397
Compare
This commit adds the capture methods `capture_2d`, `capture_3d`, and `capture_2d_3d`. These methods are intended to replace `capture` which is now deprecated.
johningve
force-pushed
the
ZIVID-9207-wrap-new-capture-methods
branch
2 times, most recently
from
3f0a119 to
fc5cf96
Compare
This extends test_settings to convert to the internal (C++) settings and back, ensuring that the settings are propagated correctly to C++.
johningve
force-pushed
the
ZIVID-9207-wrap-new-capture-methods
branch
from
fc5cf96 to
22ee714
Compare
apartridge
approved these changes
Dec 9, 2024
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.
These methods are intended to replace
capturewhich is now deprecated.