blender
/
blender
134
398
Fork 579
Code Issues 6.3k Pull Requests 754 Projects 19 Wiki Activity

DrawData recalc fixes/improvements #114112

New Issue
Closed
opened 2023-10-24 17:04:23 +02:00 by Miguel Pozo · 21 comments
Miguel Pozo commented 2023-10-24 17:04:23 +02:00
Member
Copy Link

Draw Engines don't use the main ID::recalc flag (the flag is actually already cleared by the time rendering happens), they use a copy stored in the DrawData instead. This flag is filled in flush_engine_data_update and it's up to the engines to clear it when the update has been handled.

EEVEE-Next relies on the DrawData::recalc flags to detect the shadow-map sections that need to be updated.
However, there's an issue with the current system that prevents it from working correctly.

Since DrawData is stored per DrawEngineType, and not per engine instance. The first instance that process the flags will clear them, causing the rest of the instances to not update correctly:
Clipboard - 23 de octubre de 2023 16 34

The printed data on the console is Instance address : Material name : Material recall flags.

A partial solution could be iterating all DrawData after all engine instances have been updated and clearing the flags then.
However, not all viewports are always updated, the main (maybe only?) exception being animation playback on a single viewport.

A full solution could be storing DrawData per engine instance instead, but DrawData as it is right now is not that optimal performance-wise and this would make it worse. This would also require iterating all IDs on instance deletion to remove its associated DrawData.

Since the problem is the recalc flag itself (the only part required by Next engines), and not the full DrawData system.
I propose a simpler solution:

  • Add a single recalc flag that can be shared by all engines/instances to the IdDdtTemplate.
  • Have a global tick counter (ie. a counter that increments every WM_main loop).
  • Add a last_updated property to the IdDdtTemplate.
  • When flush_engine_data_updatewrites the recalc, store also the current tick in last_updated (the recalc flag is never cleared).
  • Engine instances also keep track of the last tick they were updated. When instances sync an ID they check their last_update:
    • If instance.last_update >= id.last_update, then recalc is 0.
    • If instance.last_update + 1 == id.last_update, then recalc is IdDdtTemplate.recalc.
    • If instance.last_update + 1 < id.last_update, then recalc is ID_RECALC_ALL since the instance lost track of the recalc updates.
Draw Engines don't use the main `ID::recalc` flag (the flag is actually already cleared by the time rendering happens), they use a copy stored in the `DrawData` instead. This flag is filled in `flush_engine_data_update` and it's up to the engines to clear it when the update has been handled. EEVEE-Next relies on the `DrawData::recalc` flags to detect the shadow-map sections that need to be updated. However, there's an issue with the current system that prevents it from working correctly. Since `DrawData` is stored per `DrawEngineType`, and not per engine instance. The first instance that process the flags will clear them, causing the rest of the instances to not update correctly: ![Clipboard - 23 de octubre de 2023 16 34](/attachments/e2c15c8f-72e2-4c09-82c0-3e4b65fbdfa6) > The printed data on the console is *Instance address : Material name : Material recall flags*. A partial solution could be iterating all `DrawData` after all engine instances have been updated and clearing the flags then. However, not all viewports are always updated, the main (maybe only?) exception being animation playback on a single viewport. A full solution could be storing `DrawData` per engine instance instead, but `DrawData` as it is right now is not that optimal performance-wise and this would make it worse. This would also require iterating all IDs on instance deletion to remove its associated `DrawData`. Since the problem is the `recalc` flag itself (the only part required by `Next` engines), and not the full `DrawData` system. I propose a simpler solution: * Add a single `recalc` flag that can be shared by all engines/instances to the `IdDdtTemplate`. * Have a global `tick` counter (ie. a counter that increments every `WM_main` loop). * Add a `last_updated` property to the `IdDdtTemplate`. * When `flush_engine_data_update`writes the `recalc`, store also the current tick in `last_updated` (the `recalc` flag is never cleared). * Engine instances also keep track of the last `tick` they were updated. When instances sync an `ID` they check their `last_update`: * If `instance.last_update >= id.last_update`, then `recalc` is 0. * If `instance.last_update + 1 == id.last_update`, then `recalc` is `IdDdtTemplate.recalc`. * If `instance.last_update + 1 < id.last_update`, then `recalc` is `ID_RECALC_ALL` since the instance lost track of the recalc updates.
Clipboard - 23 de octubre de 2023 16 34.png
661 KiB
👍 2
Miguel Pozo added this to the 4.1 milestone 2023-10-24 17:04:23 +02:00
Miguel Pozo added the
Interest
Core
Module
EEVEE & Viewport
Type
Design
 labels 2023-10-24 17:04:24 +02:00
Miguel Pozo added this to the EEVEE & Viewport project 2023-10-24 17:04:27 +02:00
Miguel Pozo added the
Interest
EEVEE
 label 2023-11-07 16:10:17 +01:00
Sergey Sharybin commented 2023-11-14 15:21:30 +01:00
Owner
Copy Link

Making the render engine dependent on a tick counter from the main even loop does not sound like a good idea to me. Headless (blender --background) renders do not have main loop running. The F12 renders do have main loop running, but it is on a different thread. And in both cases the system should be allowing to avoid full updates when next frame is rendered.

Even within a case when you have 2 viewports, ideally you should be able to avoid recalculating/updating everything when you full-screen one viewport, do some modification, and exit the full-screen of that viewport.

