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

Split camera device base class into Base and LegacyBase #590

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

henrypinkard
Copy link
Member

@henrypinkard henrypinkard commented Mar 5, 2025

As discussed in: #243 (comment)

This splits the camera base class that all camera adapters derive from into a legacy class (CLegacyCameraBase) which derives from a new, preferred CCameraBase class. The primary difference between theses is that the Legacy class contains a bunch of code for implementing sequence acquisitions as a series of snaps, which was originally for debugging, but ends up being inherited by all camera devices adapters.

I changed the name to LegacyBase now, rather than in the future as described in (#243 (comment)), because it seems like we would already like device adapter writers to inherit from the new base class.

See below for a few questions.

Copy link
Member

@marktsuchida marktsuchida left a comment

Choose a reason for hiding this comment

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

Thanks for doing this as a separate PR. I'm okay with immediately renaming to "legacy" -- makes sense if going ahead with modifying the camera adapters.

@henrypinkard
Copy link
Member Author

It says you requested a change, but for some reason I don's see what that is. Is there anything else needed before merging this?

@marktsuchida
Copy link
Member

It says you requested a change, but for some reason I don's see what that is.

I think you made the 'small fixes' commit at the same time I was reviewing. In it, you fixed PrepareSequenceAcqusition the way I requested.

But now that PrepareSequenceAcqusition is implemented in CCameraBase, presumably a bunch of cameras can have their change to CLegacyCameraBase undone?

Also, I commented that the Transpose_* properties should be kept in the base class because they are an entirely separate issue that needs analysis and cleanup.

(See my comments above that are marked resolved.)

@henrypinkard
Copy link
Member Author

But now that PrepareSequenceAcqusition is implemented in CCameraBase, presumably a bunch of cameras can have their change to CLegacyCameraBase undone?

Probably. This was a simple refactor-rename, so I think that can be done in a separate step (#598)

Also, I commented that the Transpose_* properties should be kept in the base class because they are an entirely separate issue that needs analysis and cleanup.

Fixed in previous commit

@henrypinkard
Copy link
Member Author

(See my comments above that are marked resolved.)

Should be all addressed now

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.

2 participants