Fix #106425: Mantaflow guiding with domains is broken #117067

Philipp Oeser · 2024-01-12T15:09:58+01:00

Philipp Oeser commented

2024-01-12 15:09:58 +01:00

Caused by e968b4197b / 67e23b4b29

Since culprit commits, we are not running MANTA::initGuiding /
fluid_alloc_guiding if guiding is meant to be pulled from other
domains (it does work with effectors though). This is because atm. only
the FLUID_DOMAIN_ACTIVE_GUIDE flag sets mUsingGuiding to true
(activated in update_obstacleflags, so only for effectors).

So to fix this, we have to ensure that MANTA::initGuiding runs (also
if guiding is pulled from domains), but also make sure we dont run into
the crashes from T102257.

It seems that the real reason we were getting crashes in T102257 is that
67e23b4b29 made it so that resetting the cache (including a call to
fluid_modifier_reset_ex) in fluid_modifier_processDomain happens
after FluidDomainSettings.active_fields have been updated (this
happens in update_flowsflags & update_obstacleflags).
But fluid_modifier_reset_ex also resets
FluidDomainSettings.active_fields. If we then update pointers later in
fluid_modifier_processDomain, we never get anything in the guiding
related pointers for the new mCurrentID (specifically mPhiGuideIn
will be a nullptr). This is the real reason initGuiding() runs a
second time (it does once for fluid_modifier_init then again as a
consequence of the call to manta_guiding).

This patch proposes to move the resetting process from 67e23b4b29
above the refreshing of the FluidDomainSettings.active_fields,
this way these field stay intact, we do get the first run of
initGuiding() from fluid_modifier_init, manta pointers get updated
with intact fields afterwards (so we then get a valid mPhiGuideIn),
which then prevents the second run from manta_guiding to actually call
the python script again.

The fix from e968b4197b is then not needed and reverted in this patch.

This should be good for LTS I think.

Caused by e968b4197b / 67e23b4b29 Since culprit commits, we are not running `MANTA::initGuiding` / `fluid_alloc_guiding` if guiding is meant to be pulled from other domains (it does work with effectors though). This is because atm. only the `FLUID_DOMAIN_ACTIVE_GUIDE` flag sets `mUsingGuiding` to true (activated in `update_obstacleflags`, so only for effectors). So to fix this, we have to ensure that `MANTA::initGuiding` runs (also if guiding is pulled from domains), but also make sure we dont run into the crashes from T102257. It seems that the real reason we were getting crashes in T102257 is that 67e23b4b29 made it so that resetting the cache (including a call to `fluid_modifier_reset_ex`) in `fluid_modifier_processDomain` happens **after** `FluidDomainSettings.active_fields` have been updated (this happens in `update_flowsflags` & `update_obstacleflags`). But `fluid_modifier_reset_ex` also resets `FluidDomainSettings.active_fields`. If we then update pointers later in `fluid_modifier_processDomain`, we never get anything in the guiding related pointers for the new `mCurrentID` (specifically `mPhiGuideIn` will be a nullptr). This is the real reason `initGuiding()` runs a second time (it does once for `fluid_modifier_init` then again as a consequence of the call to `manta_guiding`). This patch proposes to move the resetting process from 67e23b4b29 **above** the refreshing of the `FluidDomainSettings.active_fields`, this way these field stay intact, we do get the first run of `initGuiding()` from `fluid_modifier_init`, manta pointers get updated with intact fields afterwards (so we then get a valid `mPhiGuideIn`), which then prevents the second run from `manta_guiding` to actually call the python script again. The fix from e968b4197b is then not needed and reverted in this patch. This should be good for LTS I think.

Philipp Oeser added 1 commit 2024-01-12 15:10:11 +01:00

fd090aee59 Fix #106425 : Mantaflow guiding with domains is broken

Caused by e968b4197b / 67e23b4b29