The proposal does not cover is the what happens on rollover of the counter.

What I am not sure about, is why can't render-instance-specific data be explicitly stored in a per-render-instance-specific storage? Like, assign a session_uid to render engine, and use those to access render-instance-speicfic data (similarly to how the similar mechanism is used with session_uuid on the ID level).

P.S. saying that recalc is ID_RECALC_ALL is not correct. The ID_RECALC_ALL can only be used in a context of id.recalc & ID_RECALC_ALL to see if SOMETHING has changed. Nowadays we can/should get rid of this flag and do id.recalc for that purposes.

Making the render engine dependent on a tick counter from the main even loop does not sound like a good idea to me. Headless (`blender --background`) renders do not have main loop running. The F12 renders do have main loop running, but it is on a different thread. And in both cases the system should be allowing to avoid full updates when next frame is rendered. Even within a case when you have 2 viewports, ideally you should be able to avoid recalculating/updating everything when you full-screen one viewport, do some modification, and exit the full-screen of that viewport. The proposal does not cover is the what happens on rollover of the counter. What I am not sure about, is why can't render-instance-specific data be explicitly stored in a per-render-instance-specific storage? Like, assign a `session_uid` to render engine, and use those to access render-instance-speicfic data (similarly to how the similar mechanism is used with `session_uuid` on the ID level). P.S. saying that `recalc` is `ID_RECALC_ALL` is not correct. The `ID_RECALC_ALL` can only be used in a context of `id.recalc & ID_RECALC_ALL` to see if SOMETHING has changed. Nowadays we can/should get rid of this flag and do `id.recalc` for that purposes.
Miguel Pozo commented 2023-11-14 16:37:42 +01:00
Author
Member
Copy Link

Making the render engine dependent on a tick counter from the main even loop does not sound like a good idea to me. Headless (blender --background) renders do not have main loop running. The F12 renders do have main loop running, but it is on a different thread. And in both cases the system should be allowing to avoid full updates when next frame is rendered.

The main loop idea may be naive thinking on my side, I don't have the full picture of how the whole Depsgraph system works.
Couldn't we just have separate main counter/recalcs and render counter/recalcs?

Even within a case when you have 2 viewports, ideally you should be able to avoid recalculating/updating everything when you full-screen one viewport, do some modification, and exit the full-screen of that viewport.

While it would be great to avoid any redundant recalculations, I would prioritize performance on interaction and playback, rather than one-time context switches.

What I am not sure about, is why can't render-instance-specific data be explicitly stored in a per-render-instance-specific storage? Like, assign a session_uid to render engine, and use those to access render-instance-speicfic data (similarly to how the similar mechanism is used with session_uuid on the ID level).

I think this would be similar to what I mention here:

A full solution could be storing DrawData per engine instance instead, but DrawData as it is right now is not that optimal performance-wise and this would make it worse. This would also require iterating all IDs on instance deletion to remove its associated DrawData.

Another issue is that, ideally, the recalc flags should be cleared after the whole instance is fully updated.

So, IMO, this would result in a much more complex system, that is slower overall (due to hashmaps lookups and crossed dependencies) except for the special case of one-time context switches.

The proposal does not cover is the what happens on rollover of the counter.

Even assuming a simple uint32 counter and constant 240 fps updates, that's 207 days running Blender non-stop without it becoming an issue. If that's actually a problem, we can use a uint64.

P.S. saying that recalc is ID_RECALC_ALL is not correct. The ID_RECALC_ALL can only be used in a context of id.recalc & ID_RECALC_ALL to see if SOMETHING has changed. Nowadays we can/should get rid of this flag and do id.recalc for that purposes.

That's what we use in EEVEE Next:
https://projects.blender.org/blender/blender/src/branch/main/source/blender/draw/engines/eevee_next/eevee_sync.cc#L40
Maybe we should replace it with an EEVEE specific flag that tags only what we care about?

> Making the render engine dependent on a tick counter from the main even loop does not sound like a good idea to me. Headless (blender --background) renders do not have main loop running. The F12 renders do have main loop running, but it is on a different thread. And in both cases the system should be allowing to avoid full updates when next frame is rendered. The main loop idea may be naive thinking on my side, I don't have the full picture of how the whole Depsgraph system works. Couldn't we just have separate main counter/recalcs and render counter/recalcs? > Even within a case when you have 2 viewports, ideally you should be able to avoid recalculating/updating everything when you full-screen one viewport, do some modification, and exit the full-screen of that viewport. While it would be great to avoid any redundant recalculations, I would prioritize performance on interaction and playback, rather than one-time context switches. > What I am not sure about, is why can't render-instance-specific data be explicitly stored in a per-render-instance-specific storage? Like, assign a session_uid to render engine, and use those to access render-instance-speicfic data (similarly to how the similar mechanism is used with session_uuid on the ID level). I think this would be similar to what I mention here: > A full solution could be storing DrawData per engine instance instead, but DrawData as it is right now is not that optimal performance-wise and this would make it worse. This would also require iterating all IDs on instance deletion to remove its associated DrawData. Another issue is that, ideally, the recalc flags should be cleared after the whole instance is fully updated. So, IMO, this would result in a much more complex system, that is slower overall (due to hashmaps lookups and crossed dependencies) except for the special case of one-time context switches. > The proposal does not cover is the what happens on rollover of the counter. Even assuming a simple uint32 counter and constant 240 fps updates, that's 207 days running Blender non-stop without it becoming an issue. If that's actually a problem, we can use a uint64. > P.S. saying that recalc is ID_RECALC_ALL is not correct. The ID_RECALC_ALL can only be used in a context of id.recalc & ID_RECALC_ALL to see if SOMETHING has changed. Nowadays we can/should get rid of this flag and do id.recalc for that purposes. That's what we use in EEVEE Next: https://projects.blender.org/blender/blender/src/branch/main/source/blender/draw/engines/eevee_next/eevee_sync.cc#L40 Maybe we should replace it with an EEVEE specific flag that tags only what we care about?
Brecht Van Lommel commented 2023-11-15 15:37:16 +01:00
Owner
Copy Link

