Port to 4.x: submodule or a class scope for exported classes #21488#21553
Port to 4.x: submodule or a class scope for exported classes #21488#21553alalek merged 4 commits intoopencv:4.xfrom
Conversation
All classes are registered in the scope that corresponds to C++ namespace or exported class. Example: `cv::ml::Boost` is exported as `cv.ml.Boost` `cv::SimpleBlobDetector::Params` is exported as `cv.SimpleBlobDetector.Params` For backward compatibility all classes are registered in the global module with their mangling name containing scope information. Example: `cv::ml::Boost` has `cv.ml_Boost` alias to `cv.ml.Boost` type
|
|
||
| cv.gapi.streaming.queue_capacity = cv.gapi_streaming_queue_capacity | ||
|
|
||
| cv.gapi.wip.GStreamerPipeline = cv.gapi_wip_gst_GStreamerPipeline |
There was a problem hiding this comment.
Still required because GStreamerPipeline is registered as cv.gapi.wip.gst.GStreamerPipeline
There was a problem hiding this comment.
@OrestChura do you know why GStreamerPipeline can't be just in cv.gapi.wip.gst (not cv.gapi.wip) ?
Is it because we have alias in c++?
There was a problem hiding this comment.
@TolyaTalamanov Yes, we have cv::gapi::wip::GStreamerPipeline alias, but the initial declaration is in cv::gapi::wip::gst.
But it is not necessary to have it in Python, I think.
We can discuss and if needed fix it in a separate PR
|
It's kind of strange to see On Ubuntu 20.04 GCC 9.3.0 and Clang 10 with and without Other CI builds are fine too.... |
|
Looks like the problem is related to
Strings should be passed as strings instead of identifier or empty argument. |
|
My configuration works without any errors Details
C++11 pre-print draft
According to this statement the following code is completely valid (unfortunately) even with #include <iostream>
#define foo(x, y) x ## y
int main() {
const char* helloworld = "hello, world!";
std::cout << "Two arguments: '" << foo(hello, world)
<< "'\nFirst argument only: '" << foo(helloworld, )
<< "'\nSecond argument only: '"<< foo(, 1)
<< "'\nCompletely empty: '" << foo(,) "'\n";
return 0;
}Outputs: Let's try specify strings arguments explicitly |
|
Explicit string arguments in macros are not fixing builds for Linux configuration with |
|
@alalek Can you share a content of the DetailsUnavailable modules diff: x - means not built
|
|
According to error messages on Ubuntu 16/18/20 this problem is related to "viz" module: There is mess between:
Wrapper is defined through alias name: Probably It makes sense to bring similar test case into core's To reproduce original error please install |
|
Yep, thanks for reproducer. I'll add the appropriate test case with a fix. It makes sense to use |
43271bc to
0cc7954
Compare
| self.export_name = self.original_name | ||
| self.scope_name = re.sub(r"^cv\.?", "", self.scope_name) | ||
|
|
||
| self.class_id = normalize_class_name(name) |
There was a problem hiding this comment.
If we want to rename .name, .wname, .cname, .sname wtf fields then we should do it anywhere (for all entities, and for all bindings generators and with clear documentation).
Is it possible to have minimal patch here?
There was a problem hiding this comment.
Yes, but there is already a mess with the naming, because different parts treats them as they want.
Something uses wname as identifier, something uses it as general class name.
At least better naming helps to describe the actual intent.
What is sname in this case?
There was a problem hiding this comment.
I completely agree with that.
My statement that we should not have this change now in the current PR. Lets try to keep it minimal.
modules/python/src2/pycompat.hpp
Outdated
| { \ | ||
| char str[1000]; \ | ||
| sprintf(str, "<"#WNAME" %p>", self); \ | ||
| sprintf(str, "< " MODULESTR SCOPE"."#NAME" %p>", self); \ |
There was a problem hiding this comment.
IMHO, patch just switched semantic of macro parameters (switched the first and the second parameter).
Previously:
NAMEis internal, C++ code connected identifier.WNAMEis wrapper name.
If we really want to do that, then robust patch should rename the macro too to avoid misusing mistakes.
There was a problem hiding this comment.
Actual name of the NAME macro should be a CLASS_ID, WNAME should be actually NAME in this case.
modules/python/src2/gen2.py
Outdated
| self.scope_name, self.original_name = name.rsplit(".", 1) | ||
|
|
||
| self.export_name = self.original_name | ||
| self.scope_name = re.sub(r"^cv\.?", "", self.scope_name) |
There was a problem hiding this comment.
Current code emits (pyopencv_generated_modules_content.h):
static PyMethodDef methods_viz[] = {
{"PyAffine3d_Identity", CV_PY_FN_WITH_KW_(pyopencv_cv_viz_PyAffine3d_Identity, 0), "PyAffine3d_Identity() -> retval\n."},
...
- "PyAffine3d_Identity" -> "Affine3d_Identity" as we see and use
Affine3dfrom Python. pyopencv_cv_viz_PyAffine3d_Identity. With similar case in Java it is important to have "wrapper" name in method declaration, e.g:pyopencv_cv_viz_Affine3d_Identity()- Doc string should be replaced too.
Next (pyopencv_generated_types_content.h):
if( PyArg_ParseTupleAndKeywords(py_args, kw, "O:viz_Affine3d.product", (char**)keywords, &pyobj_t)
- "O:viz_PyAffine3d.product" => "O:viz_Affine3d.product"
struct CV_EXPORTS_W_SIMPLE CV_WRAP_AS(Affine3d) PyAffine3d
^ ^
we must see this in binding |
we use this internally in C++ code only
Test case:
// class CV_EXPORTS_W CV_WRAP_AS(ExportClassName) OriginalClassName
static PyMethodDef methods_utils_nested[] = {
{"OriginalClassName_create", CV_PY_FN_WITH_KW_(pyopencv_cv_utils_nested_OriginalClassName_create, 0), "OriginalClassName_create() -> retval\n."},
There was a problem hiding this comment.
Yep, I know, that's why the patch is in WIP
0cc7954 to
559c32e
Compare
| } | ||
| else if (PyType_CheckExact(scope)) | ||
| { | ||
| scope = getScopeFromTypeObject(scope, current_scope_name); |
There was a problem hiding this comment.
getScopeFromTypeObject() may trigger error (through PyErr_Format() with NULL result).
But its message will be overwritten below (line 412)
Can we "stack" errors?
There was a problem hiding this comment.
No, it was rejected. PEP 490. Manual solution is out of the current patch scope.
559c32e to
433b1eb
Compare
alalek
left a comment
There was a problem hiding this comment.
LGTM 👍
Could you please update 3.4 PR with new tests?
…classes-4x-port 4.x: submodule or a class scope for exported classes * feature: submodule or a class scope for exported classes All classes are registered in the scope that corresponds to C++ namespace or exported class. Example: `cv::ml::Boost` is exported as `cv.ml.Boost` `cv::SimpleBlobDetector::Params` is exported as `cv.SimpleBlobDetector.Params` For backward compatibility all classes are registered in the global module with their mangling name containing scope information. Example: `cv::ml::Boost` has `cv.ml_Boost` alias to `cv.ml.Boost` type * refactor: remove redundant GAPI aliases * fix: use explicit string literals in CVPY_TYPE macro * fix: add handling for class aliases
Port of #21488
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.