micro-manager / mmCoreAndDevices

Micro-Manager's device control layer, written in C++
40 stars 108 forks source link

Add `snap()` method #150

Closed ianhi closed 2 years ago

ianhi commented 2 years ago

When scripting to get a snapped image you currently need to call both:

snapImage()
getImage()

which is bummer when interactivly scripting becasue it's twice as much text. Consequently I almost always writing a convenience method like so:

def gen_snap(mmc):
   def snap():
       mmc.snapImage()
       return mmc.getImage()

and recently added this to pymmcore-plus: https://github.com/tlambert03/pymmcore-plus/pull/74

I think this is generally useful and propose adding this convenience method here.

Ideally along with a callback like I added to pymmcore-plus in order to enable synchronization of scrpiting with any gui elements.

nicost commented 2 years ago

I am not so sure that would be a good thing to add to the Core. Snapping an image (exposing the camera), and returning the actual pixel data into computer memory are very distinct operations, and more often than not need to be separated. For instance, if you have a computer controller light shutter in that system, you will always want to close that shutter after exposing the camera. I am not much in favor of making that distinction vague by presenting a wrapped version of those two functions. After all, it really is not that much typing, and you can always create your own convenience function as you did.

marktsuchida commented 2 years ago

I tend to agree with @nicost on this, if only to keep the API to a minimum. Please feel free to reopen if you're not satisfied :)

ianhi commented 2 years ago

Fair enough!

What abut the idea of a callback that runs when snapImage is called? To notify all listeners that there is new data to display?

marktsuchida commented 2 years ago

That might be okay to have; the callback should fire just before (or just after) snapImage returns, after the exposure has finished. I'm not particularly opposed to adding that.

But perhaps MMCore is not the best layer to do this kind of thing in, because you might soon outgrow it into a situation where each conceptual "snap" is a multi-channel acquisition (say, DIC and GFP), so that a simple callback from snapImage is no longer sufficient to update the UI correctly. There is no way to handle that situation in MMCore, because MMCore is not aware of any kind of software sequencing, such as multi-channel snaps.

The notification callbacks in MMCore were originally motivated by the (minimal) need to transmit events that originate in hardware, and still only really work well for that case (as you probably know by now, you can't expect a change notification when you set a property from software; some devices always notify, others never, and yet others only notify if the change originated in hardware). An exception is onSystemConfigurationLoaded, which probably could have been handled by the application layer (it was a backward-compatibility expedient).

Although it would be nice if MMCore worked more like a general event dispatch mechanism, with more consistent behavior, unfortunately that work has not been done, and I'm not sure it would be best to try to accomplish that within its current (one might say broken) architecture. At least it will be a lot of work, particularly if backward compatibility is to be maintained. I think it makes more sense to have a (Python) layer to intercept calls to (and callbacks from) MMCore so that you can generate whatever events you need, with only the absolutely necessary bits added to MMCore. But I'm definitely open to further discussion.

ianhi commented 2 years ago

But perhaps MMCore is not the best layer to do this kind of thing in, because you might soon outgrow it into a situation where each conceptual "snap" is a multi-channel acquisition

MMCore so that you can generate whatever events you need, with only the absolutely necessary bits added to MMCore. But I'm definitely open to further discussion

Adding them to pymmcore+ works for me, but I was trying to be mindful of the concerns about adding extra api on the python layer that were raised here: https://github.com/tlambert03/napari-micromanager/issues/97

marktsuchida commented 2 years ago

Yeah, that is a valid point. I guess it really depends on what we want in the long term.

On the one hand, MMCore and MMStudio are so excessively coupled that it takes way too much work to evolve MMCore significantly, so I feel that if I were working in Python I would treat MMCore as a legacy (and slow-changing) subsystem and wrap it as needed to make it behave nicely (and with the expectation that some other backend may come along in the future). Of course this way of thinking may not work with pycro-manager in the equation.

On the other hand, I also agree that it would be nice if new device-control-related features could be added in a manner that benefits MMStudio (and other Java/C++ users) as well, provided that it is done cleanly (without adding technical debt to the MMCore API). The main challenge is that this is much harder to do right, compared to coding something in Python.

Adding an onSnap notification, for example, seems completely harmless, so if it addresses your needs, even in the short term, it sounds good to me. I was mostly trying to temper your expectations about MMCore as a proper event dispatcher.