Page MenuHome

Mantaflow Review
Open, Needs Information from UserPublic

Description

Since Mantaflow project review itself is split in over ten patches, think it's worth having a general task to link them all, and also to manage general requests and discussion.

Detailed review is to happen in relevant differentials.

Event Timeline

@Sebastián Barschkis (sebbas) think first step here would be to merge (new, 2.8) master into the branch, feels a bit odd to work on code for 2.7x generation. Gave it a quick try, but there are some noisy amount of conflicts, better to be solved by someone who knows the code. ;)

Bastien Montagne (mont29) lowered the priority of this task from Normal to Needs Information from User.Jan 24 2019, 6:08 PM

@Bastien Montagne (mont29) Yes, that's definitely a good idea. I just updated the diffs (now it's branched from the 2.8 "master") and also synced it with the fluid-mantaflow branch which can be used for test builds. From here we could now get the review rolling.

Hi everyone, great to see the manta integration being re-revived! @Sebastián Barschkis (sebbas) thanks for the large patch collection. I just checked out and tried the revised diff "from scratch". Works nicely on my MAC. The sims also run through without problem (I tested based on the quick-smoke and quick-liquid setups). So I think this patch looks good, overall. There are some obvious next steps, like removing the old liquid/smoke modifiers, and cleaning up the code, but I think this would be better to address once the manta code is integrated.

I think the basic simulation functionality has also been tested in depth over the course of the last year (and the core solver has been in use for many years), so that should be unproblematic. The integration uses the existing interface for smoke & liquids (the "old" solvers), so I think the key point to look at are the changes to the blender interface. This is relatively straight forward though.

@Sebastián Barschkis (sebbas) - I noticed the exported python scene has a few lines at the top for OS/multi-processing checks that don't seem to be used or necessary. (from "withMP ... up to VARIABLES", ca. lines 8 to 18). Those could be removed, right?

Can someone remind me why this is integrated through Python rather than C++, and how much work it would to change it to C++?

Using Python for modifiers is really bad performance due to the global interpreter lock, and just generally seems to add an additional layer between Blender and Mantaflow whose purpose I don't understand. Not saying it's a showstopper, but I would like to understand what our options are here.

@Nils Thuerey (n_t) Yes, you're right. Those lines won't be needed in the future anymore. Will remove them.

@Brecht Van Lommel (brecht) Right now the only way to access all Mantaflow functions is through Python. In terms of performance I wouldn't say that this is an issue. Once the Manta solver acquires the GIL it will compute one "step" and only once completed release it. The time between steps where other resources (like the UI) can acquire the GIL and Manta has to wait for it would be the only delay during a bake job. This delay is negligible from my experience unless there are other resources that could block the GIL for a very long time.

An alternative to this setup would be to use Mantaflow's C++ API. I am not sure though how far the development of that is, @Nils Thuerey (n_t)? And even if it is ready, I don't think using it would bring a performance boost as all code that is called through Python right now runs in OpenMP / TBB. Using a C++ API would also make the solver setup more "static". The current API through Python is more flexible. It makes it really to export the Blender scene as a standalone Mantaflow script. I think it would also be helpful to have Python access for a future node based solver.

There's a pull request from David Ullmann extending the NOPYTHON option.

@David Ullmann (robocyte) Perhaps you can give some insight in regards to performance?

@Brecht Van Lommel (brecht) Good question, to add to sebbas comments, there are a few reasons for the python bindings: a first mundane one is that that's how the mantaflow solver worked originally, and it would have been a lot more work to switch to C/C++ bindings. Also, the performance impact is negligible, as mantaflow provides all the solver building blocks via python, but each of the steps is quite expensive. So there are no low-level operations in python (e.g. access to grid cells), but just a small number of calls to high level functions that typically make several passes over the full grid.

And then the python bindings provide some interesting outlooks: it will make it very easy to merge any future improvements from mantaflow back into blender, and hopefully at some point also allow for very flexible simulations. E.g., users could add own scripted hooks and modifications via python into the simulation loop to change the way the simulation runs without having to recompile. In general, it makes the data flow in the simulations very flexible. In the long run, I think it would also be awesome to have a kind of node editor to set up the simulations, but that's a bit further away :)

Ok. The main concern is not so much that it's slow for fluid simulation by itself, but if multiple performance sensitive areas use Python it does become a problem to have these global locks. If this is how the Mantaflow API works and it's a big burden to use C++ instead it seems acceptable.

For further customization, we should try to figure out a way to fit this into the new node system that is being designed, rather than using Python scripting. We are still trying to work out the design for these kinds of systems, but if there are specific ideas regarding Mantaflow nodes it would be interesting to see what they could look like and how they can fit in.
https://wiki.blender.org/wiki/Source/Nodes

@Brecht Van Lommel (brecht) Okay, yes - the GIL seems to be unproblematic so far. It could make the UI somewhat unresponsive when baking large sims, though. The new node system notes look interesting. I think for mantaflow it would neat to go for the "everything-nodes" direction, where a node graph would define the data flow of the simulation. This could then use a python back-end, or directly hook into the mantaflow functions (the functions that are exposes to python are the crucial ones that ideally would also be available as nodes). It's definitely a good point to keep in mind.

