Details

Reviewers

Summary

Ported the finding projection vector code to C.
Packing, Scaling the uv maps uses existing C code aswell.

Need your help to check whether I'm freeing memory properly, as I'm not too familiar with C

Ported the code directly from C to Python
Tested using a couple of meshes on "Release" build
a high poly model (286971 polys), old: hangs blender, new: 2.25 seconds
5 suzanne (2500 polys), old: 0.10 seconds, new: 0.013947 seconds

You can test it out by going to editmode -> Smart Uv Project C, I still kept the old one for comparison sake.

Potential backward breaking compatibillity:
After Smart Project Uv C is conducted, the UV selection before the operation is not retained. (The python operator retains the UV selections)

Suzzane unwrap comparison
old (Smart Projection time: 0.16):

new (Smart Projection time: 0.014):

Diff Detail

Repository

rB Blender

Branch

refactor-smart-uv (branched from master)

Build Status

Buildable 9040
Build 9040: arc lint + arc unit

Event Timeline

Andreas Terrius (andreasterrius) requested review of this revision.Jul 16 2020, 3:25 PM

Andreas Terrius (andreasterrius) created this revision.

Harbormaster completed remote builds in B9040: Diff 26867.Jul 16 2020, 3:25 PM

Andreas Terrius (andreasterrius) mentioned this in T76532: Improve performance of "Smart UV Project".Jul 16 2020, 3:27 PM

Andreas Terrius (andreasterrius) edited the summary of this revision. (Show Details)Jul 16 2020, 3:29 PM

Andreas Terrius (andreasterrius) edited the summary of this revision. (Show Details)

ogierm awarded a token.Jul 16 2020, 4:41 PM

ogierm added a subscriber: ogierm.

Brecht Van Lommel (brecht) added a reviewer: Campbell Barton (campbellbarton).Jul 16 2020, 4:58 PM

Brecht Van Lommel (brecht) added a subscriber: Brecht Van Lommel (brecht).

Brecht Van Lommel (brecht) added inline comments.

source/blender/editors/uvedit/uvedit_unwrap_ops.c
1851	This is comparing pointers rather than area.

Daniel Ulrich (dulrich) awarded a token.Jul 16 2020, 10:55 PM

I fixed the pointer comparison but I still see some projection bug when I paid a closer attention to the unwrapped result

I'll fix it and report back when the bug has been ironed out

Andreas Resch (AndreasResch) awarded a token.Jul 18 2020, 8:59 AM

Mike (demmordor) awarded a token.Jul 19 2020, 7:05 AM

Nabil Adel (mywa880) awarded a token.Jul 19 2020, 9:34 AM

Ramil Roosileht (Limarest) awarded a token.Jul 19 2020, 3:26 PM

Fixed wrong area comparison function

Harbormaster completed remote builds in B9116: Diff 26992.Jul 20 2020, 5:50 PM

Andreas Terrius (andreasterrius) edited the summary of this revision. (Show Details)Jul 20 2020, 5:57 PM

Removed debug print, match default parameter to old one

Harbormaster completed remote builds in B9117: Diff 26993.Jul 20 2020, 6:01 PM

Fix wrongly change default value

Harbormaster completed remote builds in B9119: Diff 26995.Jul 20 2020, 6:47 PM

Andreas Terrius (andreasterrius) edited the summary of this revision. (Show Details)Jul 20 2020, 6:57 PM

I think this can be reviewed again now, I've mostly fixed the bugs from the first diff.

Please notify me if I should just remove the old operator and replace the operator completely in the UI with the C operator, this shouldn't be too hard.

Dalai Felinto (dfelinto) added a subscriber: Dalai Felinto (dfelinto).Jul 23 2020, 11:52 AM

Dalai Felinto (dfelinto) added inline comments.

source/blender/editors/uvedit/uvedit_unwrap_ops.c
2064	Please check code style for comments: /** * Smallest first, ... */ https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments

If this is effectively replacing the other operator, you can just remove it. Unless it makes code review more complicated

Good start, most commends inline.

