Fix several warnings in Python doc generation

modules/core/include/visp3/core/vpColVector.h

@@ -319,7 +319,7 @@ class VISP_EXPORT vpColVector : public vpArray2D<double>
    *   ofs.close();
    * }
    * \endcode
-   * produces `log.csvè file that contains:
+   * produces `log.csv` file that contains:
    * \code
    * 0
    * 1

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,13 +113,15 @@ 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
 ^^^^^^^^^^^^^^^^^^^
 
 If a class does not appear in the configuration dictionary, it takes on the default value of each option.
 
 
 .. 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<Type>& fn(vpImage<vpRGBa>&, Type, double&)",
     "static": false,

modules/python/doc/rst/dev/custom_bindings.rst

@@ -1,3 +1,4 @@
 .. _Custom binding:
+
 Adding a custom function binding
 =================================

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<unsigned char> 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 <Function options>`.
+
+Abstract class not detected
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+If you have this error:
+
+  error: invalid new-expression of abstract class type ‘vpTemplateTrackerMI’
+  return new Class{std::forward<Args>(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.

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<unsigned char> 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>(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.

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 ''