Details

Reviewers

Sergey Sharybin (sergey)
Nils Thuerey (n_t)
Georg Kohl (GeorgKohl)
Bastien Montagne (mont29)

Maniphest Tasks

T59995: Mantaflow Review

Commits

rB2aa4301c88aa: Mantaflow [Part 2]: Added fluid wrapper files
rB6cf9f1e525ec: Mantaflow: Cleanup debug output and some refactoring
rBc3ba70bbae7b: Mantaflow: Updated fluid wrapper and script files
rB9eeaf303574d: Merge branch 'mantaFiles' into mantaWrapper
rBe9413e9ec6b8: Merge branch 'mantaFiles' into mantaWrapper
rB8a498582b3b0: Mantaflow: Refactoring for smoke outflow
rBa1a1d4300ecd: Mantaflow: Improved allocation of fluid outflow objects
rB2b3ada655a49: Mantaflow: Added fluid wrapper files

Summary

Files from /intern/mantaflow handle the communication between core Blender code and Mantaflow itself. It's the bridge to communicate with Mantas Python functions.

Code from /intern/mantaflow/intern/strings/ is pure Manta code and would likely need less attention in the review.

Diff Detail

Repository: rB Blender

Event Timeline

Sebastián Barschkis (sebbas) created this revision.Oct 29 2018, 4:47 PM

Added changes from new outflow object allocation setup.

Fix for smoke outflow (walls and outflow objects)

Martin Felke (scorpion81) added a subscriber: Martin Felke (scorpion81).Nov 25 2018, 5:33 PM

Jérôme Matthey (JeromeM) added a subscriber: Jérôme Matthey (JeromeM).Dec 28 2018, 11:29 PM

Bastien Montagne (mont29) added a task: T59995: Mantaflow Review.Dec 30 2018, 10:07 PM

Update manta files after merge with master.

Harbormaster completed remote builds in B3292: Diff 14643.Apr 6 2019, 2:13 PM

Cleaned up debug out and did some minor refactoring.

Harbormaster completed remote builds in B3302: Diff 14653.Apr 6 2019, 6:19 PM

Made some comments about style, not sure how strict we want to be there though, but this is in /intern/… @Sergey Sharybin (sergey) or @Brecht Van Lommel (brecht) would know better on that aspect I guess (also because this is C++ code).

intern/mantaflow/CMakeLists.txt
58–60	Does it even make any sense to do anything currently is `WITH_PYTHON` is FALSE?
intern/mantaflow/extern/manta_fluid_API.h
38–46	(… and whole func definitions actually) Unless am mistaken, this is a public API, right? Then it would be better to have a common prefix, also for the struct name… Something like `MantaFluid` for the struct, and `MANTA_fluid_free()` etc. for the functions?
intern/mantaflow/intern/manta_fluid_API.cpp
37–38	Are all those `extern "C"` declarations really needed? I thought since their declarations in header are enclosed inside an `extern "C" {}` bloc, that was enough?

Updated diff with latest changes from fluid-mantaflow branch

Harbormaster completed remote builds in B4641: Diff 17620.Aug 28 2019, 8:26 PM

You did not address nor answer to my two previous comments re naming of public API funcs and using extern C everywhere...

You're right, there is no real point in having the extern "C" in the source files. It's fixed in the updated diff.

Harbormaster completed remote builds in B5818: Diff 19908.Nov 27 2019, 5:33 PM

Sebastián Barschkis (sebbas) edited the summary of this revision. (Show Details)Dec 5 2019, 11:42 AM

Sebastián Barschkis (sebbas) edited reviewers, added: Bastien Montagne (mont29); removed: Brecht Van Lommel (brecht).

I've probably missed something but is there a documented design about data flow between Blender and Manta?

intern/mantaflow/CMakeLists.txt
31	What if TBB is disabled?
intern/mantaflow/extern/manta_fluid_API.h
41	Missing forward declarations? From the name `MantaModifierData` it seems it's a DNA structure. Using it in `intern/` is, strictly speaking, bad level access.
intern/mantaflow/extern/manta_python_API.h
38	Either missing `include` statement or forward declaration of `PyObject`.
intern/mantaflow/intern/MANTA_main.h
115	All those doesn't really seem necessary. Is a pass-through without any extra checks. What is the actual point here?
422	In Blender we never omit fractional part. The case when `i` is negative or is above the boundary sounds something what should be assert-ed, not quietly ignored.
440	`at` performs bounds check. Since the bounds are checked already use `(*mMeshNodes)[i].normal`.
intern/mantaflow/intern/manta_fluid_API.cpp
44	C++11, `nullptr`, wink wink ;)
49	use early output, and uninedent the actual code.
125–128	Broken indentation.
346–361	Indentation. Review other cases in this file. Maybe even get covered by clang-format.
432	What are those magic constants?
477	Comment is a full sentence, starting with a capital and ending with a fullstop.
intern/mantaflow/intern/strings/fluid_script.h
37	For the simplicity of maintenance/troubleshooting/getting into this area better have those as a .py scripts which are datatoc-ed (similar to GLSL shaders).

This revision now requires changes to proceed.Dec 10 2019, 2:46 PM

Sebastián Barschkis (sebbas) marked 13 inline comments as done.Dec 10 2019, 6:46 PM

Sebastián Barschkis (sebbas) added inline comments.

intern/mantaflow/CMakeLists.txt
31	The TBB setup is much cleaner now: the options for OpenMP are gone in the global CMakeLists there is an additional check for TBB before enabling Mantaflow
intern/mantaflow/extern/manta_fluid_API.h
41	I see, `MantaModifierData` is in fact being used in `intern/mantaflow/intern/`. I can definitely change that, but would preferably do that at a later stage. Can we leave it like this for v1?
intern/mantaflow/intern/MANTA_main.h
115	Those are just being used to access the private member variables.
intern/mantaflow/intern/strings/fluid_script.h
37	I agree, it's fairly easy to mess up the line breaks. I would still keep it this way for now because you really should know what you're doing when touching those files anyways (access to fluid sim) we might get rid of them either way with the nodes setup.

Again, I'm first pushing changes to fluid-mantaflow branch.

82d75a46ce37 has the updates.

After rB82d75a46ce37 and the feedback from your side think it's pretty much ready.

intern/mantaflow/extern/manta_fluid_API.h
41	I can live with this for now, yes.
intern/mantaflow/intern/MANTA_main.h
45	Types should start with a capital. Also, since it's C++ you don't need tyo do verbose C-style typedef. Simple `struct PData { ... };` should work.
115	Point is: don't make members private with a trivial accessors. But here it's read-only, so think it's ok to go your way.
intern/mantaflow/intern/strings/fluid_script.h
37	you really should know what you're doing when touching those files anyways (access to fluid sim) Don't really buy it. Same applies for GLSL. Thing is, having it implemented this way is just adds extra things to worry about for those who knows what they are doing. we might get rid of them either way with the nodes setup. That is a good point. Lets live with this for until there is clarity with nodes.

Sebastián Barschkis (sebbas) marked 4 inline comments as done.Dec 11 2019, 4:16 PM

Sebastián Barschkis (sebbas) added inline comments.

intern/mantaflow/extern/manta_fluid_API.h
41	Okay good, added this to my refactor todo list.
intern/mantaflow/intern/strings/fluid_script.h
37	Don't really buy it. Same applies for GLSL. One day let's code a fluid solver together :)