blender/blender

Fix #119211: Masks do not update in GPU compositor #123085

Merged

Omar Emara merged 3 commits from OmarEmaraDev/blender:fix-119211 into blender-v4.2-release

2024-06-13 07:43:23 +02:00

Omar Emara commented

2024-06-11 17:07:21 +02:00

Member

Masks are not updated when edited when using the GPU compositor. That's
because the GPU compositor caches static resources and invalidates them
according to the recalculate flags that the depsgraph flushes to IDs.
The issue is that the flags are not flushed to the evaluated IDs of the
compositor depsgraph, but rather to some other evaluated versions of the
IDs.

To fix this, we make the compositor depsgraph persistent and store it in
the scene runtime. This allows us to reliably track changes to resources
used by the compositor and also reduces the overhead of depsgraph
creation in the compositor job.

Patch originally provided by Sergey.

Fixes #121188.

Masks are not updated when edited when using the GPU compositor. That's because the GPU compositor caches static resources and invalidates them according to the recalculate flags that the depsgraph flushes to IDs. The issue is that the flags are not flushed to the evaluated IDs of the compositor depsgraph, but rather to some other evaluated versions of the IDs. To fix this, we make the compositor depsgraph persistent and store it in the scene runtime. This allows us to reliably track changes to resources used by the compositor and also reduces the overhead of depsgraph creation in the compositor job. Patch originally provided by Sergey. Fixes #121188.

Omar Emara added the

labels 2024-06-11 17:07:21 +02:00

Omar Emara added 1 commit 2024-06-11 17:07:32 +02:00

Fix #119211 : Masks do not update in GPU compositor 0ff0ce49ba

Masks are not updated when edited when using the GPU compositor. That's
because the GPU compositor caches static resources and invalidates them
according to the reclaculate flags that the depsgraph flushes to IDs.
The issue is that the flags are not flushed to the evaluated IDs of the
compositor depsgraph, but rather to some other evaluated versions of the
IDs.

To fix this, we also flush the recalculate flags to the draw data of
original IDs, and get them from original IDs in the compositor side. The
downside is that the draw data will not get freed until the ID is
deleted or the file is closed. But that is realisticly not an issue.

Omar Emara requested review from Sergey Sharybin 2024-06-11 17:08:47 +02:00

Sergey Sharybin requested changes 2024-06-12 10:33:53 +02:00

Dismissed

Sergey Sharybin left a comment

Owner

There are a couple of not-so-obvious drawbacks of this change:

It moves us back to the core parts of the DEG relying more on the original ID. We should be doing the opposite: going away from it as much as possible, to help us having situation when manipulation of DEGs is safe from perspective of possibly accessing data which was freed by UI manipulations.
It makes it so render pipeline can access original data when doing F12 render, outside of the lock where it is actually allowed. This is another thing we are trying to get away from, to increase separation between "viewport" and "render" data.

I think fundamentally this case should behave closer to how the "Persistent Data" work for Cycles, where we cache dependency graph used for the render pipeline. Rendering then re-uses this dependency graph. I've tried to implement similar idea for the compositor, and attached the current patch I have.

The downside is that it might increase memory usage in the cases like the ones described in the #121188, so that is not super great. But, on another hand:

Such increased memory usage is not different from situation when an artist switches active view layer to something more heavy, goes back to the "lite" view layer. DEG is never freed in this case.
People really like the Persistent Data, and the usage of compositor DEG is unlikely to go beyond of what the DEG for the persistent data uses.
We do need to allow weak linking for ID properties anyway, so there will be tool for add-on developers to avoid "heavy" IDs being pulled into the DEG.
We can also work on a mechanism which will somehow ignore ID properties which are not needed by the drivers, keeping memory usage low.
The peak memory usage is not really changed that much, as during comp all that heavy scene is allocated anyway.
It probably will solve the lag before comp starts in the #121188.
It allows to keep the viewport and final render logic of the compositor pretty much aligned.

A bit lengthy, but I wanted to have a bit fuller overview of cons/pros and possible mitigations.

I didn't test the patch to verify all the cases like adding and removing and modifying IDs, this is something that needs to happen if we go this route. To me going this route seems more preferred, so unless you have some concerns maybe we can explore it further?

P.S. I've noticed compositor's cache system only uses ID name. The name of ID is only guaranteed to be valid within a single .blend file. You can have ID name collision if you link IDs from different files. Not directly related to this PR, but we shouldn't forget about addressing it. Using name+library name should be unique.

