Decide and Implement which level of code is responsible for Depsgraph Tagging #116601

New Issue

Open

opened 2023-12-28 13:08:28 +01:00 by Bastien Montagne · 6 comments

Bastien Montagne commented

2023-12-28 13:08:28 +01:00

Owner

Current code is very inconsistent regarding which level of code is responsible to tag an ID for depsgraph updates when it gets modified.

A simple example: BKE_collection_object_add will tag the affected parent collection, while BKE_collection_add will not.

So depending on which BKE function some higher-level of code uses (like RNA, Editor, I/O, etc.), it has to guess or search whether it should handle depsgraph tagging or not.

Proposal:

In general, do not handle depsgraph tagging in BKE API code. When some BKE code does, it should be clearly documented, and in some cases it may allow disabling it as an option.

Rational:

Consistency.
BKE code should not assume that it is operating on data from the G_MAIN database. In fact, it is likely to be less and less true in the future, as we work toward better support for off-main and temp-main data handling.

Exceptions:

Some BKE code performs complex operations involving many IDs and their relationships (like liboverride, or ID remapping). In that case it only makes sense to handle the tagging in the same code.
Options (like ID_REMAP_SKIP_UPDATE_TAGGING) can be used then to optionally disable that tagging.

Current code is _very_ inconsistent regarding which level of code is responsible to tag an ID for depsgraph updates when it gets modified. A simple example: `BKE_collection_object_add` will tag the affected parent collection, while `BKE_collection_add` will not. So depending on which BKE function some higher-level of code uses (like RNA, Editor, I/O, etc.), it has to guess or search whether it should handle depsgraph tagging or not. ### Proposal: In general, do not handle depsgraph tagging in BKE API code. When some BKE code does, it should be clearly documented, and in some cases it may allow disabling it as an option. Rational: * Consistency. * BKE code should not assume that it is operating on data from the `G_MAIN` database. In fact, it is likely to be less and less true in the future, as we work toward better support for off-main and temp-main data handling. Exceptions: * Some BKE code performs complex operations involving many IDs and their relationships (like liboverride, or ID remapping). In that case it only makes sense to handle the tagging in the same code. * Options (like `ID_REMAP_SKIP_UPDATE_TAGGING`) can be used then to optionally disable that tagging.

Bastien Montagne added the

Dependency Graph

labels 2023-12-28 13:08:42 +01:00

Bastien Montagne commented

2023-12-28 13:16:11 +01:00

Author

Owner

@brecht @Sergey @JacquesLucke thoughts?

@brecht @Sergey @JacquesLucke thoughts?

Brecht Van Lommel commented

2023-12-28 14:05:54 +01:00

Owner

The issue with moving it out of BKE API functions is that the tagging can be conditional on exactly what the function did, or may involve tagging other datablocks. And even in simpler cases, it feels weak to have a hidden dependency between what the BKE function does exactly and the appropriate corresponding depsgraph tag to use outside of it.

I would prefer to have the code that modifies the struct member and the corresponding tagging to be as close to each other as possible. For efficiency that is not always possible, but to me it seems those cases should be the exception where we have to be careful, and in others we keep them together.

However calling the depsgraph tag function is inefficient and so doing it in lower level API functions is not great now.

What I would like to move to eventually is to replace depsgraph tagging with setting a flag on datablocks as they are modified. This would then be flushed to the relevant depsgraphs later, and would also be used to make for example undo writing more efficient by skipping unmodified datablocks.

The issue with moving it out of BKE API functions is that the tagging can be conditional on exactly what the function did, or may involve tagging other datablocks. And even in simpler cases, it feels weak to have a hidden dependency between what the BKE function does exactly and the appropriate corresponding depsgraph tag to use outside of it. I would prefer to have the code that modifies the struct member and the corresponding tagging to be as close to each other as possible. For efficiency that is not always possible, but to me it seems those cases should be the exception where we have to be careful, and in others we keep them together. However calling the depsgraph tag function is inefficient and so doing it in lower level API functions is not great now. What I would like to move to eventually is to replace depsgraph tagging with setting a flag on datablocks as they are modified. This would then be flushed to the relevant depsgraphs later, and would also be used to make for example undo writing more efficient by skipping unmodified datablocks.