Since culprit commits, we are not running `MANTA::initGuiding` /
`fluid_alloc_guiding` if guiding is meant to be pulled from other
domains (it does work with effectors though). This is because atm. only
the `FLUID_DOMAIN_ACTIVE_GUIDE` flag sets `mUsingGuiding` to true
(activated in `update_obstacleflags`, so only for effectors).

So to fix this, we have to ensure that `MANTA::initGuiding` runs (also
if guiding is pulled from domains), but also make sure we dont run into
the crashes from T102257.

It seems that the real reason we were getting crashes in T102257 is that
67e23b4b29 made it so that resetting the cache (including a call to
`fluid_modifier_reset_ex`) in `fluid_modifier_processDomain` happens
**after** `FluidDomainSettings.active_fields` have been updated (this
happens in `update_flowsflags` & `update_obstacleflags`).
But `fluid_modifier_reset_ex` also resets
`FluidDomainSettings.active_fields`. If we then update pointers later in
`fluid_modifier_processDomain`, we never get anything in the guiding
related pointers for the new `mCurrentID` (specifically `mPhiGuideIn`
will be a nullptr). This is the real reason `initGuiding()` runs a
second time (it does once for `fluid_modifier_init` then again as a
consequence of the call to `manta_guiding`).

This patch proposes to move the resetting process from 67e23b4b29
**above** the refreshing of the `FluidDomainSettings.active_fields`,
this way these field stay intact, we do get the first run of
`initGuiding()` from `fluid_modifier_init`, manta pointers get updated
with intact fields afterwards (so we then get a valid `mPhiGuideIn`),
which then prevents the second run from `manta_guiding` to actually call
the python script again.

The fix from e968b4197b is then not needed and reverted in this patch.

This should be good for LTS I think.

Philipp Oeser requested review from Germano Cavalcante 2024-01-12 15:10:46 +01:00

Philipp Oeser requested review from Sebastian Parborg 2024-01-12 15:10:54 +01:00

Philipp Oeser requested review from Sergey Sharybin 2024-01-12 15:11:01 +01:00

Philipp Oeser commented

2024-01-12 15:14:48 +01:00

Wouldnt have spent time on this if this was in @sebbas hands, but since I noticed this was caused/reviewed by still active devs, I gave it a shot [and it is basically a pretty bad regression I think].

@Sergey @ZedDB , you reviewed https://archive.blender.org/developer/D15210, https://archive.blender.org/developer/D16416 apparently did not have review at all

Wouldnt have spent time on this if this was in @sebbas hands, but since I noticed this was caused/reviewed by still active devs, I gave it a shot [and it is basically a pretty bad regression I think]. @Sergey @ZedDB , you reviewed https://archive.blender.org/developer/D15210, https://archive.blender.org/developer/D16416 apparently did not have review at all

Philipp Oeser referenced this pull request

2024-01-12 15:16:28 +01:00

Regression: Guiding in manta is broken #106425

Germano Cavalcante commented

2024-01-12 18:46:02 +01:00

I need to re-study how this Guides thing works.

If I understand correctly, it seems like there are two types of Guides.

"Guide" type effectors;
A Domain can be used to "Guide" another Domain.

Apparently, e968b4197b broke the second type of "Guide", because even with fds->flags having the flag FLUID_DOMAIN_USE_GUIDE, the script that calls MANTA::initGuiding or fluid_alloc_guiding was not being generated.

This caused the python error presented in #106425 and the consequent failure of the Guide.

So the first solution then is to revert e968b4197b to check the FLUID_DOMAIN_USE_GUIDE flag again.

But this brought back the #102257 crash, caused by "Guide" type Effectors.
The real problem in #102257 was that the functions update_flowsflags, update_obstacleflags, fluid_modifier_reset_ex and fluid_modifier_init were being called in the wrong order.
fluid_modifier_reset_ex needs to be called before update_flowsflags, update_obstacleflags and fluid_modifier_init as FluidDomainSettings.active_fields is reset in fluid_modifier_reset_ex.
So the correct order is fluid_modifier_reset_ex, update_flowsflags, update_obstacleflags and fluid_modifier_init, right?
This prevent initGuiding() from being called twice in #102257 because manta_guiding (called in fluid_modifier_init) first checks whether FluidDomainSettings.active_fields are valid?

