Page MenuHome

Mantaflow [Part 2]: Added fluid wrapper files
ClosedPublic

Authored by Sebastián Barschkis (sebbas) on Oct 29 2018, 4:47 PM.

Diff Detail

Repository
rB Blender

Event Timeline

Added changes from new outflow object allocation setup.

Fix for smoke outflow (walls and outflow objects)

Update manta files after merge with master.

Cleaned up debug out and did some minor refactoring.

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

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.

Sergey Sharybin (sergey) requested changes to this revision.Dec 10 2019, 2:46 PM

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.
Sebastián Barschkis (sebbas) marked 4 inline comments as done.Dec 10 2019, 6:47 PM

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 :)

Sebastián Barschkis (sebbas) marked 2 inline comments as done.Dec 11 2019, 5:34 PM
Sebastián Barschkis (sebbas) added inline comments.
intern/mantaflow/intern/MANTA_main.h
45

Ok, just changed it too. See d9328f69b827

This revision is now accepted and ready to land.Dec 12 2019, 9:34 AM

Updated diff with latest changes from fluid-mantaflow branch

Updated diff with latest changes from fluid-mantaflow branch