I have another question here for the experienced blender devs (@Brecht Van Lommel (brecht) @Sam Van Hulle (sam_vh) @Martin Felke (scorpion81) @Jacques Lucke (JacquesLucke) and maybe others?): what would be the best next steps for an official 'approval' of the mantaflow diffs? I saw the 2.8 timeline, and I guess it would be ideal to verify that the patch is in a good enough state some time soon. Then it could be merged into the main branch as soon as possible after 2.8 is released in July, such that it can be included for the 2.81 release.

We could also continue with smaller fixes on @Sebastián Barschkis (sebbas) branch on the side. These shouldn't change anything within the larger structure of the diff here, but there are a few smaller cleanup and UI fixes that would be good to address.

The plan is still for @Bastien Montagne (mont29) to do the initial code review. @Bastien Montagne (mont29), will you have time to do this in the next few weeks? Spending like one day to go over the code and testing the functionality.

Yes, it’s still on my radar for this week or next one…

@Nils Thuerey (n_t) Hmm, looks like the branch could use a merge with master again. ;)

Some initial usability review after using the branch for a few tests, and a first quick review of the patches:

Results looks great!

Am by no means a specialist of that area, but there is definitively a serious improvement over previous fluid/smoke sims. And I have yet to try all the advanced stuff (especially guiding…).

Usage is less 'user friendly' than with previous sims

  • You have to bake to see anything, there is no real-time preview/simulation done during play.
  • For fluids, besides the Show FLIP option which can give you an idea of the result, you even need two steps of baking to get a result (first bake the simulation, then bake geometry and/or particles from sim).

I don’t have a very strong opinion on that, it is more a topic to discuss with artists I guess. Yet I’d expect it to make the 'entry point' for beginners/casual users much higher at least… Realize this is also likely a limitation of the python binding design, this could probably not work well in a real-time context?

On the other hand, the ability to separate baking in steps can also be very useful in more production-oriented cases I think, since you save time by not having to recompute everything if you want to e.g. generate finer mesh.

And the possibility to pause and resume baking is also a nice feature.

Showstoppers

IMHO, opened to discussion, and other devs are welcome to shine in on that topic of course.

Cache Handling

If you go to Edit mode, or try to render non-first frame of the (fully baked) simulation, you lose everything and get dummy domain cube again, until you go back to first frame and 'scroll' again… This is a caching bug, which does not affect cloth simulation in same branch e.g., so think there is an issue in how mantaflow integration code handles this?

Incidentally, there is also missing feedback about cached frames in the timeline (the colored band in the cached range of frames).

No Versioning & Crash

There is no versioning done at all. This means that currently old elbeem sim remain, old smoke one is kind of badly converted to new manta (and crashes)… this should be addressed, in worst case by just nuking old sims altogether.

Trying to open a file with some old sims just crashes for me:

.

Misc

Am getting those messages repeatedly in the console:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "<string>", line 143, in bake_mesh_74
  File "<string>", line 128, in bake_mesh_process_74
NameError: name 'sm74' is not defined

Names are not fully consistent (e.g. the modifier place-holder is still called Smoke, think Fluid would be best here, since it’s the name chosen for the Physics Sims button…). This seems to also sneak in code, where sometimes smoke is used for general MantaFlow handling (in Cycles class name e.g.)…

The old fluid sim code seems to still be around, you can even still use the 'fluid' quick effect operator to set up one, though only half-exposed in the UI then… Think that one should be fully removed.

Adaptive domain is missing (marked as TODO in some areas of code), not a showstopper imho, but still a feature regression.

Code Wise

Some more detailed comments will follow in each differentials, but from general point of view:

  • Why try to re-use existing Smoke modifier code? To me it only adds confusion, makes naming fuzzy (prefix vary between defines/enums/structs/functions/etc., from old smoke to (way too) generic fluid to few cases using better manta one…). Some names are even plain old ones from smoke, that is utterly confusing.
  • Old Fluid (elbeem) sim code remains half-active, user cannot directly see/edit it but it can still be added indirectly (through quick effect e.g.), this should be cleaned up I think.
  • To me it looks like the patches have been split by some kind of logical categorization, but unless am mistaken, they are not independent (as in, I doubt much previous patches can be applied and compile without the last one (D3861)). This is fine for review, but has to be kept in mind for final commit into master (we want all commits there to build and not crash ;) ).

Note: just updated the branch against latest master (was kinda painful, thanks to some 'cleanup' in BKE physics files :( ), would be nice if patches could be updated too.

@Bastien Montagne (mont29) thanks for the detailed comments! We'll have to more carefully look at the points you mentioned. Good idea to make the naming scheme consistent, to properly indicate the parts of the new solver. I'd suggest using "manta" there to distinguish it from the old smoke and liquid version.

And you think it would be better to remove the elbeem and smoke solvers directly? That definitely would be a good next step. The manta code was built using the previous smoke version, hence all the similarities. The old smoke modifier should be removed though. And the liquid one as well, together with all the elbeem code...

The cache handling looks like a bug, I'll try to reproduce that. And regarding the old smoke and liquid sims, I think it would be safest and easiest to add a warning they're not supported anymore. The smoke ones potentially could be converted to some extend, but the liquid sims work quite differently now.