diff --git a/modules/core/include/visp3/core/vpColVector.h b/modules/core/include/visp3/core/vpColVector.h index 56111c54f3..518b461043 100644 --- a/modules/core/include/visp3/core/vpColVector.h +++ b/modules/core/include/visp3/core/vpColVector.h @@ -319,7 +319,7 @@ class VISP_EXPORT vpColVector : public vpArray2D * ofs.close(); * } * \endcode - * produces `log.csvè file that contains: + * produces `log.csv` file that contains: * \code * 0 * 1 diff --git a/modules/python/doc/rst/dev/config.rst b/modules/python/doc/rst/dev/config.rst index 15dc915c80..50f9f58736 100644 --- a/modules/python/doc/rst/dev/config.rst +++ b/modules/python/doc/rst/dev/config.rst @@ -86,12 +86,14 @@ Module-level options .. warning:: + Exceptions are not handled: they should always be placed in :code:`ignored_classes`. When a ViSP exception is thrown to the Python interpreter, it is converted to a RuntimeError .. _Enum options: + Enum-level options ^^^^^^^^^^^^^^^^^^^ @@ -111,6 +113,7 @@ If this flag is true, no binding is generated for this enum. The default value i A possible improvement would be to add an :code:`arithmetic` flag to the configuration options to handle this. .. _Class options: + Class-level options ^^^^^^^^^^^^^^^^^^^ @@ -118,6 +121,7 @@ If a class does not appear in the configuration dictionary, it takes on the defa .. code-block:: json + "ignored_attributes": ["myAttribute"] "additional_bindings": "bindings_vpArray2D", "use_buffer_protocol": true, @@ -185,10 +189,12 @@ If a class does not appear in the configuration dictionary, it takes on the defa .. _Function options: + Function-level options ^^^^^^^^^^^^^^^^^^^^^^^^^^^ .. code-block:: json + { "signature": "vpImage& fn(vpImage&, Type, double&)", "static": false, diff --git a/modules/python/doc/rst/dev/custom_bindings.rst b/modules/python/doc/rst/dev/custom_bindings.rst index bbe33b1e3d..583404dc52 100644 --- a/modules/python/doc/rst/dev/custom_bindings.rst +++ b/modules/python/doc/rst/dev/custom_bindings.rst @@ -1,3 +1,4 @@ .. _Custom binding: + Adding a custom function binding ================================= diff --git a/modules/python/doc/rst/dev/dev.rst b/modules/python/doc/rst/dev/dev.rst index e602fde58c..a3478c4bb0 100644 --- a/modules/python/doc/rst/dev/dev.rst +++ b/modules/python/doc/rst/dev/dev.rst @@ -11,3 +11,88 @@ Modifying and contributing to the bindings config.rst custom_bindings.rst python_side.rst + + + +Remaining work +----------------------- + +In this section, we list some remaining issues or work to be done. + + +Changes to ViSP C++ API +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +* Write initTracking for vpKltOpencv taking a vpImage as input. Ignore setInitialGuess. + +Code generation +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +* n-ary operators are not generated +* Matrix multiplication should be done through the @ operator (__matmul\__) +* Get operators for vpArray2D and the subclasses should be ignored, as they are reimplemented through custom bindings +* Classes that are not in the top level namespace are ignored. +* Inner classes are also ignored +* The default return policy for references is to copy, which is probably not the expected usage. ViSP sometimes returns references to STL containers, which have to be copied to Python +* Add parameters in config for: + + * GIL scope + +* Add callback for before_module and after_module so that we can define additional bindings by hand in the module. This is already done per class + +Documentation +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +* Generate documentation for: + + * Functions in namespaces etc. + +* Reference python types in Documentation +* Prefer Python examples instead of C++ ones ? + +To be written: +* Documentation for the overall workflow of the bindings generation + + +Python side +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +* UI + +* Add python sources to visp package + + * Matplotlib based plotter + + + +Errors when generating bindings +------------------------------------- + +When modifying the bindings, you may encounter errors. + +Here is a very non-exhaustive list of errors. + +If you encounter a compilation error, make sure to first try rebuilding after cleaning the CMake cache +Pybind did generate problems (an error at the pybind include line) that were solved like this. + +Static and member methods have the same name +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +If, when importing visp in python, you encounter this message: + + ImportError: overloading a method with both static and instance methods is not supported; error while attempting to bind instance method visp.xxx() -> None + +Then it means that a class has both a static method and a member method with the same name. You should :ref:`rename either one through the config files `. + +Abstract class not detected +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +If you have this error: + + error: invalid new-expression of abstract class type ‘vpTemplateTrackerMI’ + return new Class{std::forward(args)...}; + In file included from /home/visp_ws/visp_build/modules/python/bindings/src/tt_mi.cpp:13:0: + /home/visp_ws/visp/modules/tracker/tt_mi/include/visp3/tt_mi/vpTemplateTrackerMI.h:46:19: note: because the following virtual functions are pure within ‘vpTemplateTrackerMI’: + class VISP_EXPORT vpTemplateTrackerMI : public vpTemplateTracker + +You should define the class (here vpTemplaterMI) as pure virtual in the config file (via the flag is_virtual). +This error occurs because some methods are defined as pure virtual in a parent class and are not defined in the class this class: Pure virtual class detection does not look in the class hierarchy but only at the present class. diff --git a/modules/python/doc/rst/known_issues.rst b/modules/python/doc/rst/known_issues.rst index d991af0d1e..a469289ba1 100644 --- a/modules/python/doc/rst/known_issues.rst +++ b/modules/python/doc/rst/known_issues.rst @@ -3,100 +3,28 @@ Known issues ====================== -We are aware of some issues remaining +We are aware of some remaining issues. +If you encounter another problem, please file an issue on Github. -No implicit conversion from ViSP types to Numpy -------------------------------------------------- - - -ViSP 3rd party types (such as cv::Mat) cannot be used from Python -------------------------------------------------- - -Cannot inherit from ViSP ------------------------------------------------- - - - - - -Changes to ViSP ------------------- - -* Write initTracking for vpKltOpencv taking a vpImage as input. Ignore setInitialGuess. - -Code generation -------------------- - -* There is an issue with vpFeatureMomentDatabse::get and vpMomentDatabase::get, ignored for now => a tester -* n ary operators -* Exclude get operators for vpArray2D ? -* Parse subnamespaces - - * Classes in subnamespaces are ignored +Usability +-------------------- -* Keep alive for numpy interfaces -* Keep alive very probably for mbt -* How should we handle parameters coming from external APIs ? e.g. realsense2, PCL. Can we interact with other bindings such as of opencv's -* Reimplement a framegrabber tutorial in python, with matplotlib -* Test return policy for lvalue references (automatic is copy, so this is problematic) -* Add parameters in config for: - - * Return policy (feature moments database) - * Keep alive - * GIL scope - -* Add callback for before_module and after_module so that we can define additional bindings by hand in the module. This is already done per class -* Add a way to replace a default method binding with a custom one (What about doc?) - -Documentation ----------------- -* Generate documentation for: - - * Functions in namespaces etc. - -* Reference python types in Documentation -* Prefer Python examples instead of C++ ones ? - - -To be written: -* Specific changes from C++ to Python API -* Documentation for the overall workflow of the bindings generation -* In code documentation for the generator -* Document config files - -* Failure cases - - * If you have this error: - error: invalid new-expression of abstract class type ‘vpTemplateTrackerMI’ - return new Class{std::forward(args)...}; - In file included from /home/visp_ws/visp_build/modules/python/bindings/src/tt_mi.cpp:13:0: - /home/visp_ws/visp/modules/tracker/tt_mi/include/visp3/tt_mi/vpTemplateTrackerMI.h:46:19: note: because the following virtual functions are pure within ‘vpTemplateTrackerMI’: - class VISP_EXPORT vpTemplateTrackerMI : public vpTemplateTracker - You should define the class (here vpTemplaterMI) as pure virtual in the config file (via the flag is_virtual). - This error occurs because some methods are defined as pure virtual in a parent class and are not defined in the class this class: Pure virtual class detection does not look in the class hierarchy but only at the present class. - -Packaging ------------------- - -* Root CMake - - * Build after doc if doc can be generated +No implicit conversion from ViSP types to Numpy +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -Python side ------------------ -* Testing +Numpy array cannot be implicitely converted to a ViSP representation when calling a ViSP function. - * Test numpy arrays, partially done - * Test specific methods with specific returns - * Test keep alive if possible ? -* Generate some examples +ViSP 3rd party types (such as cv::Mat) cannot be used from Python +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - * Tracking (megapose/mbt) - * Frame grabbing - * UI +We do not interface with other bindings (as it is not trivial and may require specific Pybind ABI), and we do not wrap third party types. +Thus, alternatives must be provided by hand into the ViSP API (or wrapped through custom bindings) so that the functionalities can be used from Python -* Add python sources to visp package +Cannot inherit from a ViSP class in Python +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - * Matplotlib based plotter +Right now, it is not possible to inherit from a ViSP class with a Python class. Virtual methods cannot be overriden. +To remedy this, trampoline classes should be implemented into the generator, either fully automated (but that is a lot of complexity) +or by providing the trampoline by hand and adding a way to reference the trampoline class in the configuration file. diff --git a/modules/python/generator/visp_python_bindgen/doc_parser.py b/modules/python/generator/visp_python_bindgen/doc_parser.py index d6e060dc6e..4dd47d8325 100644 --- a/modules/python/generator/visp_python_bindgen/doc_parser.py +++ b/modules/python/generator/visp_python_bindgen/doc_parser.py @@ -152,31 +152,51 @@ class DocElements(object): ] def escape_for_rst(text: str) -> str: - return text + forbidden_chars = ['_', '*'] + res = text + for c in forbidden_chars: + res = res.replace(c, '\\' + c) + return res + -def process_mixed_container(container: MixedContainer, level: int, level_string='') -> str: + +def process_mixed_container(container: MixedContainer, level: int, level_string='', escape_rst=False) -> str: ''' :param level_string: the string being built for a single level (e.g. a line/paragraph of text) + This is equivalent to a left fold operation, + so don't forget to aggregate the results in level_string if you add another component ''' if container.name in IGNORED_MIXED_CONTAINERS: return level_string one_indent = ' ' * 2 indent_str = one_indent * level requires_space = not level_string.endswith(('\n', '\t', ' ')) and len(level_string) > 0 + + def process_markup(symbol: str) -> str: + text = symbol if not requires_space or len(level_string) == 0 else ' ' + symbol + for c in container.value.content_: + text = process_mixed_container(c, level, text, escape_rst=escape_rst) + text += symbol + ' ' + return text + # Inline blocks - if isinstance(container.value, str): - return level_string + escape_for_rst(container.value.replace('\n', '\n' + indent_str).strip()) + if isinstance(container.value, str) and container.name != 'verbatim': + content = container.value.replace('\n', '\n' + indent_str).strip() + if escape_rst: + content = escape_for_rst(content) + return level_string + content if container.name == 'text': - return escape_for_rst(container.value.replace('\n', '\n' + indent_str).strip()) + content = container.value.replace('\n', '\n' + indent_str).strip() + if escape_rst: + content = escape_for_rst(content) + return level_string + content if container.name == 'bold': - markup_start = '**' if not requires_space or len(level_string) == 0 else ' **' - return level_string + markup_start + container.value.valueOf_ + '** ' + return level_string + process_markup('**') if container.name == 'computeroutput': - markup_start = '`' if not requires_space or len(level_string) == 0 else ' `' - return level_string + markup_start + escape_for_rst(container.value.valueOf_) + '` ' + return level_string + process_markup('`') if container.name == 'emphasis': markup_start = '*' if not requires_space else ' *' - return level_string + markup_start + container.value.valueOf_ + '* ' + return level_string + process_markup('*') if container.name == 'sp': return level_string + ' ' if container.name == 'linebreak': @@ -204,7 +224,10 @@ def process_mixed_container(container: MixedContainer, level: int, level_string= return level_string + (' ' if requires_space else '') + container.value.valueOf_ + ' ' if container.name == 'verbatim': - raise NotImplementedError() + text = container.value + text = escape_for_rst(text) + text = '\n' * 2 + indent_str + '::\n\n' + indent_str + one_indent + text.strip().replace('\n', '\n' + indent_str + one_indent).strip() + '\n' * 2 + return level_string + text # Block types if container.name == 'simplesect': @@ -244,7 +267,7 @@ def process_mixed_container(container: MixedContainer, level: int, level_string= for h in line.highlight: c = '' for hh in h.content_: - c = process_mixed_container(hh, level, c) + c = process_mixed_container(hh, level, c, escape_rst=False) cs.append(c) s = ''.join(cs) lines.append(s) @@ -271,7 +294,7 @@ def process_paragraph(para: docParaType, level: int) -> str: res = '' contents: List[MixedContainer] = para.content_ for content_item in contents: - res = process_mixed_container(content_item, level, res) + res = process_mixed_container(content_item, level, res, escape_rst=True) return res def process_description(brief: Optional[descriptionType]) -> str: @@ -305,10 +328,10 @@ def __init__(self, path: Optional[Path], env_mapping: Dict[str, str]): for method_def in method_defs: is_const = False if method_def.const == 'no' else True is_static = False if method_def.static == 'no' else True - ret_type = ''.join(process_mixed_container(c, 0) for c in method_def.type_.content_).replace('vp_deprecated', '').replace('VISP_EXPORT', '') + ret_type = ''.join(process_mixed_container(c, 0, escape_rst=False) for c in method_def.type_.content_).replace('vp_deprecated', '').replace('VISP_EXPORT', '') param_types = [] for param in method_def.get_param(): - t = ''.join(process_mixed_container(c, 0) for c in param.type_.content_) + t = ''.join(process_mixed_container(c, 0, escape_rst=False) for c in param.type_.content_) param_types.append(t) if method_def.name == cls_name or ret_type != '': signature_str = f'{ret_type} {cls_name}::{method_def.name}({",".join(param_types)}) {{}}' @@ -368,20 +391,22 @@ def get_documentation_for_method(self, cls_name: str, signature: MethodDocSignat params_dict = self.get_method_params(method_def) cpp_return_str = self.get_method_return_str(method_def) + param_strs = [] for param_name in input_param_names: if param_name in params_dict: - param_strs.append(f':param {param_name}: {params_dict[param_name]}') + param_strs.append(f':param {escape_for_rst(param_name)}: {params_dict[param_name]}') param_str = '\n'.join(param_strs) + if len(output_param_names) > 0: return_str = ':return: A tuple containing:\n' # TODO: if we only return a single element, we should modify this if signature.ret != 'void' and signature.ret is not None: return_str += f'\n\t * {cpp_return_str}' for param_name in output_param_names: if param_name in params_dict: - return_str += f'\n\t * {param_name}: {params_dict[param_name]}' + return_str += f'\n\t * {escape_for_rst(param_name)}: {params_dict[param_name]}' else: - return_str += f'\n\t * {param_name}' + return_str += f'\n\t * {escape_for_rst(param_name)}' else: return_str = f':return: {cpp_return_str}' if len(cpp_return_str) > 0 else ''