To me it seems reasonable to use ticks for the draw code specifically.

For the depsgraph in general it would be harder because there are many different ID_RECALC flags and you would need a separate tick counter for each, for the updates to be fine grained enough. But for the draw code it seems like a nice simple solution to me. If you need to distinguish between different flags in the future I guess there would not be that many.

The main loop idea may be naive thinking on my side, I don't have the full picture of how the whole Depsgraph system works.
I guess a better option might be to store it in the Depsgraph itself and increment it on deg_graph_flush_updates?
Or just have a main counter and a render counter.

Assuming the draw manager data is all per-depsgraph, storing it on the depsgraph sounds right to me. I don't think having it global is right since there can be multiple depsgraphs alive at the same time, and there are depsgraphs that are completely separate like the one for preview renders.

I guess a global atomic variable that you increment in deg_graph_flush_updates would work too, but why have it be global if it can be local per depsgraph.

To me it seems reasonable to use ticks for the draw code specifically. For the depsgraph in general it would be harder because there are many different `ID_RECALC` flags and you would need a separate tick counter for each, for the updates to be fine grained enough. But for the draw code it seems like a nice simple solution to me. If you need to distinguish between different flags in the future I guess there would not be that many. > The main loop idea may be naive thinking on my side, I don't have the full picture of how the whole Depsgraph system works. > I guess a better option might be to store it in the Depsgraph itself and increment it on `deg_graph_flush_updates`? > Or just have a main counter and a render counter. Assuming the draw manager data is all per-depsgraph, storing it on the depsgraph sounds right to me. I don't think having it global is right since there can be multiple depsgraphs alive at the same time, and there are depsgraphs that are completely separate like the one for preview renders. I guess a global atomic variable that you increment in `deg_graph_flush_updates` would work too, but why have it be global if it can be local per depsgraph.
Miguel Pozo commented 2023-11-15 15:48:35 +01:00
Author
Member
Copy Link

I've actually just edited my answer (probably while you were writing yours, sorry) to remove the part about storing it on the Depsgraph since the counter itself is not enough, it would also need recalc flags per ID and per Depsgraph, and then you have the same problem of needing some kind of hashmap/list per ID.

Maybe that's still ok, though?

In any case, yes, this would only apply to separate recalc flags (only used for drawing), not to the main ones.

I've actually just edited my answer (probably while you were writing yours, sorry) to remove the part about storing it on the Depsgraph since the counter itself is not enough, it would also need recalc flags per ID and per Depsgraph, and then you have the same problem of needing some kind of hashmap/list per ID. Maybe that's still ok, though? In any case, yes, this would only apply to separate recalc flags (only used for drawing), not to the main ones.
Sergey Sharybin commented 2023-11-15 15:51:14 +01:00
Owner
Copy Link

Counter of a dependency graph level is not easier to maintain and check. It will break the assumption that there is only single update between two redraws. That is not the case, even with a single viewport which is always visible: you can have a Python handler which triggers an extra depsgraph evaluation. It also would mean that render engine instances would need to react to change of the active scene and view layer. So overall I do not see such counter be reliable or clear solution.

I do agree that the DrawData is not very well designed currently. There were some pivots from the original ideas made, and as per nowadays, it will be more optimal to replace list wit ha struct of per-engine-type pointers. This should reduce the overall size by avoiding next/prev pointers, and will make access to engine-type-specific data O(1). "Back-pointers" should also be avoided. There is no good reason to store ID and animation data permanently in the draw data.

For finding good solution to the problem described here I'd need to know more details.
For example, I do hope that the actual content of the DrawData is independent from the engine-instance (otherwise two instances will constantly fight for the truthful state of the data). But if that's the case, why does it matter if the DrawData was calculated by another engine instance, or was calculated by the same engine at a previews redraw?
Or, why can't it be handled from editor_update_cb which knows all engines active in the viewport and can decide that re-render is needed because the callback received an object with a modified geometry?

Counter of a dependency graph level is not easier to maintain and check. It will break the assumption that there is only single update between two redraws. That is not the case, even with a single viewport which is always visible: you can have a Python handler which triggers an extra depsgraph evaluation. It also would mean that render engine instances would need to react to change of the active scene and view layer. So overall I do not see such counter be reliable or clear solution. I do agree that the `DrawData` is not very well designed currently. There were some pivots from the original ideas made, and as per nowadays, it will be more optimal to replace list wit ha struct of per-engine-type pointers. This should reduce the overall size by avoiding `next`/`prev` pointers, and will make access to engine-type-specific data `O(1)`. "Back-pointers" should also be avoided. There is no good reason to store ID and animation data permanently in the draw data. For finding good solution to the problem described here I'd need to know more details. For example, I do hope that the actual content of the `DrawData` is independent from the engine-instance (otherwise two instances will constantly fight for the truthful state of the data). But if that's the case, why does it matter if the `DrawData` was calculated by another engine instance, or was calculated by the same engine at a previews redraw? Or, why can't it be handled from `editor_update_cb` which knows all engines active in the viewport and can decide that re-render is needed because the callback received an object with a modified geometry?
Sergey Sharybin commented 2023-11-15 15:59:58 +01:00
Owner
Copy Link