If that's the case, the solution seems ok.

One last question that might be worth considering (I don't remember the details) but why is the FLUID_DOMAIN_ACTIVE_GUIDE flag no longer checked?

I need to re-study how this Guides thing works. If I understand correctly, it seems like there are two types of Guides. - "Guide" type effectors; - A Domain can be used to "Guide" another Domain. Apparently, e968b4197b broke the second type of "Guide", because even with `fds->flags` having the flag `FLUID_DOMAIN_USE_GUIDE`, the script that calls `MANTA::initGuiding` or `fluid_alloc_guiding` was not being generated. This caused the python error presented in #106425 and the consequent failure of the Guide. So the first solution then is to revert e968b4197b to check the `FLUID_DOMAIN_USE_GUIDE` flag again. But this brought back the #102257 crash, caused by "Guide" type Effectors. The real problem in #102257 was that the functions `update_flowsflags`, `update_obstacleflags`, `fluid_modifier_reset_ex` and `fluid_modifier_init` were being called in the wrong order. `fluid_modifier_reset_ex` needs to be called before `update_flowsflags`, `update_obstacleflags` and `fluid_modifier_init` as `FluidDomainSettings.active_fields` is reset in `fluid_modifier_reset_ex`. So the correct order is `fluid_modifier_reset_ex`, `update_flowsflags`, `update_obstacleflags` and `fluid_modifier_init`, right? This prevent `initGuiding()` from being called twice in #102257 because `manta_guiding` (called in `fluid_modifier_init`) first checks whether `FluidDomainSettings.active_fields` are valid? If that's the case, the solution seems ok. One last question that might be worth considering (I don't remember the details) but why is the `FLUID_DOMAIN_ACTIVE_GUIDE` flag no longer checked?

Philipp Oeser commented

2024-01-12 19:45:16 +01:00

Typing this on the mobile, but:

So the correct order is fluid_modifier_reset_ex, update_flowsflags, update_obstacleflags and fluid_modifier_init, right?

This is my understanding, yes

This prevent initGuiding() from being called twice in #102257 because manta_guiding (called in fluid_modifier_init) first checks whether FluidDomainSettings.active_fields are valid?

from the PR description:

this way these field stay intact, we do get the first run of
initGuiding() from fluid_modifier_init, manta pointers get updated
with intact fields afterwards (so we then get a valid mPhiGuideIn),
which then prevents the second run from manta_guiding to actually call
the python script again.

So it is the fact we first dont have a valid mPhiGuideIn in the first run, on the second run we have.

One last question that might be worth considering (I don't remember the details) but why is the FLUID_DOMAIN_ACTIVE_GUIDE flag no longer checked?

Because that was only the workaround from e968b4197b which we dont actually need / can revert?

Typing this on the mobile, but: > So the correct order is `fluid_modifier_reset_ex`, `update_flowsflags`, `update_obstacleflags` and `fluid_modifier_init`, right? This is my understanding, yes > This prevent `initGuiding()` from being called twice in #102257 because `manta_guiding` (called in `fluid_modifier_init`) first checks whether `FluidDomainSettings.active_fields` are valid? from the PR description: >this way these field stay intact, we do get the first run of initGuiding() from `fluid_modifier_init`, manta pointers get updated with intact fields afterwards (so we then get a valid `mPhiGuideIn`), which then prevents the second run from `manta_guiding` to actually call the python script again. So it is the fact we first dont have a valid `mPhiGuideIn` in the first run, on the second run we have. > One last question that might be worth considering (I don't remember the details) but why is the `FLUID_DOMAIN_ACTIVE_GUIDE` flag no longer checked? Because that was only the workaround from e968b4197b which we dont actually need / can revert?

Germano Cavalcante approved these changes 2024-01-12 21:13:02 +01:00

Germano Cavalcante left a comment

My last question was unclear, it was more of a code question to better understand what is happening (since I don't remember the details). In other words the question is: Why is FLUID_DOMAIN_ACTIVE_GUIDE not necessary when we use FLUID_DOMAIN_USE_GUIDE? However, it doesn't seem like something important to be investigated.

There are other details in the code that leave me lost like: What is this mPhiGuideIn? How is it invalidated?

Either way, it's certainly more correct now :) (Nice work)

(Maybe it's worth adding a comment or an assert just to avoid this error? It appears to be fragile code).
(From my side it is approved).

My last question was unclear, it was more of a code question to better understand what is happening (since I don't remember the details). In other words the question is: Why is `FLUID_DOMAIN_ACTIVE_GUIDE` not necessary when we use `FLUID_DOMAIN_USE_GUIDE`? However, it doesn't seem like something important to be investigated. There are other details in the code that leave me lost like: What is this `mPhiGuideIn`? How is it invalidated? Either way, it's certainly more correct now :) (Nice work) (Maybe it's worth adding a comment or an assert just to avoid this error? It appears to be fragile code). (From my side it is approved).

Gary Parris commented

2024-01-13 06:16:51 +01:00

First-time contributor

Sounds promising, is it likely to be in Blender v4.0 and/or v4.1? or just v3.6 LTS!?

Sergey Sharybin approved these changes 2024-01-15 09:36:01 +01:00

Philipp Oeser commented

2024-01-15 11:43:57 +01:00

Sounds promising, is it likely to be in Blender v4.0 and/or v4.1? or just v3.6 LTS!?

Should be in 4.1 and LTS, 4.0 is not getting another update

> Sounds promising, is it likely to be in Blender v4.0 and/or v4.1? or just v3.6 LTS!? Should be in 4.1 and LTS, 4.0 is not getting another update

Philipp Oeser commented

2024-01-15 12:12:59 +01:00

Why is FLUID_DOMAIN_ACTIVE_GUIDE not necessary when we use FLUID_DOMAIN_USE_GUIDE?

FLUID_DOMAIN_ACTIVE_GUIDE is only activated in update_obstacleflags (so only for effectors).
If we always check that, guiding by domains will be ignored.

There are other details in the code that leave me lost like: What is this mPhiGuideIn? How is it invalidated?

That this is a grid of guiding velocities (fluid guiding is basically builtin functionality of mantaflow itself, dont really have a deeper understanding of how this is used in mantaflows solvers, probably not worth diving deeper at this point in time -- at least for me)
gets invalidated in MANTA::updatePointers

> Why is `FLUID_DOMAIN_ACTIVE_GUIDE` not necessary when we use `FLUID_DOMAIN_USE_GUIDE`? `FLUID_DOMAIN_ACTIVE_GUIDE` is only activated in `update_obstacleflags` (so only for effectors). If we always check that, guiding by domains will be ignored. > There are other details in the code that leave me lost like: What is this `mPhiGuideIn`? How is it invalidated? - That this is a grid of guiding velocities (fluid guiding is basically builtin functionality of mantaflow itself, dont really have a deeper understanding of how this is used in mantaflows solvers, probably not worth diving deeper at this point in time -- at least for me) - gets invalidated in `MANTA::updatePointers`

Sebastian Parborg refused to review 2024-01-15 12:15:21 +01:00

Philipp Oeser merged commit 4da32e9334 into main

2024-01-15 12:31:53 +01:00

Philipp Oeser deleted branch 106425_b

2024-01-15 12:31:55 +01:00

Philipp Oeser referenced this issue from a commit

2024-01-15 12:31:57 +01:00

Fix #106425: Mantaflow guiding with domains is broken

Gary Parris commented

2024-01-15 17:29:28 +01:00

First-time contributor

Sounds promising, is it likely to be in Blender v4.0 and/or v4.1? or just v3.6 LTS!?

Should be in 4.1 and LTS, 4.0 is not getting another update

be glad to try it out over the coming days - Thanks to Philipp and everyone involved!

> > Sounds promising, is it likely to be in Blender v4.0 and/or v4.1? or just v3.6 LTS!? > > Should be in 4.1 and LTS, 4.0 is not getting another update be glad to try it out over the coming days - Thanks to Philipp and everyone involved!

Philipp Oeser commented

2024-01-16 09:15:05 +01:00

FYI: this wont be in todays LTS updates (so only in 4.1 for now), should be in next months LTS releases (if no issues come up with it in 4.1)

Kazashi Yoshioka referenced this issue from a commit

2024-01-16 14:55:04 +01:00

Fix #106425: Mantaflow guiding with domains is broken

Gary Parris commented

2024-01-17 05:31:36 +01:00

First-time contributor

i will definitely check it out shortly (4.1) and report anything! thanks all happy new year to you

Gary Parris commented

2024-01-19 04:30:28 +01:00

First-time contributor

I can confirm the Domain guide is working and seems stable!

I have noticed that domain does not show the guided smoke in viewport solid mode when created from the fluid domain, it does show up in rendered mode with principled volume added though.

still more testing to do, but that's the only things i have found so far. Fluid -> Smoke

I can confirm the Domain guide is working and seems stable! I have noticed that domain does not show the guided smoke in viewport solid mode when created from the fluid domain, it does show up in rendered mode with principled volume added though. still more testing to do, but that's the only things i have found so far. Fluid -> Smoke

Philipp Oeser commented

2024-01-19 11:03:22 +01:00

I can confirm the Domain guide is working and seems stable!

Thx for testing!

I have noticed that domain does not show the guided smoke in viewport solid mode when created from the fluid domain, it does show up in rendered mode with principled volume added though.

Cant repro in my simple test, just to make sure we are all on the same page: could you create a report for this (including the .blend that you used)?

> I can confirm the Domain guide is working and seems stable! Thx for testing! > I have noticed that domain does not show the guided smoke in viewport solid mode when created from the fluid domain, it does show up in rendered mode with principled volume added though. Cant repro in my simple test, just to make sure we are all on the same page: could you create a report for this (including the .blend that you used)?

lichen commented

2024-01-19 11:07:33 +01:00

First-time contributor

Hello,
The style of these two pictures displayed in rendering mode

Hello, The style of these two pictures displayed in rendering mode

SMOKE.jpg

661 KiB

LUQUID.jpg

529 KiB

lichen commented

2024-01-19 11:18:05 +01:00

First-time contributor

Hello,
The style of these two pictures displayed in rendering mode

I think it's correct to display it like smoke

> Hello, > The style of these two pictures displayed in rendering mode I think it's correct to display it like smoke

Philipp Oeser commented

2024-01-19 11:20:37 +01:00

@lichen : same for you: if you believe you have found a bug/issue, please open a bugreport for it, feel free to ping me there.

It is not clear if these issues are bugs and/or related to this pull request at all...

@lichen : same for you: if you believe you have found a bug/issue, please open a bugreport for it, feel free to ping me there. It is not clear if these issues are bugs and/or related to this pull request at all...

Arye Ramaty referenced this issue from a commit

2024-01-21 15:37:41 +01:00

Fix #106425: Mantaflow guiding with domains is broken

Jonas Dichelle referenced this issue from a commit

2024-02-08 17:29:02 +01:00

Fix #106425: Mantaflow guiding with domains is broken

Philipp Oeser referenced this issue from a commit

2024-02-14 15:33:26 +01:00

Fix #106425: Mantaflow guiding with domains is broken

Philipp Oeser referenced this issue from a commit

2024-02-14 16:37:32 +01:00

Fix #106425: Mantaflow guiding with domains is broken