WIP interface categories#3146
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a formal interface system for node descriptors in Meshroom, allowing automatic interface discovery and dynamic method dispatch from the UI. While this is a solid architectural improvement that replaces hardcoded node-type checks in the QML viewer, several issues were identified in the review. These include a critical runtime typo (getr instead of get), a flaw in the metaclass that incorrectly treats concrete parent classes as interfaces, potential duplicate entries when multiple plugins implement the same interface, a lack of safety checks for nodeDesc in callInterfaceMethod, and a potential type mismatch in QML if null is returned instead of an array.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for interface in node.nodeDesc.interfaces: | ||
| self.activeNodes.getr(interface).node = node |
There was a problem hiding this comment.
There is a critical typo here: getr is used instead of get. This will raise an AttributeError at runtime when setActiveNode is called. Additionally, we should safely check if nodeDesc exists and has the interfaces attribute, and ensure that self.activeNodes.get(interface) actually returns a valid entry before setting its node. Note that the same typo getr also exists on line 1029 (which is outside the current diff hunk) and should be fixed as well.
| for interface in node.nodeDesc.interfaces: | |
| self.activeNodes.getr(interface).node = node | |
| nodeDesc = getattr(node, "nodeDesc", None) | |
| if nodeDesc and hasattr(nodeDesc, "interfaces"): | |
| for interface in nodeDesc.interfaces: | |
| entry = self.activeNodes.get(interface) | |
| if entry: | |
| entry.node = node |
| cls.interfaces = [ | ||
| b.__name__ for b in type.mro(cls) | ||
| if b is not Interface and b is not cls | ||
| and isinstance(b, type) and issubclass(b, Interface) | ||
| ] |
There was a problem hiding this comment.
In Python, subclasses of a class that implements an interface will also return True for issubclass(b, Interface). This means that if a concrete node class (e.g., MyNode) implements FeatureProviderInterface, any subclass of MyNode (e.g., MySubNode) will mistakenly include MyNode in its interfaces list. To prevent concrete parent classes from being treated as interfaces, we should exclude classes that are instances of InterfaceMeta (the metaclass for concrete interfaced classes) by checking not isinstance(b, mcs).
| cls.interfaces = [ | |
| b.__name__ for b in type.mro(cls) | |
| if b is not Interface and b is not cls | |
| and isinstance(b, type) and issubclass(b, Interface) | |
| ] | |
| cls.interfaces = [ | |
| b.__name__ for b in type.mro(cls) | |
| if b is not Interface and b is not cls | |
| and isinstance(b, type) and issubclass(b, Interface) | |
| and not isinstance(b, mcs) | |
| ] |
| for nodeType, nodePlugin in meshroom.core.pluginManager.getRegisteredNodePlugins().items(): | ||
| nodeDesc = nodePlugin.nodeDescriptor | ||
| for interface in nodeDesc.interfaces: | ||
| self._activeNodes.add(ActiveNode(interface, parent=self)) |
There was a problem hiding this comment.
Since multiple node plugins can implement the same interface (e.g., FeatureProviderInterface), iterating over all registered node plugins and adding their interfaces directly to self._activeNodes will attempt to add duplicate keys to the DictModel. We should keep track of already added interfaces using a set to avoid duplicate entries.
| for nodeType, nodePlugin in meshroom.core.pluginManager.getRegisteredNodePlugins().items(): | |
| nodeDesc = nodePlugin.nodeDescriptor | |
| for interface in nodeDesc.interfaces: | |
| self._activeNodes.add(ActiveNode(interface, parent=self)) | |
| addedInterfaces = set() | |
| for nodeType, nodePlugin in meshroom.core.pluginManager.getRegisteredNodePlugins().items(): | |
| nodeDesc = nodePlugin.nodeDescriptor | |
| for interface in nodeDesc.interfaces: | |
| if interface not in addedInterfaces: | |
| self._activeNodes.add(ActiveNode(interface, parent=self)) | |
| addedInterfaces.add(interface) |
| node = entry.node | ||
| method = getattr(node.nodeDesc, methodName, None) |
There was a problem hiding this comment.
To make this call more robust and prevent potential AttributeErrors (for example, if node is a CompatibilityNode or does not have a nodeDesc attribute), we should safely retrieve nodeDesc using getattr and verify it is not None before attempting to fetch the method.
node = entry.node
nodeDesc = getattr(node, "nodeDesc", None)
if not nodeDesc:
return None
method = getattr(nodeDesc, methodName, None)| result = _currentScene.callInterfaceMethod("FeatureProviderInterface", "getFeaturesFolders") | ||
| console.log(result) |
There was a problem hiding this comment.
If callInterfaceMethod returns null (e.g., if the active node is not set or the method fails), returning null to featureFolders can cause downstream errors in QML components expecting an array. We should fallback to an empty array []. Also, the console.log(result) leftover from debugging should be removed to keep the console output clean.
result = _currentScene.callInterfaceMethod("FeatureProviderInterface", "getFeaturesFolders") || []
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3146 +/- ##
===========================================
- Coverage 85.36% 85.22% -0.14%
===========================================
Files 73 74 +1
Lines 11455 11494 +39
===========================================
+ Hits 9778 9796 +18
- Misses 1677 1698 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f8c2871 to
962b609
Compare
| class InterfacedClass(metaclass=InterfaceMeta): | ||
| """Base class for objects that want automatic interface discovery via InterfaceMeta.""" | ||
| pass |
There was a problem hiding this comment.
For clarity I would rename it HasInterfaces
| cls.interfaces = [ | ||
| b.__name__ for b in type.mro(cls) | ||
| if b is not Interface and b is not cls | ||
| and isinstance(b, type) and issubclass(b, Interface) | ||
| ] |
There was a problem hiding this comment.
I would rename it interfaceNames to prevent user to try code like if MyInterface in MyNode.interfaces:
| if callable(method): | ||
| try: | ||
| return method(node) | ||
| except Exception as e: | ||
| logging.error(f"callInterfaceMethod: {interfaceName}.{methodName} raised: {e}") | ||
| return None | ||
| return None |
There was a problem hiding this comment.
On failure, it could try to load the next possible activeNode that implement this interface, using a while ?
962b609 to
25a58fe
Compare
| allUiNodes = set(self.uiNodes) | usedNodeTypes | allLoadedNodeTypes | ||
|
|
||
| for nodeType in allUiNodes: | ||
| for nodeType in allUiNodes: |
There was a problem hiding this comment.
| for nodeType in allUiNodes: | |
| for nodeType in allUiNodes: |
Interface-based node category system
Introduces a lightweight interface mechanism for node descriptors, replacing hardcoded node type lists with a declarative, extensible approach. The initial interface implemented is
FeatureProviderInterface.See alicevision/AliceVision#2142
Motivation
Previously, the UI determined which node to use as the active "feature provider" by checking against a hardcoded list of node type names (
"featureProvider": ["FeatureExtraction", "FeatureMatching", ...]). This made it impossible for external plugins to participate without modifying core Meshroom. Similarly, the QML code for resolving features folders contained node-type-specific branching logic.Changes
meshroom/core/desc/interface.py(new file)Interface,InterfaceMeta, andInterfacedClass— a minimal metaclass-based system that automatically populates ainterfaceslist on any node descriptor class that inherits from anInterface.FeatureProviderInterfaceas the first concrete interface, with agetFeaturesFolders(node)method.meshroom/core/desc/node.pyBaseNodenow inherits fromInterfacedClass, enabling all node descriptors to declare interfaces.meshroom/ui/scene.pyinitActiveNodesnow iterates all registered node plugins and registers anActiveNodeentry per interface declared on the descriptor.setActiveNodeupdates the interface-keyed active node entries when a node becomes active.callInterfaceMethod(interfaceName, methodName)slot allows QML to generically call any method defined by an interface on the currently active node for that interface, with error handling.meshroom/ui/qml/Viewer/Viewer2D.qml"FeatureProviderInterface"as the key instead of"featureProvider"._currentScene.callInterfaceMethod("FeatureProviderInterface", "getFeaturesFolders").How to implement a new interface
In an external plugin descriptor:
No changes to core Meshroom are needed for the node to be discovered and used by the UI.