Page MenuHome

Add more tests for the generative modifiers
AcceptedPublic

Authored by Jesse Y (deadpin) on Tue, Jan 14, 8:59 AM.

Details

Summary

Adds new Generative modifier tests using the test framework committed earlier this week. The aim is to start drilling into the common settings typically changed in each modifier, rather than just testing the defaults, as well as automating regression tests for bugs that get filed.

New tests:
Array

  • A FIT_LENGTH array using Relative Offset and Start/End Caps
  • A FIT_CURVE array using Constant Offset
  • A FIXED_COUNT radial array using Object Offset and Merge First/Last

Decimate

  • Collapse decimate at 25% with triangulation
  • Dissolve decimate at 30deg
  • Unsubdivide decimate at 2 levels

Mirror

  • Normal mirror (local coords)
  • Global mirror using an object offset helper
  • A radial bisect test chaining together the X, Y, and Y-flipped axis
  • Regression test for T58411 (mirror not passing on weight/crease data)

Screw

  • An asymmetric object screw'd using the offset method with interesting angles, iterations, and steps
  • An asymmetric object screw'd using a helper object

Solidify

  • A "Complex" mode test where result is drastically better than the default "Simple" mode
  • Regression test for T63063 (solidify w/open mesh)
  • Regression test for T61979 (solidify w/high quality normals option)

SubD

  • CatmullClark sub-d test
  • Simple sub-d test
  • 2d crease test
  • 3d crease test

Weld

  • Normal weld scenario showing typical merging behavior
  • Weld scenario which welds all vertices down to 1 vertex (large threshold)
  • Weld scenario where nothing is welded (small threshold)
  • Weld scenario after Screw modifier (one of the two primary motivators for having a weld modifier)
  • Regression test for T72380
  • Regression test for T72792

New modifiers.blend (updated 2020-01-20):

Diff Detail

Repository
rB Blender
Branch
addmodifiertests (branched from master)
Build Status
Buildable 6337
Build 6337: arc lint + arc unit

Event Timeline

Jesse Y (deadpin) edited the summary of this revision. (Show Details)Tue, Jan 14, 9:00 AM

Formatting cleanup

Add 3 more regression tests and 1 more functional test

Jesse Y (deadpin) edited the summary of this revision. (Show Details)Sat, Jan 18, 1:31 AM

From a first look, the tests look fine and they do indeed seem to cover relevant tests cases, so thanks for the patch (I haven't verified them in detail though, e.g. whether theses test can indeed fail of if for some reason one will always pass even if it shouldn't etc...). However, I do have some comments:

I don't think adding the extra parameter "scene" to the core class MestTest is a good idea for the following reasons:

  1. It adds complexity to the code / breaks the api. For instance:
    • documentation is missing in MeshTest,__init__(self) and in example usage in ModifiersTest is now outdated.
    • the parameter scene is not saved in self.scene like the other parameters
    • for the operators test, the parameter is now redundant.

but has no real benefits. For instance:

  1. when you enter a wrong scene name, you get an exception 'context is incorrect', making writing tests and tracking errors just a little bit harder for no reason.
  2. I thought the idea behind adding a different scene is to avoid extending tests along a different axis, but you still do it here, there is no benefits for extra scenes

So here are my suggestions:

  1. Just extend tests along the Z-axis, so do exactly like was done in operators.blend. Collaboration should be easier this way, since you should be able to tell the difference between two blend files faster by just having to look at the file in orthographic mode, without having to switch through scenes / views.
  2. Add comments to numerate the tests (e.g. add comment every 5 or 10 tests) because the framework identifies the tests by indices for now.
  3. If you really really really want to create new scenes, then make the MeshTest scene agnostic (the same way it is collection agnostic). So you could simply detect the scene before applying the modifier/operator and then switch to it.

Please let me know what you think.

Habib Gahbiche (zazizizou) requested changes to this revision.Sat, Jan 18, 4:54 PM
This revision now requires changes to proceed.Sat, Jan 18, 4:54 PM

Go back to using just 1 Scene and re-number the tests...

Jesse Y (deadpin) edited the summary of this revision. (Show Details)Sat, Jan 18, 10:33 PM
Habib Gahbiche (zazizizou) requested changes to this revision.Sat, Jan 18, 11:09 PM

Patch looks good to me except for some minor issues (see inlined comments).

tests/python/modifiers.py
213

Comment blocks here are reserved for the text you can see in the blend file, e.g. 'List of Deform Modifiers' or 'One Generate Modifier'. Do you mind staying consistent?

260

These contain two modifiers, so technically, they should be under 'List of Generate Modifiers' in this file and in the blend file.

277

There is already a subdivision surface collection (see test # 9). Could you add these tests to it instead of adding a new one?

280

Could you write the full name with no acronyms, i.e. expectedCatmullClarkSubsurfDecimate instead of expectedCatmullClarkSubD. It makes searching much easier.

286

I suggest using testCrease3dSubdivisionSurface instead of testCrease3dSubD

This revision now requires changes to proceed.Sat, Jan 18, 11:09 PM
Jesse Y (deadpin) edited the summary of this revision. (Show Details)Sun, Jan 19, 11:29 PM

Addressed comments as best I could:

  • Changed SubD slang term to SubdivisionSurface
  • Removed the test comment blocks
  • Placed all tests using multiple modifiers in the "List of Generate modifiers" section of both the .py and .blend
  • Placed all tests using a single modifier in the other section of the the .py and .blend
Jesse Y (deadpin) marked 5 inline comments as done.

Add test for the Complex solidify mode

Jesse Y (deadpin) edited the summary of this revision. (Show Details)Tue, Jan 21, 1:09 AM

I don't see any test that cannot be reproduced reliably and the tests do fail when the expected object is changed, so the changes look good to me.

@Howard Trickey (howardt) do you mind pushing this commit (with @Jesse Y (deadpin) as author) if it looks fine to you as well?

This revision is now accepted and ready to land.Sat, Jan 25, 6:47 PM