There are a couple of not-so-obvious drawbacks of this change: - It moves us back to the core parts of the DEG relying more on the original ID. We should be doing the opposite: going away from it as much as possible, to help us having situation when manipulation of DEGs is safe from perspective of possibly accessing data which was freed by UI manipulations. - It makes it so render pipeline can access original data when doing F12 render, outside of the lock where it is actually allowed. This is another thing we are trying to get away from, to increase separation between "viewport" and "render" data. I think fundamentally this case should behave closer to how the "Persistent Data" work for Cycles, where we cache dependency graph used for the render pipeline. Rendering then re-uses this dependency graph. I've tried to implement similar idea for the compositor, and attached the current patch I have. The downside is that it might increase memory usage in the cases like the ones described in the #121188, so that is not super great. But, on another hand: - Such increased memory usage is not different from situation when an artist switches active view layer to something more heavy, goes back to the "lite" view layer. DEG is never freed in this case. - People really like the Persistent Data, and the usage of compositor DEG is unlikely to go beyond of what the DEG for the persistent data uses. - We do need to allow weak linking for ID properties anyway, so there will be tool for add-on developers to avoid "heavy" IDs being pulled into the DEG. - We can also work on a mechanism which will somehow ignore ID properties which are not needed by the drivers, keeping memory usage low. - The peak memory usage is not really changed that much, as during comp all that heavy scene is allocated anyway. - It probably will solve the lag before comp starts in the #121188. - It allows to keep the viewport and final render logic of the compositor pretty much aligned. A bit lengthy, but I wanted to have a bit fuller overview of cons/pros and possible mitigations. I didn't test the patch to verify all the cases like adding and removing and modifying IDs, this is something that needs to happen if we go this route. To me going this route seems more preferred, so unless you have some concerns maybe we can explore it further? P.S. I've noticed compositor's cache system only uses ID name. The name of ID is only guaranteed to be valid within a single .blend file. You can have ID name collision if you link IDs from different files. Not directly related to this PR, but we shouldn't forget about addressing it. Using name+library name should be unique.

persistent_deg.patch

2.6 KiB

Omar Emara commented

2024-06-12 11:03:48 +02:00

Author

Member

@Sergey Thanks for the explanation. I will look into your patch and let you know what I find.

I added a To Do for the cached resources issue, along with another issue I discovered yesterday. #123120

@Sergey Thanks for the explanation. I will look into your patch and let you know what I find. I added a To Do for the cached resources issue, along with another issue I discovered yesterday. #123120

Omar Emara added 2 commits 2024-06-12 13:37:46 +02:00

Revert "Fix #119211 : Masks do not update in GPU compositor" 6538909414

This reverts commit 0ff0ce49ba.

Use patch from Sergey

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

Omar Emara changed title from ~~Fix #119211: Masks do not update in GPU compositor~~ to WIP: Fix #119211: Masks do not update in GPU compositor

2024-06-12 13:41:00 +02:00

Omar Emara changed target branch from main to blender-v4.2-release

2024-06-12 14:20:31 +02:00

Omar Emara changed title from ~~WIP: Fix #119211: Masks do not update in GPU compositor~~ to Fix #119211: Masks do not update in GPU compositor

2024-06-12 14:20:50 +02:00

Omar Emara requested review from Sergey Sharybin 2024-06-12 14:22:09 +02:00

Omar Emara commented

2024-06-12 14:22:47 +02:00

Author

Member

@Sergey I tested your patch with some cached resources and everything seems to be working.

@Sergey I tested your patch with some cached resources and everything seems to be working.

Sergey Sharybin commented

2024-06-12 16:04:29 +02:00

Owner

@blender-bot build

@blender-bot build

Sergey Sharybin approved these changes 2024-06-12 16:46:55 +02:00

Sergey Sharybin left a comment

Owner

Thanks for testing and finishing the patch!

Thanks for testing and finishing the patch!

Omar Emara merged commit 95eb3e13bf into blender-v4.2-release

2024-06-13 07:43:23 +02:00

Omar Emara referenced this issue from a commit

2024-06-13 07:43:24 +02:00

Fix #119211: Masks do not update in GPU compositor

Omar Emara deleted branch fix-119211

2024-06-13 07:43:34 +02:00

Omar Emara referenced this pull request

2024-06-13 07:46:37 +02:00

WIP: Fix #119211: GPU Compositor: Masks are not updating correctly in Backdrop/Viewer Node #119561

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Viewport & EEVEE

Virtual Reality

Asset Browser Project

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Viewport & EEVEE

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#123085

No description provided.