It would also need recalc flags per ID and per Depsgraph, and then you have the same problem of needing some kind of hashmap/list per ID.

If you operate on IDs which come from the depsgraph you naturally get id->recalc be per-depsgraph. The id gets copied to every depsgraph it is part of, and is potentially evaluated to different states.

> It would also need recalc flags per ID and per Depsgraph, and then you have the same problem of needing some kind of hashmap/list per ID. If you operate on IDs which come from the depsgraph you naturally get `id->recalc` be per-depsgraph. The id gets copied to every depsgraph it is part of, and is potentially evaluated to different states.
Miguel Pozo commented 2023-11-15 16:33:48 +01:00
Author
Member
Copy Link

Counter of a dependency graph level is not easier to maintain and check. It will break the assumption that there is only single update between two redraws.

We could increment the counter only once the updates are actually consumed by the Draw module.

It also would mean that render engine instances would need to react to change of the active scene and view layer.

AFAIK, a change of active scene/layer will trigger the delete/creation of engine instances anyway.

For finding good solution to the problem described here I'd need to know more details.
For example, I do hope that the actual content of the DrawData is independent from the engine-instance (otherwise two instances will constantly fight for the truthful state of the data). But if that's the case, why does it matter if the DrawData was calculated by another engine instance, or was calculated by the same engine at a previews redraw?

EEVEE Next doesn't really store anything in DrawData, it only needs the recalc flags.
I actually don't think we need DrawData at all in any of the "new" engines, we only need the recalc flags.

As for context, the recalc data right now is used for:

  • Tracking when the sampling needs to be reset. Maybe that would be better handled by editor_update_cb, as you pointed out.
  • Track Light Probe updates.
  • Track object motion and deform (for motion vectors).
  • Track which virtual shadow map tiles need to be updated (these are view-dependent).

This is all tracked and handled per instance, that's why we need reliable recalc updates for each instance.
There's no shared engine-specific data between instances (except for Volume Probes, but that still doesn't use DrawData).

If you operate on IDs which come from the depsgraph you naturally get id->recalc be per-depsgraph. The id gets copied to every depsgraph it is part of, and is potentially evaluated to different states.

Oh, I wasn't counting on that. That makes things easier.