Bastien Montagne commented

2023-12-29 16:36:50 +01:00

Author

Owner

The issue with moving it out of BKE API functions is that the tagging can be conditional on exactly what the function did, or may involve tagging other datablocks. And even in simpler cases, it feels weak to have a hidden dependency between what the BKE function does exactly and the appropriate corresponding depsgraph tag to use outside of it.

I would prefer to have the code that modifies the struct member and the corresponding tagging to be as close to each other as possible. For efficiency that is not always possible, but to me it seems those cases should be the exception where we have to be careful, and in others we keep them together.

However calling the depsgraph tag function is inefficient and so doing it in lower level API functions is not great now.

Fair enough, I can understand how that would be good too. But then, what about operations on IDs that do not belong to G_MAIN? Currently, depsgraph tagging essentially assumes that it's operating on G_MAIN data (in 99% of cases). Do we accept this fuzzyness/extra 'useless' processing when IDs do not belong to main data?

What I would like to move to eventually is to replace depsgraph tagging with setting a flag on datablocks as they are modified. This would then be flushed to the relevant depsgraphs later, ...

That indeed sounds like a nice idea. Where would you do the flushing then? Before (or even as part of) depsgraph update calls? And before global undo pushes?

...and would also be used to make for example undo writing more efficient by skipping unmodified datablocks.

In theory, we could already skip writing of unmodified IDs, since deg tags are written on IDs too, and are used for the partial depsgraph update when loading an undo step. Would not consider this seriously currently though, since am fairly certain that we are still missing a lot of correct tagging when modifying data... This gets relatively unnoticed because updates happen from other sources most of the time. We'd need some sort of debug assert that all edits get properly tagged, but not sure how to do that currently. And some ID types are ignored by the depsgraph currently (like Text...).

> The issue with moving it out of BKE API functions is that the tagging can be conditional on exactly what the function did, or may involve tagging other datablocks. And even in simpler cases, it feels weak to have a hidden dependency between what the BKE function does exactly and the appropriate corresponding depsgraph tag to use outside of it. > > I would prefer to have the code that modifies the struct member and the corresponding tagging to be as close to each other as possible. For efficiency that is not always possible, but to me it seems those cases should be the exception where we have to be careful, and in others we keep them together. > > However calling the depsgraph tag function is inefficient and so doing it in lower level API functions is not great now. Fair enough, I can understand how that would be good too. But then, what about operations on IDs that do not belong to `G_MAIN`? Currently, depsgraph tagging essentially assumes that it's operating on `G_MAIN` data (in 99% of cases). Do we accept this fuzzyness/extra 'useless' processing when IDs do not belong to main data? > What I would like to move to eventually is to replace depsgraph tagging with setting a flag on datablocks as they are modified. This would then be flushed to the relevant depsgraphs later, ... That indeed sounds like a nice idea. Where would you do the flushing then? Before (or even as part of) depsgraph update calls? And before global undo pushes? > ...and would also be used to make for example undo writing more efficient by skipping unmodified datablocks. In theory, we could already skip writing of unmodified IDs, since deg tags are written on IDs too, and are used for the partial depsgraph update when loading an undo step. Would not consider this seriously currently though, since am fairly certain that we are still missing _a lot_ of correct tagging when modifying data... This gets relatively unnoticed because updates happen from other sources most of the time. We'd need some sort of debug assert that all edits get properly tagged, but not sure how to do that currently. And some ID types are ignored by the depsgraph currently (like Text...).

Brecht Van Lommel commented

2023-12-30 11:38:40 +01:00

Owner

Fair enough, I can understand how that would be good too. But then, what about operations on IDs that do not belong to G_MAIN? Currently, depsgraph tagging essentially assumes that it's operating on G_MAIN data (in 99% of cases). Do we accept this fuzzyness/extra 'useless' processing when IDs do not belong to main data?

