Skip to content

js: fix enum generation issues#26147

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
vrabaud:opencv_js
Dec 19, 2024
Merged

js: fix enum generation issues#26147
asmorkalov merged 1 commit intoopencv:4.xfrom
vrabaud:opencv_js

Conversation

@vrabaud
Copy link
Copy Markdown
Contributor

@vrabaud vrabaud commented Sep 13, 2024 •
edited
Loading

enums were not taken into account

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
force_builders=Custom
build_image:Docs=docs-js
Xbuild_image:Custom=javascript
Xbuild_image:Custom=javascript-simd
build_image:Custom=javascript-simd:3.1.64
buildworker:Custom=linux-4

@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Sep 13, 2024

RFC: what should we do for namespaces for const/enums? As we have a collision here, I prepended the FISHEYE_ prefix, but we don't have do it for all namespaces (e.g. cv::utils::logging::).

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Sep 13, 2024

@vrabaud whitelist was spit on dedicated json files to support contrib_modules. py file presumed for compatibility. See #25986

@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Sep 13, 2024

Thx @asmorkalov , fixed. Does that mean that platforms/js/opencv_js.config.py should be removed ?

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Sep 13, 2024

In long term - yes.

asmorkalov
asmorkalov reviewed Sep 16, 2024
View reviewed changes
modules/js/generator/embindgen.py
Comment on lines +940 to +942
# TODO CALIB_FIX_FOCAL_LENGTH is defined both in cv:: and cv::fisheye
prefix = 'FISHEYE_' if 'fisheye' in ns_name else ''
for name, const in sorted(ns.consts.items()):
name = prefix + name
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if use namespace_prefix_override solution like for the functions? Namespace is already appended to the function names, if it's not overridden by config. It's less hacky and more obvious.

Copy link
Copy Markdown
Contributor Author

@vrabaud vrabaud Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that would work but I don't know how to use it. Should all the enums be added to

opencv/modules/js/generator/embindgen.py

Line 989 in 450e741

namespace_prefix_override = {
?

@asmorkalov asmorkalov added this to the 4.11.0 milestone Sep 16, 2024
@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Dec 16, 2024

@asmorkalov , do you want me to split this PR in 3 per the 3 points I mentioned ? 2 should be easy to merge, the Python script needs more review (though parts of it can easily be merged, like the double const generation)

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 16, 2024

Yes, let's split.

@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Dec 18, 2024

Done with #26638, #26639, #26640

@vrabaud vrabaud changed the title Fix some js generation issues. js: fix enum generation issues Dec 18, 2024
@vrabaud
Copy link
Copy Markdown
Contributor Author

vrabaud commented Dec 18, 2024

#26644 was also split out of this PR. The PR now only contains enum-related changes.

@asmorkalov asmorkalov self-assigned this Dec 19, 2024
@asmorkalov asmorkalov merged commit 6a0affd into opencv:4.x Dec 19, 2024
@vrabaud vrabaud deleted the opencv_js branch December 19, 2024 14:37
@asmorkalov asmorkalov mentioned this pull request Jan 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

Assignees

@asmorkalov asmorkalov

Labels

category: javascript (js)

Projects

None yet

Milestone

4.11.0

Development

Successfully merging this pull request may close these issues.

2 participants

@vrabaud @asmorkalov