> Counter of a dependency graph level is not easier to maintain and check. It will break the assumption that there is only single update between two redraws. We could increment the counter only once the updates are actually consumed by the Draw module. > It also would mean that render engine instances would need to react to change of the active scene and view layer. AFAIK, a change of active scene/layer will trigger the delete/creation of engine instances anyway. > For finding good solution to the problem described here I'd need to know more details. For example, I do hope that the actual content of the DrawData is independent from the engine-instance (otherwise two instances will constantly fight for the truthful state of the data). But if that's the case, why does it matter if the DrawData was calculated by another engine instance, or was calculated by the same engine at a previews redraw? EEVEE Next doesn't really store anything in DrawData, it only needs the recalc flags. I actually don't think we need `DrawData` at all in any of the "new" engines, we only need the recalc flags. As for context, the recalc data right now is used for: - Tracking when the sampling needs to be reset. Maybe that would be better handled by `editor_update_cb`, as you pointed out. - Track Light Probe updates. - Track object motion and deform (for motion vectors). - Track which virtual shadow map tiles need to be updated (these are view-dependent). This is all tracked and handled per instance, that's why we need reliable recalc updates for each instance. There's no shared engine-specific data between instances (except for Volume Probes, but that still doesn't use DrawData). > If you operate on IDs which come from the depsgraph you naturally get id->recalc be per-depsgraph. The id gets copied to every depsgraph it is part of, and is potentially evaluated to different states. Oh, I wasn't counting on that. That makes things easier.
Brecht Van Lommel commented 2023-11-15 16:39:50 +01:00
Owner
Copy Link

Counter of a dependency graph level is not easier to maintain and check. It will break the assumption that there is only single update between two redraws. That is not the case, even with a single viewport which is always visible: you can have a Python handler which triggers an extra depsgraph evaluation.

I don't think there is such an assumption? The way I imagine it to work is:

  • The depsgraph increases its counter on every update, also from a Python handler.
  • The draw engine stores the counter value when it was last updated
  • Check if the counter value from the depsgraph is bigger than the one in the draw engine
> Counter of a dependency graph level is not easier to maintain and check. It will break the assumption that there is only single update between two redraws. That is not the case, even with a single viewport which is always visible: you can have a Python handler which triggers an extra depsgraph evaluation. I don't think there is such an assumption? The way I imagine it to work is: * The depsgraph increases its counter on every update, also from a Python handler. * The draw engine stores the counter value when it was last updated * Check if the counter value from the depsgraph is bigger than the one in the draw engine
Miguel Pozo commented 2023-11-15 16:45:55 +01:00
Author
Member
Copy Link

That would be enough to know if the ID has been updated, but not to be sure of what has been updated:

From the main post:

If instance.last_update + 1 == id.last_update, then recalc is IdDdtTemplate.recalc.
If instance.last_update + 1 < id.last_update, then recalc is ID_RECALC_ALL since the instance lost track of the recalc updates.

That would be enough to know if the ID has been updated, but not to be sure of what has been updated: From the main post: > If instance.last_update + 1 == id.last_update, then recalc is IdDdtTemplate.recalc. If instance.last_update + 1 < id.last_update, then recalc is ID_RECALC_ALL since the instance lost track of the recalc updates.
Miguel Pozo commented 2023-11-15 17:08:24 +01:00
Author
Member
Copy Link

Maybe we could just store a different last_update property for each relevant flag (I think we only would need ID_RECALC_TRANSFORM, ID_RECALC_GEOMETRY and ID_RECALC_SHADING). We use ID_RECALC_COPY_ON_WRITE too, but that could simply update all of them.

That would allow it to work the way @brecht suggests.

Maybe we could just store a different `last_update` property for each relevant flag (I think we only would need `ID_RECALC_TRANSFORM`, `ID_RECALC_GEOMETRY` and `ID_RECALC_SHADING`). We use `ID_RECALC_COPY_ON_WRITE` too, but that could simply update all of them. That would allow it to work the way @brecht suggests.
Brecht Van Lommel commented 2023-11-15 17:08:48 +01:00
Owner
Copy Link

Then you need a separate counter per flag right? last_update_transform, last_update_geometry, etc.

Then you need a separate counter per flag right? `last_update_transform`, `last_update_geometry`, etc.
Miguel Pozo commented 2023-11-15 17:11:13 +01:00
Author
Member
Copy Link

I guess we wrote the same thing at the same time. 😅
Yes, I think that would work.

I guess we wrote the same thing at the same time. 😅 Yes, I think that would work.
Sergey Sharybin commented 2023-11-15 18:24:14 +01:00
Owner
Copy Link

Is it something that can be implemented via the ED_render_id_flush_update and ED_render_scene_update? Would be nice if the depsgrapgh did not need to worry about draw-manager specific design.

P.S. The ID_RECALC_COPY_ON_WRITE is another internal-ish flag. In a lot of cases where it is used it should not. So I'd like to know details behind this.

Is it something that can be implemented via the `ED_render_id_flush_update` and `ED_render_scene_update`? Would be nice if the depsgrapgh did not need to worry about draw-manager specific design. P.S. The `ID_RECALC_COPY_ON_WRITE` is another internal-ish flag. In a lot of cases where it is used it should not. So I'd like to know details behind this.
Miguel Pozo commented 2023-11-15 18:46:52 +01:00
Author
Member
Copy Link

Is it something that can be implemented via the ED_render_id_flush_update and ED_render_scene_update? Would be nice if the depsgrapgh did not need to worry about draw-manager specific design.

Looking at it, it doesn't seem like it has any kind of per-object granularity?
If that's the case, then I don't think so.

Ideally, especially for shadow maps, we need to know exactly which objects have updated their transform/geometry/shading (including any kind of instance).

Also, keep in mind that we need some kind of solution for 4.1, the further we depart from the already existing solution, the riskier it gets.

P.S. The ID_RECALC_COPY_ON_WRITE is another internal-ish flag. In a lot of cases where it is used it should not. So I'd like to know details behind this.

We just check for it to reset sampling.

const int recalc_flags = ID_RECALC_COPY_ON_WRITE | ID_RECALC_TRANSFORM | ID_RECALC_SHADING | ID_RECALC_GEOMETRY;
if ((handle.recalc & recalc_flags) != 0) {
    inst_.sampling.reset();
}
> Is it something that can be implemented via the ED_render_id_flush_update and ED_render_scene_update? Would be nice if the depsgrapgh did not need to worry about draw-manager specific design. Looking at it, it doesn't seem like it has any kind of per-object granularity? If that's the case, then I don't think so. Ideally, especially for shadow maps, we need to know exactly which objects have updated their transform/geometry/shading (including any kind of instance). Also, keep in mind that we need some kind of solution for 4.1, the further we depart from the already existing solution, the riskier it gets. > P.S. The ID_RECALC_COPY_ON_WRITE is another internal-ish flag. In a lot of cases where it is used it should not. So I'd like to know details behind this. We just check for it to reset sampling. ``` const int recalc_flags = ID_RECALC_COPY_ON_WRITE | ID_RECALC_TRANSFORM | ID_RECALC_SHADING | ID_RECALC_GEOMETRY; if ((handle.recalc & recalc_flags) != 0) { inst_.sampling.reset(); } ```
Sergey Sharybin commented 2023-11-15 18:57:44 +01:00
Owner
Copy Link

Looking at it, it doesn't seem like it has any kind of per-object granularity? If that's the case, then I don't think so.

ED_render_id_flush_update is called for every ID which received (possibly indirectly) recalc flags. What happens deeper in there I am not currently sure. From a quick glance it seems strange the things that happen there, and does not seem to be integrated with draw manager. Maybe we should make such integration? So that the draw manager can handle current logic from flush_engine_data_update there as well.

We just check for it to reset sampling.

What case does the ID_RECALC_COPY_ON_WRITE cover which is not covered with ID_RECALC_TRANSFORM | ID_RECALC_SHADING | ID_RECALC_GEOMETRY ?

> Looking at it, it doesn't seem like it has any kind of per-object granularity? If that's the case, then I don't think so. `ED_render_id_flush_update ` is called for every ID which received (possibly indirectly) recalc flags. What happens deeper in there I am not currently sure. From a quick glance it seems strange the things that happen there, and does not seem to be integrated with draw manager. Maybe we should make such integration? So that the draw manager can handle current logic from `flush_engine_data_update` there as well. > We just check for it to reset sampling. What case does the `ID_RECALC_COPY_ON_WRITE ` cover which is not covered with `ID_RECALC_TRANSFORM | ID_RECALC_SHADING | ID_RECALC_GEOMETRY` ?
Clément Foucault commented 2023-11-15 19:21:59 +01:00
Member
Copy Link

A few precisions:

  • We want to remove DrawData eventually since we store all engine specific data per render engine instance now.
  • If I remember correctly ID_RECALC_COPY_ON_WRITE was needed for properties update on scene, world and the like (maybe the tagging itself is wrong).

I haven't a clear picture of the whole thing, but what I can say, is that we should strive to not keep it delegated to the draw manager.

A few precisions: - We want to remove `DrawData` eventually since we store all engine specific data per render engine instance now. - If I remember correctly `ID_RECALC_COPY_ON_WRITE` was needed for properties update on scene, world and the like (maybe the tagging itself is wrong). I haven't a clear picture of the whole thing, but what I can say, is that we should strive to not keep it delegated to the draw manager.
Sergey Sharybin commented 2023-11-15 19:43:07 +01:00
Owner
Copy Link

Logic specific to the behavior and implementation the draw manager without intent and clear design of other users of the logic should be local to the draw manager. We should not have draw manager specific or Eevee specific logic in dependency graph. And we should not not have multiple mechanisms of informing users of the fact that scene has been re-evaluated, or that some IDs goe new recalc flags.

Edit. If scene's property affects shading/rendering using ID_RECALC_COPY_ON_WRITE for its tag is indeed incorrect.

Logic specific to the behavior and implementation the draw manager without intent and clear design of other users of the logic should be local to the draw manager. We should not have draw manager specific or Eevee specific logic in dependency graph. And we should not not have multiple mechanisms of informing users of the fact that scene has been re-evaluated, or that some IDs goe new recalc flags. Edit. If scene's property affects shading/rendering using `ID_RECALC_COPY_ON_WRITE ` for its tag is indeed incorrect.
Sergey Sharybin commented 2023-11-16 12:45:18 +01:00
Owner
Copy Link

Talked to Clément. Here is the summary of what we've discussed:

  • Have an uint64_t update tick in the Depsgraph which is incremented in deg_evaluate_on_refresh() prior to the initialize_execution().
  • Have last_update_{transform, geometry, shading} on Id runtime. Could be specific for Objects and Lamps, and not to other ID types.
  • The evaluation function responsible for the component of that ID assigns the last_update tick. For example, BKE_object_eval_transform_final() witll assign object->runtime.last_update_transform = DEG_get_current_tick(depsgraph);.

This allows to have enough level of granularity, which potentially will be even more granular than what the depsgraph can provide. For example, BKE_object_handle_data_update can go an extra step and keep track of topology vs. coordinates only changes.

Making this work reliably will probably mean that we'll need to fix some things in the topology of the depsgraph, to ensure proper dependencies. Also, would need to use more proper flags for properties updates like DoF on camera. Doing those fixes and changes will be very welcome, and will open possibilities for further optimizations. This is how we've ended up the next part of discussion, which is not directly related to this proposal, but which seems to be very relevant to simplify and speed up things in draw manager.

Basically, two things which do not take much changes on the depsgraph level:

  • Make id_type_updated available after the scene evaluation, so that render engines in the viewport can access it. Seems easy to do by moving DEG_ids_clear_recalc(depsgraph, false); to the beginning of the scene_graph_update_tagged
  • Add did_anything_change flag in the depsgraph, which is basically true iff any of the elements of the id_type_updated is true. This can be done by graph->is_anything_changed = !graph->entry_tags.is_empty(); in the beginning of the deg_evaluate_on_refresh.

This should allow shortcuts like (only demonstration purposes, exact phrasing of the code is different):

/* Update viewport matrix, as  UI is not covered by the depsgraph. */

if (!graph->is_anything_changed) {
  /* No data changed, skip the rest of the bulk work. */
  return;
}

if (graph->id_type_updated[ID_CA]) {
  update_cameras_data();
}

This should allow to avoid things like assign_if_different and react to changes more directly. And will avoid need of looping over all possible IDs in the scene to detect changes.

Talked to Clément. Here is the summary of what we've discussed: - Have an uint64_t update tick in the Depsgraph which is incremented in `deg_evaluate_on_refresh()` prior to the `initialize_execution()`. - Have `last_update_{transform, geometry, shading}` on Id runtime. Could be specific for Objects and Lamps, and not to other ID types. - The evaluation function responsible for the component of that ID assigns the last_update tick. For example, `BKE_object_eval_transform_final()` witll assign `object->runtime.last_update_transform = DEG_get_current_tick(depsgraph);`. This allows to have enough level of granularity, which potentially will be even more granular than what the depsgraph can provide. For example, `BKE_object_handle_data_update` can go an extra step and keep track of topology vs. coordinates only changes. Making this work reliably will probably mean that we'll need to fix some things in the topology of the depsgraph, to ensure proper dependencies. Also, would need to use more proper flags for properties updates like DoF on camera. Doing those fixes and changes will be very welcome, and will open possibilities for further optimizations. This is how we've ended up the next part of discussion, which is not directly related to this proposal, but which seems to be very relevant to simplify and speed up things in draw manager. Basically, two things which do not take much changes on the depsgraph level: - Make `id_type_updated` available after the scene evaluation, so that render engines in the viewport can access it. Seems easy to do by moving `DEG_ids_clear_recalc(depsgraph, false);` to the beginning of the `scene_graph_update_tagged` - Add `did_anything_change` flag in the depsgraph, which is basically true iff any of the elements of the `id_type_updated` is true. This can be done by `graph->is_anything_changed = !graph->entry_tags.is_empty();` in the beginning of the `deg_evaluate_on_refresh`. This should allow shortcuts like (only demonstration purposes, exact phrasing of the code is different): ``` /* Update viewport matrix, as UI is not covered by the depsgraph. */ if (!graph->is_anything_changed) { /* No data changed, skip the rest of the bulk work. */ return; } if (graph->id_type_updated[ID_CA]) { update_cameras_data(); } ``` This should allow to avoid things like `assign_if_different` and react to changes more directly. And will avoid need of looping over all possible IDs in the scene to detect changes.
Miguel Pozo commented 2023-11-16 16:36:45 +01:00
Author
Member
Copy Link

The evaluation function responsible for the component of that ID assigns the last_update tick.

If that's what you decided, fine then.
But that sounds way more complex than just updating flush_engine_data_update.
I'd like to understand the advantages of this approach.
The "topology vs. coordinates only changes" seems to have more to do with the BKE_MESH_BATCH_DIRTY flags than what we are discussing here?

Could be specific for Objects and Lamps, and not to other ID types.

We need Materials too. Material/Nodes changes don't tag any Object recalc flag.

About the id_type_updated and did_anything_change, doesn't this have the same issue of assuming there has been a single Depsgraph updates between 2 redraws?

> The evaluation function responsible for the component of that ID assigns the last_update tick. If that's what you decided, fine then. But that sounds way more complex than just updating `flush_engine_data_update`. I'd like to understand the advantages of this approach. The "topology vs. coordinates only changes" seems to have more to do with the `BKE_MESH_BATCH_DIRTY` flags than what we are discussing here? > Could be specific for Objects and Lamps, and not to other ID types. We need Materials too. Material/Nodes changes don't tag any Object recalc flag. About the `id_type_updated` and `did_anything_change`, doesn't this have the same issue of assuming there has been a single Depsgraph updates between 2 redraws?
Sergey Sharybin commented 2023-11-16 17:28:40 +01:00
Owner
Copy Link

There are few advantages:

  • The dependency graph should not worrying about specifics of evaluation of different ID types, and it should not worry about how and what different areas expects to be evaluated. The flush_engine_data_update should not be happening from the dependency graph code.
  • We already have multiple ways of how dependency graph communicates about updates and needed evaluations. Adding yet another way of doing so is not a good step forward.
  • The DrawData is planned to be removed, which would also mean the flush_engine_data_update could be removed, leaving dependency graph doing the least amount of non-general code.
  • We do not want to have ID level granular last_update flags (simply put it, we do not want last_geoemtry_update on a shader node tree). And we do not want to have code like if (is_object) { if (recalc & RECALC_GEOMTRY) object.last_geoemtry_update = foo; .... } if (material) { ... } type of chain.

Long story short: saying "object knows what renderer needs to draw" is shorter than "dependency graph knows what renderer needs to draw object".

We need Materials too. Material/Nodes changes don't tag any Object recalc flag.

See my comment about changes to the topology in the graph.
Render engine does not handle materials floating in the air, or affecting invisible objects or used in drivers. So knowing that material changed is not enough, as you'll need to figure out what changed material affected on. If there is relation coming from material to object then this check becomes easier to do.

Nothing prevents of having last_updated flag on material as well. With the proposal here it is all in control of the material itself.

About the id_type_updated and did_anything_change, doesn't this have the same issue of assuming there has been a single Depsgraph updates between 2 redraws?

Those flags are accumulated between those updates. The id_type_updated is how Cycles checks for updates.

There are few advantages: - The dependency graph should not worrying about specifics of evaluation of different ID types, and it should not worry about how and what different areas expects to be evaluated. The `flush_engine_data_update` should not be happening from the dependency graph code. - We already have multiple ways of how dependency graph communicates about updates and needed evaluations. Adding yet another way of doing so is not a good step forward. - The `DrawData` is planned to be removed, which would also mean the `flush_engine_data_update` could be removed, leaving dependency graph doing the least amount of non-general code. - We do not want to have ID level granular last_update flags (simply put it, we do not want last_geoemtry_update on a shader node tree). And we do not want to have code like `if (is_object) { if (recalc & RECALC_GEOMTRY) object.last_geoemtry_update = foo; .... } if (material) { ... }` type of chain. Long story short: saying "object knows what renderer needs to draw" is shorter than "dependency graph knows what renderer needs to draw object". > We need Materials too. Material/Nodes changes don't tag any Object recalc flag. See my comment about changes to the topology in the graph. Render engine does not handle materials floating in the air, or affecting invisible objects or used in drivers. So knowing that material changed is not enough, as you'll need to figure out what changed material affected on. If there is relation coming from material to object then this check becomes easier to do. Nothing prevents of having last_updated flag on material as well. With the proposal here it is all in control of the material itself. > About the `id_type_updated` and `did_anything_change`, doesn't this have the same issue of assuming there has been a single Depsgraph updates between 2 redraws? Those flags are accumulated between those updates. The `id_type_updated` is how Cycles checks for updates.
👍 2
Miguel Pozo commented 2023-11-20 17:24:46 +01:00
Author
Member
Copy Link

I've implemented the Depsgraph part following (I think) @Sergey design. #115196
I'd like to do the EEVEE side implementation before review (even if that goes on a separate PR).
But I would still appreciate some early feedback, since this is the first time I look at this part of the code
and I wouldn't be surprised if I got something super wrong.

I've implemented the Depsgraph part following (I think) @Sergey design. #115196 I'd like to do the EEVEE side implementation before review (even if that goes on a separate PR). But I would still appreciate some early feedback, since this is the first time I look at this part of the code and I wouldn't be surprised if I got something super wrong.
❤️ 1
Blender Bot added the
Status
Archived
 label 2023-12-04 15:59:45 +01:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
main
brush-assets-project
universal-scene-description
fix-121021
blender-v4.1-release
blender-v3.3-release
blender-v3.6-release
blender-v3.6-temp_wmoss_animrig_public
temp-sculpt-dyntopo
gpencil-next
anim/animation-id-113594
blender-v4.0-release
blender-projects-basics
bridge-curves
sculpt-blender
asset-browser-frontend-split
asset-shelf
tmp-usd-python-mtl
tmp-usd-3.6
blender-v3.5-release
blender-v2.93-release
realtime-clock
sculpt-dev
bevelv2
xr-dev
v4.1.1
v3.6.11
v3.3.18
v4.1.0
v3.3.17
v3.6.10
v3.6.9
v3.3.16
v3.6.8
v3.6.7
v3.3.14
v4.0.2
v4.0.1
v4.0.0
v3.6.5
v3.3.12
v3.6.4
v3.6.3
v3.3.11
v3.6.2
v3.3.10
v3.6.1
v3.3.9
v3.6.0
v3.3.8
v3.3.7
v2.93.18
v3.5.1
v3.3.6
v2.93.17
v3.5.0
v2.93.16
v3.3.5
v3.3.4
v2.93.15
v2.93.14
v3.3.3
v2.93.13
v2.93.12
v3.4.1
v3.3.2
v3.4.0
v3.3.1
v2.93.11
v3.3.0
v3.2.2
v2.93.10
v3.2.1
v3.2.0
v2.83.20
v2.93.9
v3.1.2
v3.1.1
v3.1.0
v2.83.19
v2.93.8
v3.0.1
v2.93.7
v3.0.0
v2.93.6
v2.93.5
v2.83.18
v2.93.4
v2.93.3
v2.83.17
v2.93.2
v2.93.1
v2.83.16
v2.93.0
v2.83.15
v2.83.14
v2.83.13
v2.92.0
v2.83.12
v2.91.2
v2.83.10
v2.91.0
v2.83.9
v2.83.8
v2.83.7
v2.90.1
v2.83.6.1
v2.83.6
v2.90.0
v2.83.5
v2.83.4
v2.83.3
v2.83.2
v2.83.1
v2.83
v2.82a
v2.82
v2.81a
v2.81
v2.80
v2.80-rc3
v2.80-rc2
v2.80-rc1
v2.79b
v2.79a
v2.79
v2.79-rc2
v2.79-rc1
v2.78c
v2.78b
v2.78a
v2.78
v2.78-rc2
v2.78-rc1
v2.77a
v2.77
v2.77-rc2
v2.77-rc1
v2.76b
v2.76a
v2.76
v2.76-rc3
v2.76-rc2
v2.76-rc1
v2.75a
v2.75
v2.75-rc2
v2.75-rc1
v2.74
v2.74-rc4
v2.74-rc3
v2.74-rc2
v2.74-rc1
v2.73a
v2.73
v2.73-rc1
v2.72b
2.72b
v2.72a
v2.72
v2.72-rc1
v2.71
v2.71-rc2
v2.71-rc1
v2.70a
v2.70
v2.70-rc2
v2.70-rc
v2.69
v2.68a
v2.68
v2.67b
v2.67a
v2.67
v2.66a
v2.66
v2.65a
v2.65
v2.64a
v2.64
v2.63a
v2.63
v2.61
v2.60a
v2.60
v2.59
v2.58a
v2.58
v2.57b
v2.57a
v2.57
v2.56a
v2.56
v2.55
v2.54
v2.53
v2.52
v2.51
v2.50
v2.49b
v2.49a
v2.49
v2.48a
v2.48
v2.47
v2.46
v2.45
v2.44
v2.43
v2.42a
v2.42
v2.41
v2.40
v2.37a
v2.37
v2.36
v2.35a
v2.35
v2.34
v2.33a
v2.33
v2.32
v2.31a
v2.31
v2.30
v2.28c
v2.28a
v2.28
v2.27
v2.26
v2.25
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset Browser

  
Interest
Asset Browser Project Overview

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
This issue affects/is about backward or forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
EEVEE & Viewport

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
Legacy
Blender 2.8 Project

  
Legacy
Milestone 1: Basic, Local Asset Browser

  
Legacy
OpenGL Error

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
EEVEE & Viewport

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Priority
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
4.1
Projects
Clear projects
No project
EEVEE & Viewport
Assignees
Clear assignees
No Assignees
4 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#114112
No description provided.
Delete Branch "%!s(<nil>)"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?