Page MenuHome

Cleanup/Speedup Mask Modifier
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Fri, Feb 7, 5:58 PM.

Details

Summary

This patch rewrites the mask modifier. There should be no functional change.

Here are two test files, one for correctness and one for performance:


In the performance test file the playback speed is increased from 1.1 to 5.1 fps on my laptop.

Diff Detail

Repository
rB Blender

Event Timeline

In general for this kind of refactoring, it should be done in a few commits. Which makes it much easier to review, and to find the cause if a bug was introduced. The diff can be updated for every commit so that they can be viewed individually. Something like:

  • Split into smaller functions and change file to .cc
  • Convert data structures to c++
  • Improve algorithms

Since @Campbell Barton (campbellbarton) wrote this code, it would be good if he approved at least the idea of converting this to c++.

source/blender/modifiers/intern/MOD_mask.cc
59

Not part of this patch, but leave the Intrusive part out. It makes it seem like the wrapper is intrusive which it's not. It's called just ListBase everywhere, so calling it IntrusiveListBase makes it seem like there is something more going on than there really is.

This revision is now accepted and ready to land.Thu, Feb 13, 9:45 AM

Locally I committed these steps separately. Will the commits stay intact when I submit the code for review in the end (when using arcanist)?

source/blender/modifiers/intern/MOD_mask.cc
59

True, it is called Intrusive because this wrapper handles the case when the next and prev pointer are part of the struct itself (instead of using LinkData). I guess we can clarify the naming as when we have to wrap such list bases as well.

Will commit the change separately.

This revision now requires review to proceed.Thu, Feb 13, 11:01 AM

Locally I committed these steps separately. Will the commits stay intact when I submit the code for review in the end (when using arcanist)?

The commit message may get altered, I'm not sure. You can always try it on a separate branch or detached head.

Campbell Barton (campbellbarton) requested changes to this revision.Wed, Feb 19, 4:49 AM

Generally fine, some minor issues noted inline.

source/blender/modifiers/intern/MOD_mask.cc
50

This is causing a lot of errors on my system (mixing int/uint comparisons for eg).

For now it's simplest to remove. Although it would be nicer if these could be handled too, I'm not sure it's practical.

186–187

This is a more general issue with our C++ use in Blender.

I find passing references for output values is bad for readability.

See https://softwareengineering.stackexchange.com/questions/299021

A better alternative could be to use std::tuple.

This revision now requires changes to proceed.Wed, Feb 19, 4:49 AM
source/blender/modifiers/intern/MOD_mask.cc
186–187

The link on that page was broken, direct link to googles style guide: https://google.github.io/styleguide/cppguide.html#Reference_Arguments

  • remove strict flags
  • don't use references for return value parameters

I removed the strict flags for now. I did not get the warnings on windows, but I
do get them on linux now. I'll check how practical it is to fix all the cases.

I can agree with not using normal references for output parameters. However, I'm
not really a fan of using tuples/pairs as return values here (yet).

  • I cannot specify names for the returned values in the function signature (would have to use a comment).
  • Unpacking tuples/pairs on the caller side is a bit annoying (at least in C++11, haven't tried newer versions yet).
  • I don't like it when some values are returned as parameters and some values as return value (and I do think that using MutableArrayRef parameters is a good pattern).

I changed it so that pointers instead of references are used for output parameters.

This revision is now accepted and ready to land.Thu, Feb 20, 10:03 AM
This revision was automatically updated to reflect the committed changes.