Details

Reviewers

Himanshi Kalra (calra)
Habib Gahbiche (zazizizou)
Campbell Barton (campbellbarton)

Maniphest Tasks

T84999: Add Operator tests

Commits

rB7168a4fa5c78: Tests: add edit-mesh operator tests

Summary

Added operator tests for hide, symmetry_snap, tris_convert_to_quads, uvs_rotate, uvs_rotate, uv_texture_add, uv_texture_remove, vert_connect_concave, vert_connect_nonplanar, vertex_color_add, vertex_color_remove, vertices_smooth_laplacian, wireframe, sculpt_vertex_color_add and sculpt_vertex_color_remove.

New .blend

operators.blend3 MBDownload

I tried to add tests for subdivide_edgering, but found that the operator returned inconsistently, with the generated vertexes having different ordering each time, and had some weirdness with the generated vertexes not being selected after the operator, causing the test to fail. The geometry and code is included for the tests, but is commented out, so it's there if it gets fixed.

Part of T84999

Diff Detail

Repository

rB Blender

Branch

operator-tests (branched from master)

Build Status

Buildable 19078
Build 19078: arc lint + arc unit

Event Timeline

Jake (Welp) requested review of this revision.Jul 4 2021, 12:43 AM

Jake (Welp) created this revision.

Jake (Welp) edited the summary of this revision. (Show Details)Jul 4 2021, 12:45 AM

Jake (Welp) added a task: T84999: Add Operator tests.

Thanks for the patch!
You could divide it into 2 patches, one with mesh.c changes

Add in the description what are the changes to mesh.c for and why they are essential for testing.
Can skip the changes [the typo] for mesh_test for now.
Also, explain the role of properties_data_mesh.py (present in operators.blend) with regards to testing.

Edit: I only had a quick look, haven't tested them yet on my system.

Jake (Welp) edited the summary of this revision. (Show Details)Jul 4 2021, 11:33 PM

Drop changes to mesh_test.py

Also, explain the role of properties_data_mesh.py (present in operators.blend) with regards to testing.

Unlinked and uploaded new blend, was not intentional.

Hi @Jake (Welp), thanks for the patch! I have a few suggestions:

Could you rebase your patch so that we can test it
Can you confirm all the test cases can actually fail? For example, the operator 'hide' does not change the mesh, so will the test fail if test object doesn't get hidden?
If possible, use arc when submitting patches

source/blender/blenkernel/intern/mesh.c
446 ↗	(On Diff #39107)	is this still necessary after D12137?
tests/python/operators.py
325–330	you can simplify with something like `{i for i in range(96)}` to select all.

This revision now requires changes to proceed.Sep 5 2021, 12:23 PM

Habib Gahbiche (zazizizou) mentioned this in T84999: Add Operator tests.Sep 5 2021, 12:24 PM

Can you confirm all the test cases can actually fail? For example, the operator 'hide' does not change the mesh, so will the test fail if test object doesn't get hidden?

I thought I checked and confirmed it did fail, but checking again now shows it doesn't. Should I remove the test, try to make it check the flags, or something else?

Updated .blend due to changes in Laplacian smooth.

tests/python/operators.py
325–330	you can simplify with something like `{i for i in range(96)}` to select all. Here I'm selecting everything but multiples of 3, is there a concise way to do that? Either way, the operator is still returning inconsistently, making testing impossible.

Updating D11798: Add some operator tests #
Enter a brief description of the changes included in this update.
The first line is used as subject, next lines as comment. #
If you intended to create a new revision, use:
$ arc diff --create

Update added tests

Harbormaster completed remote builds in B17042: Diff 41884.Sep 16 2021, 3:47 AM

Generally these tests seem OK, requesting some changes though.

expectedPlaneVertConnectConcave

The example given could subdivide along a different diagonal (and still be correct).

A subdivision on the lower edge makes the result predictable (as shown with the active object):

This model should also include:
- An n-gon that is convex (and doesn't split)
- A concave quad (to ensure quads work as expected too).

expectedPlaneVertConnectNonplanar

Should include an n-gon as well as a quad.

expectedCubeVertConnectPath

This is a very simple example, connecting a path may cut through multiple faces/edges at once, it would be good to have a test demonstrates this.

This revision now requires changes to proceed.Nov 4 2021, 4:08 AM

Made the requested changes
I didn't write expectedCubeVertConnectPath, but I tried to make a better version of it. This required changes to mesh_test.py, as it previously wasn't loading the selection into the selection history, which the operator required to function on more than two vertexes. As a side effect, updated the expected mesh for checker deselect, as the selection history meant a different vertex was considered "active".

Harbormaster completed remote builds in B18705: Diff 44822.Nov 16 2021, 5:17 PM

Jake (Welp) edited the summary of this revision. (Show Details)Nov 16 2021, 5:19 PM

Campbell Barton (campbellbarton) requested changes to this revision.Nov 30 2021, 12:45 AM

Campbell Barton (campbellbarton) added inline comments.

tests/python/modules/mesh_test.py
340–342	Couldn't this use `items.ensure_lookup_table()` ?
344–353	Adjusting selection history should be optional, eg: `select_history=False` argument for `do_selection`.

This revision now requires changes to proceed.Nov 30 2021, 12:45 AM

Campbell Barton (campbellbarton) added inline comments.Nov 30 2021, 12:49 AM

tests/python/operators.py
325–330	To exclude multiples of 3`{i for i in range(96) if (i % 3)}`.

More requested changes

Harbormaster completed remote builds in B19078: Diff 45456.Nov 30 2021, 2:14 AM

Jake (Welp) edited the summary of this revision. (Show Details)Nov 30 2021, 3:27 AM

Don't use 'set' when select_history=True (since it's unordered and wont work predictably).
Use keyword-only argument for select_history.
Reduce line length.

Harbormaster completed remote builds in B19083: Diff 45461.Nov 30 2021, 7:32 AM

Campbell Barton (campbellbarton) mentioned this in rBL62765: Add some operator tests.Nov 30 2021, 7:36 AM

Campbell Barton (campbellbarton) retitled this revision from Add some operator tests to Tests: add edit-mesh operator tests.Nov 30 2021, 7:37 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Remove reference to set type.

Harbormaster completed remote builds in B19084: Diff 45462.Nov 30 2021, 7:41 AM

This revision was not accepted when it landed; it landed in state Needs Review.Nov 30 2021, 7:44 AM

Closed by commit rB7168a4fa5c78: Tests: add edit-mesh operator tests (authored by • HaydenBerkson (Jake), committed by Campbell Barton (campbellbarton)). · Explain Why

This revision was automatically updated to reflect the committed changes.

Campbell Barton (campbellbarton) added a commit: rB7168a4fa5c78: Tests: add edit-mesh operator tests.

Applied, note that VertexConnectConcave was failing with different loop order.
I updated expectedPlaneVertConnectConcave to make it asymmetrical so one face of the sides will predictably be chosen as the best face to split off.

Tests: add edit-mesh operator tests
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 45456

tests/python/modules/mesh_test.py

tests/python/operators.py

Tests: add edit-mesh operator testsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 45456

tests/python/modules/mesh_test.py

tests/python/operators.py

Tests: add edit-mesh operator tests
ClosedPublic
Actions

Revision Contents
Changeset List