Yes, I think we would. Maybe there will even be a use in some cases, though not immediately sure.

That indeed sounds like a nice idea. Where would you do the flushing then? Before (or even as part of) depsgraph update calls? And before global undo pushes?

It would be right before depsgraph updates. There are some details to be worked out in terms of how you can use the same tag for multiple purposes, maybe it needs separate flags. Also how to do it efficiently without having to loop over all datablocks if nothing changed.

In theory, we could already skip writing of unmodified IDs, since deg tags are written on IDs too, and are used for the partial depsgraph update when loading an undo step. Would not consider this seriously currently though, since am fairly certain that we are still missing a lot of correct tagging when modifying data... This gets relatively unnoticed because updates happen from other sources most of the time. We'd need some sort of debug assert that all edits get properly tagged, but not sure how to do that currently. And some ID types are ignored by the depsgraph currently (like Text...).

Yes, it's a risky change and would need extensive testing with asserts like that.

There are UI properties on datablocks that don't get a depsgraph tag on changes, and datablocks like images that don't use copy on write and so also do not get tagged always. So the meaning needs to be changed a bit from DEG_id_tag_update to ID_tag_modified.

> Fair enough, I can understand how that would be good too. But then, what about operations on IDs that do not belong to `G_MAIN`? Currently, depsgraph tagging essentially assumes that it's operating on `G_MAIN` data (in 99% of cases). Do we accept this fuzzyness/extra 'useless' processing when IDs do not belong to main data? Yes, I think we would. Maybe there will even be a use in some cases, though not immediately sure. > That indeed sounds like a nice idea. Where would you do the flushing then? Before (or even as part of) depsgraph update calls? And before global undo pushes? It would be right before depsgraph updates. There are some details to be worked out in terms of how you can use the same tag for multiple purposes, maybe it needs separate flags. Also how to do it efficiently without having to loop over all datablocks if nothing changed. > In theory, we could already skip writing of unmodified IDs, since deg tags are written on IDs too, and are used for the partial depsgraph update when loading an undo step. Would not consider this seriously currently though, since am fairly certain that we are still missing _a lot_ of correct tagging when modifying data... This gets relatively unnoticed because updates happen from other sources most of the time. We'd need some sort of debug assert that all edits get properly tagged, but not sure how to do that currently. And some ID types are ignored by the depsgraph currently (like Text...). Yes, it's a risky change and would need extensive testing with asserts like that. There are UI properties on datablocks that don't get a depsgraph tag on changes, and datablocks like images that don't use copy on write and so also do not get tagged always. So the meaning needs to be changed a bit from `DEG_id_tag_update` to `ID_tag_modified`.

Bastien Montagne commented

2024-01-02 12:11:05 +01:00

Author

Owner

OK, so for the time being I will start fixing cases like BKE_collection_add missing proper tagging when I find them.

The whole tagging refactor will need its own design task anyway.

OK, so for the time being I will start fixing cases like `BKE_collection_add` missing proper tagging when I find them. The whole tagging refactor will need its own design task anyway.

Sergey Sharybin commented

2024-01-08 18:25:40 +01:00

Owner

We could definitely look into making tagging a cheaper operation. However, it is not THAT expensive, and a lot of low-level code already does tagging at a low level.

Doing tag at the place where thing is modified makes overall system so much more reliable.

P.S. It is something we've talked with Bastien IRL, but thought it worth stating here explicitly.

We could definitely look into making tagging a cheaper operation. However, it is not THAT expensive, and a lot of low-level code already does tagging at a low level. Doing tag at the place where thing is modified makes overall system so much more reliable. P.S. It is something we've talked with Bastien IRL, but thought it worth stating here explicitly.

Bastien Montagne referenced this issue

2024-01-10 16:10:39 +01:00

Fix missing tagging of collections in some BKE_collections codepath. #116986

Bastien Montagne referenced this issue from a commit

2024-01-12 16:24:29 +01:00

Fix missing tagging of collections in some BKE_collections codepath.

Sign in to join this conversation.

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 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#116601

No description provided.