Page MenuHome

Fracture Modifier
Needs RevisionPublic

Authored by Martin Felke (scorpion81) on Oct 6 2014, 8:22 PM.

Details

Summary

Brief description:

This modifier allows to break objects into many pieces, but keeps all pieces as separate mesh islands under one object. That is easier to handle, faster and doesnt clutter the outliner with many unnecessary separate objects.

User Documentation can be found here: http://wiki.blender.org/index.php/User:Scorpion81/Fracture_Documentation

Some basic design documentation can be found here: http://wiki.blender.org/index.php/User:Scorpion81/Fracture_Design

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
source/blender/blenkernel/intern/depsgraph.c
1361 ↗(On Diff #2683)

Why do these checks become necessary? Specifically:

  1. Are you storing empty "GroupObject" items in the rigidbody group for shards/fragments to populate later? OR
  2. Was this just something you patched in to solve a crash arising from some unknown cause?

I guess I'm just concerned here that the need to add extra checks here just ends up being a temporary patch until we find another place which assumed these are present and goes on to crash.

1371 ↗(On Diff #2683)

Codestyle: Going from single-statement to a block, it's better to add braces around the loop body. So,

for (go = ...) {
  if (...) {
     ...
  }
}
source/blender/blenkernel/intern/rigidbody.c
81

This could be simplified/improved by doing:

(rmd->modifier.mode & (eModifierMode_Realtime | eModifierMode_Render))

is more compact and still does the same job

81

Codestyle

102

Why this conversion? All rotation angles internally should be radians not degrees

source/blender/editors/physics/rigidbody_constraint.c
204

I'm surprised this operator isn't already in trunk. IMO, it'd be good to submit this as a separate patch which we can commit to master in the meantime, since it doesn't appear to rely on any of the other changes.

source/blender/editors/physics/rigidbody_object.c
524

Ugh! This conditional here smells of leaking implementation detail all over the show.

If you must retain the ability to directly pass a mesh + rigidbody in (for the shards), I'd rather have a method for that named BKE_reigidbody_calc_mesh_volume(). BKE_rigidbody_calc_volume() would then still take its current args,

source/blender/makesdna/DNA_fracture_types.h
76

This seems a bit bizarre, especially with the linked-list references above. So how exactly are these stored?

source/blender/makesrna/intern/rna_modifier.c
296

Firstly, why is this here in the general modifier update callback?

If you need to test for something like this for one modifier type, it would be better to create separate update callback for that modifier only (i.e. rna_FractureModifier_update) and check for fmd->refresh there instead.

Secondly, what's the point of this flag?

source/blender/makesrna/intern/rna_rigidbody.c
32

Why?

107

Leftover changes?

114

Leftover changes?

221
  1. Codestyle (asterix goes on side of variable, not type)
  2. Give your variables meaningful names, not just generic "val1" and "val2"
  3. reset could be defined as a bool
Joshua Leung (aligorith) requested changes to this revision.
This revision now requires changes to proceed.Oct 9 2014, 6:25 AM

I added some replies for some questions, which i am also not sure how to fix properly yet. Will update patch in next post, it adresses some of the issues for now only.

source/blender/blenkernel/intern/depsgraph.c
1361 ↗(On Diff #2683)

Those checks are temporary crash prevention fixes for having a fracture modifier with helper objects and moving them around in autoexecute mode, triggering continous "live" refracture. Will remove them and check whether crash is present... if it is.... i might need some help to find the original cause for it.

source/blender/blenkernel/intern/fracture_util.c
244

Ok, good point. Though this will apply to many, many Malloc calls in my code, should i really change them all ?

source/blender/blenkernel/intern/rigidbody.c
194

Probably yes. I wanted to keep my code in less files btw, reducing the need to change other unrelated files.

source/blender/editors/physics/rigidbody_object.c
524

I need to distinguish here between Mesh and Non-Mesh types, because the Fracture modifier can directly operate on Curves, Surfaces and Font objects as well, it internally converts them to derivedmesh and cleans that a bit up.
Furthermore, wouldnt that mean duplicate code if i have separate functions for calculating volumes from shards and regular rigidbodies ? I have combined that into one function BKE_rigidbody_calc_volume() ... why is that a bad idea ? Because of the different parameters ?

source/blender/makesdna/DNA_fracture_types.h
76

They are stored as array indeed, the prev and next pointers are used only in a special case where i have "shard islands", aka shards split further to their islands creating more meshIslands and rigidbodies in the simulation then being created by the intial fracture. But agree, its a bit odd here, probably i will change this to a Listbase and make use of the pointers (prev, next) there

source/blender/makesrna/intern/rna_rigidbody.c
32

This file (as some others too) contain remainders of a unlucky merge.... need to clean this up

Martin Felke (scorpion81) updated this revision to Diff 2694.

Adressed some of the issues of the first review, have added comments for points i am still unsure with before. And hrm, probably should have merged in latest master prior to diff... i diffed against origin/master this time, but my patch bases on an older master version, so there is even more unintended noise.... will do better next time :S

Martin Felke (scorpion81) updated this revision to Diff 2695.

This is a correct diff against current master, please forget the previous one....

slimmed down the patch a bit by leaving out whitespace only changes or unnecessary temporary fixes

sync with latest fracture_modifier branch, which was synced with master before

My main concern with this patch is that it goes to quite some lengths to have a kind of Object (a Shard) which then has to have duplicate code in rigidbody.c,
and RNA object handling in rna_rigidbody.c which works around shards not being in the depsgraph.

To properly support such a system, I think we first need to have a well defined way to support shards in a more general way (particles, duplis, sub-objects... ?).

We could always accept the current patch (mostly as-is), but I think we also accept too much technical debt along with it.

So, I would reject this patch, and only re-visit the feature if there is it can be implemented in a way which better fits Blender.

I realize quite a bit of work went into this patch, so if other developers think this can be updated to be supported in Blender, interested to know. But for now I don't see a good way to make small/medium changes to the patch that would make it acceptable for master.


(also, build has a lot of warnings which are in fact bugs, (callbacks not matching function signatures), likely cause of crashes, but I dont think its worth going over detailed code quality issues unless this patch can be done in a way which we can accept.).

source/blender/blenkernel/BKE_deform.h
31

is this really needed? - in general avoid adding headers to headers unless the purpose of the patch to re-arrenage header layout.

source/blender/blenkernel/intern/rigidbody.c
20

This file contains a _lot_ of copypasted code. I can see why its done, but seems like a bad workaround for shards not being objects.

In some cases I found older versions of the functions have been copied, where the original versions have since been changed. (maybe fixed?), see: rigidbody_get_shape_trimesh_from_mesh_shard for an example.

674

another almost exact copy, not great.

674–760

This is an almost exact duplicate of rigidbody_get_shape_trimesh_from_mesh, (looks like this file contains a lot of copy-pasted code, which should be a last resort).

1390

this is copypasting ~165 lines of code, (almost verbatim).

seems like it could be avoided in this case.

source/blender/editors/object/object_modifier.c
2378

This line fails, and can possible cause crashes.

source/blender/makesrna/intern/rna_modifier.c
659

Why is this added from unrelated modifier?

671

Unused function (noise for code-review), prefer to #if 0 to denote it being unused.

source/blender/makesrna/intern/rna_rigidbody.c
151–242

having to look inside all modifiers mesh islands on RNA update seems very heavy for an update function.

It seems like you're working around each shard not being a real object or apart of the depsgraph.
This isnt really good - there could be 1000's of shards and having to loop on all of them to set a value, is problematic. For example - you may want to animate some setting with an FCurve (thats one of the uses of RNA), and this isn't well suited to using heavy callbacks like this.

But this isnt so simple to solve, perhaps these shards could be moved into the depsgraph (not trivial),
or their settings could be updated with an operator (simple but clumsy).

686

Why is this changed?

797

Why is this removed?

source/blender/modifiers/intern/MOD_boolean_util.c
68

Improvements to boolean should be handled separately (Sergey, mind checking?)

415

Allocating layers for left & right seems risky, since the indices will collide between the 2, better just use the right, if they are aligned its OK, otherwise its unsupported to support a true, name based merge.

source/blender/windowmanager/intern/wm_jobs.c
95

there is no need for this, startjob takes a progress pointer which can be reused.

565

Having special checks for fracture here raises some alarm bells, surely it can be made to work with existing job system?

Changing to a different compiler, the gcc-4.9 compiler that Ideasman probably uses, and adding stricter error reporting seems to give similar errors as mentioned. From the output and some quick fixes it looks like there are only a couple real errors of concern from the issues use of the new compiler reports. The rest look like they are simple fixes. Scorpion81 will have more information in his response.

Also, in going over the issues with scorpion81 in IRC, some of the concerns raised here are with features that can easily be taken out until a better implementation can be made of them later. With those two things in mind IMHO Ideasman's concerns can be addressed adequately.

Scorpion81 will post more complete information in his response. His answers and modifications will make a difference as solutions to the issues raised IMHO.

Respectfully,
JTa

A Maniphest design task has been opened to discuss keeping or removing specific features from a design perspective. It is also intended to keep design issue comments out of this patch page. Fracture Modifier Maniphest design task: T42306

This patch was submitted for review and feedback not as a completed patch to be accepted or rejected as-is but as a patch for review to get feedback and clarify exactly what needs to be done so the Fracture Modifier can be considered for acceptance into Master. So the link below to the bf-committers post is listed here to clarify some issues and a request for help.

Post link: Bf-committers Fracture Modifier - call for hints / advice / help
This was posted by scorpion81 as a request for more feedback and help on completing or removing features that had coding issues or bugs.

Please pardon any misunderstanding on the exact method needed to get feedback on properly preparing the Fracture Modifier for consideration and inclusion in Master. Scorpion81 has some additional information and solutions he will post here regarding the issues raised above by reviewers.

As a stakeholder in and contributor to this project, thank you for your consideration and assistance.

Sergey Sharybin (sergey) requested changes to this revision.

Here's the first round of review, and it's already quite a few of changes to be done.

There're loads of smaller issues like not following the code style for braces, copy-pasted code.. But there are also major issues with design even:

Seems you've added Shard datablock which is basically the same as Mesh, with the only difference it's not an ID. This raises quite a few of issues, i.e. how do you synchronize custom data between them? Biggest issue was already raised: they're not in the dependency graph, which means you're handling with the updates manually, which is rather really bad.

Caching is also something rather weird here. Storing a DM in modifier data is really asking for huge headache. You'll end up with the same issues we're having with particles and hair. Plus some code seems to just depend on whether there's a fracture modifier or not, without taking care of modifiers before the fracture? Plus, what if you've got several fractures?

And last for now but not least, seems rigid body now has a special case for shards, which rather conflicts with our current attempts to unify the physics

Plus the inline comments from @Campbell Barton (campbellbarton) which i didn't duplicate.

I wouldn't probably call the project rejected, but with current state it's rather no-go for the master branch. For until it fits into other areas (especially depsgraph, for which you might want to wait for the refactor project to be finished) nicely at least.

extern/voro++/CMakeLists.txt
1
  • Copyright header.
  • I'm a bit conservative about using non-[0-9A-z_] for file and directories names.
extern/voro++/README2
1

Not sure we need this?

extern/voro++/SConscript
1

Copyright.

extern/voro++/src/Doxyfile
1

I'd leave this out.

extern/voro++/src/LICENSE
1

You already have a license in the parent folder.

extern/voro++/src/Makefile
1

We don't support gnu makefiles.

extern/voro++/src/Makefile.dep
1

What is it for?

extern/voro++/src/README
1

Don't think it's needed?

extern/voro++/src/cmd_line.cc
1

Is it really used?

extern/voro++/src/v_base_wl.cc
1

Is it auto-generated?

extern/voro++/src/worklist_gen.pl
1

It doesn't seem to be used?

intern/rigidbody/rb_bullet_api.cpp
572

It's not in the header.

release/scripts/startup/bl_ui/properties_physics_common.py
97

ob.type in ['CURVE', 'SURFACE', 'TYPE']

source/blender/blenkernel/BKE_deform.h
31

Currently implementation files are expected to take care of header includes.

source/blender/blenkernel/BKE_fracture.h
18

Year.

source/blender/blenkernel/SConscript
168

That's a bit weird to have carve forced to be added to the include lists for CMake but not for SCons.

source/blender/blenkernel/intern/fracture.c
414

Use ELEM().

438

0.0f

823

Braces doesn't follow blender style.

900

We don't really use camel case.

905

Caching derived mesh in modifier data is always asking for a really huge can of worms.

source/blender/blenkernel/intern/pointcache.c
1036

copy_v3_v3() and copy_v4_v4()

source/blender/blenlib/BLI_edgehash.h
33

We don't have includes in headers unless it's really essential.

source/blender/blenloader/intern/readfile.c
166

This must not be needed. DerivedMesh is totally volatile, being created only when it's needed in the scene update routines. No DM operations should happen on read/write.

source/blender/bmesh/tools/bmesh_decimate_dissolve.c
330

I think it should not be needed. Callee function is to be responsible to pass all the data. You can add BLI_assert() here, but not a silent failure.

source/blender/editors/object/object_modifier.c
885

That's just delaying the headache. No data available for RW should be shared between threads. It is possible to screw up data used by fracture job in loads of other ways than just removing the modifier.

2313

As i understand, the ob here is the object from the scene, which means you're forbidden to call makeDerivedMesh() outside of the scene update, especially from the job.

source/blender/editors/physics/rigidbody_object.c
526
  • You don't free the DM.
  • I'm not even sure why you need to create DM here.
source/blender/makesdna/DNA_rigidbody_types.h
275

Why not to use vectors?

source/blender/modifiers/intern/MOD_boolean_util.c
94

This doesn't seem to belong to loop_set function?

This revision now requires changes to proceed.Oct 20 2014, 11:24 AM

At first, sorry for the delay of my reply. Because the patch was not completely final, i have been in a process of deciding what to do with each feature which depend on others help. Part of that decision were attempts of mine to fix the issues myself, which did only succeed partially. And last but not least i have been waiting for the last pending review.
[Edit: Final review has been done in the mean time, will answer further issues in a next post]

Now i would like to answer to the general implementation issues which have been raised so far. (Detailed code issues will be answered inline, first i need to prepare a fixed patch)

RNA

First, the fracture settings are valid for the time of pre-fracture process only. They are not subject of being animated. Thats why the "Animation" flag has to be set to false explicitly by me in the future.

Changes would be triggered manually by the user only via GUI or via python, as the rigidbody simulation must be pre-configured in general, because each change invalidates cache and requires a rerun of the sim from the startframe. I intentionally limit the fracturing process to frame 1 or the respective rigidbody cache start frame. At all other frames, changes to the simulation parameters invalidate the cache and require the simulation to be restarted.

Changes which require a refracture wont be taken into account until a refracture will take place again, which will then update the geometry cache (in form of fracture shards and simulation meshislands). As animation of parameters would be deactivated then, the longer RNA callbacks will be called only once at each fracturing process or fracture simulation setup step respectively and not permanently during simulation, thus they should not affect performance negatively in general.

Rigidbody

At first, yes there is some duplicated code in rigidbody.c. But it turned out to be the best way for me not to change existing functionality and existing function parameter signatures.
That would likely have caused even more changes in other parts of the code. And changing even more unrelated code likely increases the chance of new bugs being introduced.
Generally inlined code is easier to access then changing header inclusion definitions elsewhere, keeping self contained. Especially when it contains adapted versions of functions for the modifier, with fixes and improvements even.

Also the amount of duplicated code is relatively small, there are like 4 new rigidbody functions being adapted to the shard system. They could be deduplicated, but that would require to change the function signatures, which can have disadvantages as stated before. The decision whether using the regular rigidbody system or the modifier would then be made inside the functions instead of creating dedicated functions for it. But having more code in less functions reduces the readability of the code in my opinion.

Initially i wanted to convert the entire rigidbody system to use multiple rigidbodies per object, via a modifier. But I agreed with sergof to just add my modifier side by side to the existing rigidbody system. There also has been some discussion about the constraints in the modifier. In my opinion them being separate objects is a bit harder to handle and slower because of the object overhead for the empties and the draw overhead if constraints are visible by default. And in fracture case you barely need to change constraint type or location. Using constraints in the modifier has some excellent benefits like easy and fast global manipulation of the constraints, and you can easily switch them all on and off. Furthermore they are fast to be generated and cause no object/draw overhead, they support special breaking conditions like breaking angle, which "de-rubber" the simulation behaviour massively compared to using thresholds only.

Depgraph

As the fractured object is still 1 object only, the depgraph has to handle just one object, and not possibly 1000s of separate objects. The shards are handled by bullet and modifier explicitly, as they just manipulate the mesh during the simulation. They are not meant to replace the objects, but just a way to group mesh parts to islands and cache them for simulation purpose. For convenience the shards and meshislands are also stored in the blend, to have them ready at loading time of a blend. This way a refracture at loading times is not necessary.

As depgraph is not responsible for mesh manipulation, that should not become a performance issue here or interfering with further depgraph development.
Fracture is a specialized tool and not a general animation tool for e.g. character animation, and therefore doesnt require full depsgraph compatibility. So it cannot be properly generalized to other blender concepts, but instead adds a new concept.

Shards are not objects there, but subgeometry objects. Fractured / Multiple rigidbodies being controlled by a modifier, is the generalized case of the current regular rigidbody system, because it handles single multiple rigidbodies, which can interact in a simulation.

Although it iterates over shards during sim, it's still faster or at least as fast as single objects.
According to the profiler, bullet is the bottleneck there, when dealing with many convex hulls.
Performance will get worse if shard amount increases, but that is the same issue as with single objects.

Own Issues i consider important to fix instead of removing them

First, I could need some advice/guidance about which issues could be fixed in which way. I would rather like to fix them, because the value of the modifier would be reduced as its usage possibilities would be more limited.

For now the mentioned features just work and are usable. They dont harm or affect different areas of blender, as most code is concentrated in a small number of files. Modifications in other files were either absolutely necessary for the modifier to be setup, or contain fixes and improvements to issues which were revealed by using the modifier.

Issues in detail:

Weightpainting with active fracture modifier works only if modifier is invisible and requires a refracture to interpolate vertex data down to the shards, this could be fixed by the autoexecute flag of the modifier. But if this is enabled, weights are painted at wrong locations on the mesh. I think its just some transformation issue along the mesh conversion process (BMEditMesh) necessary for drawing the meshes.

Support for all customdata is missing, its currently limited to textures and weights only.

Proper greasepencil support -> got a crash with sketching session and autoexecute, cant run both at the same time.

I will investigate those issues further, but i really could need some guidance or advice for it from other developers.

Issues Fixed by Feature Removal

Some advanced features are WIP still, but i am kinda stuck with them currently. They are subject to be removed due to impossibility to properly implement in time:

  • fracture job system -> was an attempt not to block UI during longer fracture process (it's designed for being optional atm), but causes threading / memory access problems (crashes), even after one fracture thread has finished, and another one is to be started
  • sub object groups: -> supposed to combine meshes of differently fractured objects into one object without applying each modifier -> lacks customdata support still and can be used by re-using existing islands only (may crash else)
  • non mesh object support: * can directly fracture curves, surfaces and fonts, mesh conversion is done in the modifier and is temporary -> wont render in cycles, modifier results not taken into account there, BI works
  • complete editmode support: * supposed to pick single shards and manipulate them in editmode -> currently selects all elements at once, possibly the internal drawing mesh is not properly updated
  • inner vertex group -> filled by modifier, but contains "too many" verts, meaning even faces from the outer shell will appear with e.g. the mask modifier after it

Issue here is, current Blender design just doesn't fit will with this patch.

We can postpone this patch (instead of rejecting), since the functionality is useful.

But really - this will only be accepted if Blender's internals are extended to support it better, and making these changes is a significant project.

One way forward could be:

  • Plan how this would work, how Blender's API's would have to change to support this patch.
  • Break the planning into smaller tasks which can be done in steps.
  • Complete each task and keep the fracture branch in sync with master.

... even then, I think these dot-points over simplifies the process. Having a modifier store mesh data is difficult to manage with blend file format, bullet, depsgraph... etc, working nicely is just not a straightforward project, even quite experienced developers may need to try different ideas out before settling on a good way.

release/scripts/startup/bl_ui/properties_physics_common.py
97

Better static set {'CURVE', 'SURFACE', 'FONT'}

Thanks everyone for your feedback and insight. With the new information we are going over all the points and issues you raised and evaluating them against the design and implementation of this modifier.

This modifier obviously has some unique characteristics so we may ask for some exceptions to the rule when needed and if for a good reason. We will clarify those when the review to do list is shorter.

We will do some smaller cleanup items from the list as we are considering the bigger design changes.
We will post information to the design task page as needed and post here as needed for coding feedback.

Thanks again for your assistance. This tool is not just by a single developer asking for a feature to be included. This project has a committed and growing development team with testers and artists who use this tool in production.

Thanks once more.

Lots to respond to here. The team has been actively discussing and working on responses and solutions.

Starting from the top down the voro++ library inline comments are simple and will be looked at during a cleanup of the whole external library depending on what is the best practice on trimming an extern library. Thanks for the copious tips on that and other cleanup points. : )

On the caching of the DM in the modifier, we will post some more enlightening design decision information on that on the design task #T42306 and why it can be a positive and not necessarily a negative. Just briefly adding FM to the mix can help clarify the issues and solutions. It does not necessarily make them worse in our opinion.

The same goes with the deps graph and update issue. FM has a different workflow and desired results than typical animations so deps graph is not relevant at this point to the artist who use it. More enlightenment on this design choice will also be put on the design task page.

The FM works good with some of the popular modifiers to use with it. An example will be posted in the design task of smoke and fire being used with it. Currently the FM should be the last modifier in the stack as a matter of the workflow. Ongoing testing and improvements are being made in other modifier interaction and multiple FM interactions.

The special case for shards has been modified and now has a send to key frames feature that addresses the main concern. It is almost complete and fully functional already. And again, an additional core dev for physics and the rest of the development team can assist in the development of a unified system and make it better not worse. The team already has a good working relationship with the physics devs and has already had several lengthy and still ongoing conversations in #blenderfracture.

Sergey's additional inline comments are being resolved. Thanks for some great insight on specific lines of code. Some have fixes already in progress others are dependent on other design issue decisions. We have been regularly discussing these in #blenderfracture.

"Issue here is, current Blender design just doesn't fit will with this patch.
We can postpone this patch (instead of rejecting), since the functionality is useful."
Exactly the development team's opinion except on the other side of the coin. We feel Blender master will benefit by the extra work and testing because it shines a light on specific issues and helps show good ways to improve Blender and the unified system for modifiers in general.

"But really - this will only be accepted if Blender's internals are extended to support it better, and making these changes is a significant project."
The bullet points below this statement are already happening and have been discussed even before this code review. The commitment of this team has already been long term especially by the core developer. There is no reason to believe it will be any different than any other Blender development going forward. We are hoping with any issues that persist that we can make a good case for getting an exception.

"...even quite experienced developers may need to try different ideas out before settling on a good way." Excellent way to put it. Thanks for all the great advice.

The team and the core dev have been working on answering all the issues and clarifying why exceptions maybe requested. There has been a lot of work put into the FM and more work is not a daunting task for a team that is passionate about it like we are. : )

There has already been some excellent and clever solutions to the issues coded and implemented. They will be documented in the design task page and here as necessary once they are better tested. We will post more info in smaller responses as we keep working on this fantastic tool. Thanks again for such detailed insights and recommendations so far.

The team genuinely feels that eventual inclusion of this tool will make Blender better and not worse in many ways not just for artists but for the coders and developers.

Since @Martin Felke (scorpion81) continued to work on this branch... reply regarding some general design issues.

Talked with both @Martin Felke (scorpion81) and @Sergey Sharybin (sergey) on this... and conclusion is attempting to store an entire physics sim for many rigid-body meshes inside a modifier (and in blenders DNA) isn't really an acceptable design (its some kind of hack to put a local scene-graph inside a modifier).

Currently Blender isnt ready for this kind of feature, two possible future options available:

  • new instancing system to replace dupli's (possibly node instancing), may be able to support mesh shards.
  • caching system (such as alembic) may allow tools to be written that generate physics sim to a baked into a file for playback.