Closed tlambert03 closed 5 months ago
some comments from an offline discussion (saving @marktsuchida from having to write them again here):
@marktsuchida : I guess I'm a little worried that this is in the end increasing the coupling between MMCore and pymmcore-plus. In that sense the method of minimally patching MMCore, only in pymmcore, seems more desirable than doing it wholesale in the MMCore upstream. But I think we want to try to keep this a temporary solution and have an exit plan -- or else I worry that it will hinder more fundamental improvements to MMCore itself.
@tlambert03: perhaps... but i don't think so? really, it would just be acknowledging that, inasmuch as pymmcore primarily offers the core object, it gives users of pymmcore a more "useful" object. For example, when I use PyQt/PySide, the vast majority of methods work like this (I can reimplement paintEvent and Qt itself will use it from the C++ side)
@marktsuchida: Hmm. I think the problem is that CMMCore is really not an object-oriented class, and was never designed to be inherited from. It's okay as long as CMMCore is just a singleton bag of methods (as in current pymmcore-plus), but I feel that all hell may break loose if the behavior of CMMCore starts getting constrained by it.
@tlambert03: okiedoke. fair enough :)
@marktsuchida: Note, for example, that MMCore calling its own public methods is in itself questionable and something that we'd want to avoid in the long term.
@tlambert03: if that's the direction you're going, then I agree, it does make less sense (since that would largely be my criterion for determining what should be virtual). Mostly, my thoughts/expectations here were colored by my experience with PyQt/PySide
will close this, seems unlikely we'll go in this direction
For a couple things in pymmcore-plus, it would be nice if the methods on my re-implemented subclass were called by the C-level core object (i.e. "cross-language polymorphism"). For example
that should print ("Hello! from C++")
Swig allows this feature with directors:
but it's currently disabled for CMMCore... which makes perfect sense because none of the methods defined in MMCore.h are virtual methods.
This PR enables experimental support for polymorphic virtual methods without making any modifications to mmCoreAndDevices by:
VIRTUAL_MMCORE_METHODS
env var at build time, which should be a comma-separrated list of method names, likeVIRTUAL_MMCORE_METHODS=setFocusDevice,setCameraDevice
%feature("director") CMMCore;
flag inside the swig file.There's a test that shows that it works (which isn't being run on CI at the moment, but works locally).
@marktsuchida, I wonder whether you'd consider this? Ultimately, a better question is whether this could be the default, but since I don't know too much about the implications of making methods virtual, I didn't want to leap to modifying MMCore.h itself, but it might make for a reasonable default (for pymmcore) to add virtual to a number of MMCore methods that are called internally by Core