source/blender/editors/uvedit/uvedit_unwrap_ops.c
1848	There should be a comment explaining why this small threshold is needed.
1864	Use `r_` prefix for return arguments. See https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Naming
1882	Alloca should only be used for small allocations, especially for functions called often where malloc may be a bottleneck. Use `MEM_mallocN` or `MEM_callocN` here. Note, it seems this is only used for tagging, you can use `BM_ELEM_TAG` for this. See code that uses `BM_mesh_elem_hflag_disable_all`.
1884	Use `f_index` (instead of camel caps). Matching conventions for surrounding code. Same goes for other camel-caps naming in this function.
1885	Both `--value` and `value--` used here without any reason for the difference. Prefer `value--` unless the other order is needed.
1902	Prefer call this `normal` than `vec`, eg `average_normal`.
1929	Use `!is_zero_v3(..)` here.
1943	Naming here could be improved `angle_diff_best`, `angle_diff_test` (instead of temp).
2114–2118	`axis_dominant_v3_to_m3` to create a matrix from a direction vector.
2168	Why is this needed?
2201–2208	Angles can be radians, the UI will show as degrees. See use of `PROP_ANGLE`.

Campbell Barton (campbellbarton) requested changes to this revision.Jul 23 2020, 2:42 PM

This revision now requires changes to proceed.Jul 23 2020, 2:42 PM

Replaced old UV smart project with the C one
Removed alloca, and used BM_ELEM_TAG instead
Various conformance to code style guidelines
Removed unneeded codes related to uv sync
Changed RNA_FLOAT for projection_angle to PROP_ANGLE

Harbormaster completed remote builds in B9175: Diff 27117.Jul 25 2020, 10:59 AM

Reverted accidental comment change

Harbormaster completed remote builds in B9176: Diff 27118.Jul 25 2020, 11:02 AM

stefano severi (Tetone) awarded a token.Jul 25 2020, 11:50 AM

Conform to code conventions

Harbormaster completed remote builds in B9177: Diff 27119.Jul 25 2020, 12:06 PM

Campbell Barton (campbellbarton) mentioned this in rB850234c1b10a: UV: port smart project from Python to C.Jul 26 2020, 1:30 PM

Committed with edits rB850234c1b10a828678f1b91001f2731db807f7e2

Made the following edits:

Only ignore small faces when sorting, instead of a small difference between faces since many small differences could make sorting unpredictable depending on the current order.
Pass the 3D view to BKE_view_layer_array_from_objects_in_edit_mode_unique_data_with_uvs so local-view will be respected as this is normally accessed from the 3D view.
Use pointers for project_thick_faces instead of copying the ThickFace.
Use a memory arena for LinkList use to avoid many small allocations.
Don't change selection, allow operating on unselected UV's instead rB9482cc68652d: UV: support creating ParamHandle without checking UV selection

Campbell Barton (campbellbarton) accepted this revision.Jul 26 2020, 1:38 PM

This revision is now accepted and ready to land.Jul 26 2020, 1:38 PM

Campbell Barton (campbellbarton) closed this revision.Jul 26 2020, 1:38 PM

Bataev Artem (shader) awarded a token.Jul 26 2020, 3:33 PM

While it's very nice that this is now much faster it would have been good not to remove the UV Smart project operator from object mode or at least document that you were doing this in the release notes, as any addon which calls UV smart project from object mode is now broken. If you're going to change the context in which an operator works in such a major way please mention this. Bug report submitted https://developer.blender.org/T83038

Refactor Smart Project UV from Python to C
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 26867

release/scripts/startup/bl_ui/space_view3d.py

source/blender/editors/uvedit/uvedit_intern.h

source/blender/editors/uvedit/uvedit_ops.c

source/blender/editors/uvedit/uvedit_unwrap_ops.c

Refactor Smart Project UV from Python to CClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 26867

release/scripts/startup/bl_ui/space_view3d.py

source/blender/editors/uvedit/uvedit_intern.h

source/blender/editors/uvedit/uvedit_ops.c

source/blender/editors/uvedit/uvedit_unwrap_ops.c

Refactor Smart Project UV from Python to C
ClosedPublic
Actions

Revision Contents
Changeset List