Page MenuHome
Differential D11798

Tests: add edit-mesh operator tests
ClosedPublic
Actions

Authored by Jake (Welp) on Jul 4 2021, 12:43 AM.

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 17042
Build 17042: 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.
Himanshi Kalra (calra) requested changes to this revision.EditedJul 4 2021, 9:26 PM
Himanshi Kalra (calra) added a reviewer: Himanshi Kalra (calra).
Himanshi Kalra (calra) added a subscriber: Himanshi Kalra (calra).

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
Jake (Welp) updated this revision to Diff 39107.Jul 4 2021, 11:37 PM

Drop changes to mesh_test.py

Jake (Welp) added a comment.Jul 4 2021, 11:38 PM
  • 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.

Habib Gahbiche (zazizizou) requested changes to this revision.Sep 5 2021, 12:23 PM
Habib Gahbiche (zazizizou) added a reviewer: Habib Gahbiche (zazizizou).
Habib Gahbiche (zazizizou) added a subscriber: Habib Gahbiche (zazizizou).

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
Jake (Welp) edited the summary of this revision. (Show Details)Sep 16 2021, 3:38 AM
  • 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.

Jake (Welp) updated this revision to Diff 41884.Sep 16 2021, 3:47 AM
  1. Updating D11798: Add some operator tests #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Update added tests

Harbormaster completed remote builds in B17042: Diff 41884.Sep 16 2021, 3:47 AM
Campbell Barton (campbellbarton) requested changes to this revision.EditedNov 4 2021, 4:08 AM
Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).

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
Jake (Welp) updated this revision to Diff 44822.EditedNov 16 2021, 5:17 PM

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
336–338 ↗(On Diff #44822)

Couldn't this use items.ensure_lookup_table() ?

342–346 ↗(On Diff #44822)

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)}`.

Jake (Welp) updated this revision to Diff 45456.Nov 30 2021, 2:14 AM
  • 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
Campbell Barton (campbellbarton) updated this revision to Diff 45461.Nov 30 2021, 7:32 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)
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) updated this revision to Diff 45462.Nov 30 2021, 7:41 AM
  • 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.
Campbell Barton (campbellbarton) added a comment.Nov 30 2021, 7:49 AM

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.

Revision Contents
Changeset List

PathSize
tests/
python/
97 lines

Diff 41884

View Options

tests/python/operators.py

Loading...