Fix #104798: Slow frame-change & scrubbing with many objects #104801

Manually merged
Campbell Barton merged 2 commits from ideasman42/blender:pr-scene-collection-skip-on-cow into main 2024-04-18 05:52:21 +02:00

Avoid re-copying the scene when the scene is tagged for frame change.

For scenes with many objects scrubbing & jumping between first/last caused noticeably worse performances.

Resolve by:

  • Returning false from need_tag_cow_before_update when the recalc flag is ID_RECALC_FRAME_CHANGE.
  • Change deg_graph_tag_parameters_if_needed to consider ID_RECALC_FRAME_CHANGE a clean flag.
Avoid re-copying the scene when the scene is tagged for frame change. For scenes with many objects scrubbing & jumping between first/last caused noticeably worse performances. Resolve by: - Returning false from need_tag_cow_before_update when the recalc flag is ID_RECALC_FRAME_CHANGE. - Change deg_graph_tag_parameters_if_needed to consider ID_RECALC_FRAME_CHANGE a clean flag.
Author
Owner

@blender-bot build

@blender-bot build
Author
Owner

@blender-bot build linux

@blender-bot build linux
Campbell Barton force-pushed pr-scene-collection-skip-on-cow from 23857dc29c to 3e4349f036 2023-02-16 04:01:59 +01:00 Compare
Campbell Barton requested review from Sybren A. Stüvel 2023-02-16 04:07:53 +01:00
Campbell Barton requested review from Sergey Sharybin 2023-02-16 04:07:54 +01:00
Campbell Barton requested review from Bastien Montagne 2023-02-16 04:07:54 +01:00

All the node tress regardless of whether they are "embedded" into a "parent" datablock are treated as a "stand-alone" ID data-blocks in the dependency graph builder. This takes care of ensuring that:

  1. The "embedded" node tree itself have its dedicated evaluated state.
  2. The "parent" data-block the node tree belongs to have the field re-mapped to the evaluated state.

The goal of the special cases for the scene builder was to avoid special cases in the dependency graph nodes and relations builders, at a cost of special tricks of during copy-on-write and during tagging.

The proposed patch is based on a case which solves different problem, and does not fully implement the case it is based on. Not going into details: the evaluated scene will always point to the original master_collection, which is violation of the original/evaluated state design.


A proper solution would be to make it so that NodeType::AUDIO does not involve the copy-on-write tag for the D_RECALC_FRAME_CHANGE tag.

The most straightforward way it seems to be to make it so IDRecalcFlag is passed to need_tag_cow_before_update, and have an implementation of AudioComponentNode which will return false if the tag == ID_RECALC_FRAME_CHANGE.

All the node tress regardless of whether they are "embedded" into a "parent" datablock are treated as a "stand-alone" ID data-blocks in the dependency graph builder. This takes care of ensuring that: 1. The "embedded" node tree itself have its dedicated evaluated state. 2. The "parent" data-block the node tree belongs to have the field re-mapped to the evaluated state. The goal of the special cases for the scene builder was to avoid special cases in the dependency graph nodes and relations builders, at a cost of special tricks of during copy-on-write and during tagging. The proposed patch is based on a case which solves different problem, and does not fully implement the case it is based on. Not going into details: the evaluated scene will always point to the original `master_collection`, which is violation of the original/evaluated state design. --- A proper solution would be to make it so that `NodeType::AUDIO` does not involve the copy-on-write tag for the `D_RECALC_FRAME_CHANGE` tag. The most straightforward way it seems to be to make it so `IDRecalcFlag` is passed to `need_tag_cow_before_update`, and have an implementation of `AudioComponentNode` which will return `false` if the `tag == ID_RECALC_FRAME_CHANGE`.
Sergey Sharybin requested changes 2023-02-16 10:38:29 +01:00
Dismissed
Sergey Sharybin left a comment
Owner

Marking as requested changes.

Marking as requested changes.
Author
Owner

The proposed patch is based on a case which solves different problem, and does not fully implement the case it is based on. Not going into details: the evaluated scene will always point to the original master_collection, which is violation of the original/evaluated state design.

I'm aware of this and checked to make sure this wasn't happening, attached a patch that adds a print to deg_expand_copy_on_write_datablock, in my tests it's never sharing the pointer. Is there another point in the code where the pointers would be shared?

> The proposed patch is based on a case which solves different problem, and does not fully implement the case it is based on. Not going into details: the evaluated scene will always point to the original `master_collection`, which is violation of the original/evaluated state design. I'm aware of this and checked to make sure this wasn't happening, attached a patch that adds a print to `deg_expand_copy_on_write_datablock`, in my tests it's never sharing the pointer. Is there another point in the code where the pointers would be shared?
Campbell Barton requested review from Sergey Sharybin 2023-02-17 03:45:12 +01:00
Brecht Van Lommel added this to the Core project 2023-02-20 10:40:36 +01:00

I'm aware of this and checked to make sure this wasn't happening

If you're aware that this code violates an existing design, that should be loudly warned about in the description, and not left as an exercise for the reader.

Also I don't think that adding a printf() somewhere and not seeing it print anything during testing is enough of a motivation to move away from an established design.

> I'm aware of this and checked to make sure this wasn't happening If you're aware that this code violates an existing design, that should be loudly warned about in the description, and not left as an exercise for the reader. Also I don't think that adding a `printf()` somewhere and not seeing it print anything during testing is enough of a motivation to move away from an established design.
Sergey Sharybin requested changes 2023-02-20 16:58:20 +01:00
Dismissed
Sergey Sharybin left a comment
Owner

Please always give all the information you have when presenting a patch. Especially when it does not use things the way they are intended to.

I also would not trust the prints at a specific snapshot of code. We should not base any design on the specific behavior of another area, without ensuring that the behavior is there by design.

The only reason why the evaluated ID for the master_collection is created which I can think of is that the built-in into scene master_collection is references by a layer collection. It is hard to see from the code (this is why clear design are essential). But if that is true, then we should indeed treat the masetr_collection the same way as the nodetree. Otherwise we are probably making a mess from life time of objects and memory usage. But that would mean bug fix in the depsgraph handling master_collection and not the speedup.

For the actual speedup we do need to avoid copy-on-write as much as possible, because one also does not want to do the ID remapping routines (which this patch does not address at all).

Please always give all the information you have when presenting a patch. Especially when it does not use things the way they are intended to. I also would not trust the prints at a specific snapshot of code. We should not base any design on the specific behavior of another area, without ensuring that the behavior is there by design. The only reason why the evaluated ID for the `master_collection` is created which I can think of is that the built-in into scene `master_collection` is references by a layer collection. It is hard to see from the code (this is why clear design are essential). But if that is true, then we should indeed treat the `masetr_collection` the same way as the `nodetree`. Otherwise we are probably making a mess from life time of objects and memory usage. But that would mean bug fix in the depsgraph handling `master_collection` and not the speedup. For the actual speedup we do need to avoid copy-on-write as much as possible, because one also does not want to do the ID remapping routines (which this patch does not address at all).
Author
Owner

I'm aware of this and checked to make sure this wasn't happening

If you're aware that this code violates an existing design, that should be loudly warned about in the description, and not left as an exercise for the reader.

As far as I can see it's not violating any existing design (as in I checked to make sure it's not sharing pointers).

As I understand it - the master_collection is an embedded ID, copying the COW scene already copies the ID, so it makes sense to treat this the same as other embedded ID's.

> > I'm aware of this and checked to make sure this wasn't happening > > If you're aware that this code violates an existing design, that should be loudly warned about in the description, and not left as an exercise for the reader. As far as I can see it's not violating any existing design (as in I checked to make sure it's not sharing pointers). As I understand it - the `master_collection` is an embedded ID, copying the COW scene already copies the ID, so it makes sense to treat this the same as other embedded ID's.
Author
Owner

Please always give all the information you have when presenting a patch. Especially when it does not use things the way they are intended to.

My impression was this is using the API the way it's intended - as it's following a workaround used for many other embedded ID data-blocks.

I also would not trust the prints at a specific snapshot of code. We should not base any design on the specific behavior of another area, without ensuring that the behavior is there by design.

Fair enough, however duplicating scene (the COW copy) always duplicates the master_collection so as far as I can see this will never share a pointer with the original data.

The only reason why the evaluated ID for the master_collection is created which I can think of is that the built-in into scene master_collection is references by a layer collection. It is hard to see from the code (this is why clear design are essential). But if that is true, then we should indeed treat the masetr_collection the same way as the nodetree. Otherwise we are probably making a mess from life time of objects and memory usage. But that would mean bug fix in the depsgraph handling master_collection and not the speedup.

The embedded collection shouldn't be referenced by anything except for the scene (it's not stored in G_MAIN->collections for e.g.)

For the actual speedup we do need to avoid copy-on-write as much as possible, because one also does not want to do the ID remapping routines (which this patch does not address at all).

Which area are you referring to?
This patch updates ntree_hack_remap_pointers & scene_foreach_id uses BKE_library_foreach_ID_embedded for scene->master_collection.

> Please always give all the information you have when presenting a patch. Especially when it does not use things the way they are intended to. My impression was this is using the API the way it's intended - as it's following a workaround used for many other embedded ID data-blocks. > I also would not trust the prints at a specific snapshot of code. We should not base any design on the specific behavior of another area, without ensuring that the behavior is there by design. Fair enough, however duplicating scene (the COW copy) always duplicates the `master_collection` so as far as I can see this will never share a pointer with the original data. > The only reason why the evaluated ID for the `master_collection` is created which I can think of is that the built-in into scene `master_collection` is references by a layer collection. It is hard to see from the code (this is why clear design are essential). But if that is true, then we should indeed treat the `masetr_collection` the same way as the `nodetree`. Otherwise we are probably making a mess from life time of objects and memory usage. But that would mean bug fix in the depsgraph handling `master_collection` and not the speedup. The embedded collection shouldn't be referenced by anything except for the scene (it's not stored in `G_MAIN->collections` for e.g.) > For the actual speedup we do need to avoid copy-on-write as much as possible, because one also does not want to do the ID remapping routines (which this patch does not address at all). Which area are you referring to? This patch updates `ntree_hack_remap_pointers` & `scene_foreach_id` uses `BKE_library_foreach_ID_embedded` for `scene->master_collection`.
Campbell Barton requested review from Sergey Sharybin 2023-02-27 12:08:10 +01:00

My impression was this is using the API the way it's intended - as it's following a workaround used for many other embedded ID data-blocks.

Yeah, something that i guess could have been described better in the comment above.

Basically, the dependency graph handles the "embedded" IDs the same way as non-embedded onces. This means, for example, that node tree relation builder does not destinguish non-embedded node groups from embedded compositor/shader. And the copy-on-write takes care of remapping the "embedded" pointers to the, what appears to be non-embedded from the depsgraph perspective, ID.

In practice this means that the graph builder and the copy-on-write update step needs to make the same (weak) assumptions.

Fair enough, however duplicating scene (the COW copy) always duplicates the master_collection so as far as I can see this will never share a pointer with the original data.

I don't really understand how this works.

First of all, in the case of embedded node trees we do not want the ID copy function to duplicate or allocate that pointer. The depsgraph takes care of assigning it to a proper evaluated state.

The code in the patch temporarily assigns the master_collection to nullptr, and then calls the ID copy function, and restores the pointer back. If the ID copy function allocates master_collection then there is a memory leak. But in any case, the master_collection is restored to its original value.

In the case of embedded node trees it is the BKE_library_foreach_ID_link() + foreach_libblock_remap_callback() which takes care of remapping the embedded pointer to an evaluated node ID pointer from the depsgraph.

If the master_collection follows the same exact code as the embedded node trees and it has different pointer in the evaluated scene it means that the master collection somehow got created in the dependency graph.

Thing is: unlike the embedded node trees for scene compositor and material shader the dependency graph builder does not access scene->master_collection at all. So, if there is no such explicit access where does the ID node for the master_collection is coming from?

Which leads to the next point...

The embedded collection shouldn't be referenced by anything except for the scene (it's not stored in G_MAIN->collections for e.g.)

So I tohught as well.. However. Without going deep into code or trying to find an actual design, lets use a trusty old printf way of checking the obvious suspect: view_layer->layer_collections.

diff --git a/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc b/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc
index d7420b91db4..5fc316563a2 100644
--- a/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc
+++ b/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc
@@ -53,6 +53,7 @@ void DepsgraphNodeBuilder::build_layer_collections(ListBase *lb)
     }
     if ((lc->flag & LAYER_COLLECTION_EXCLUDE) == 0) {
       build_collection(lc, lc->collection);
+      printf("%d\n", lc->collection == scene_->master_collection);
     }
     build_layer_collections(&lc->layer_collections);
   }

Sure enough, the output is:

sergey@ws-cheese ~/Developer/blender/build/release % ./bin/Blender.app/Contents/MacOS/Blender --factory-startup
Device with name AMD Radeon PRO W6800X supports metal minimum requirements
METAL API - DETECTED GPU: AMD Radeon PRO W6800X
1
0

So is it by design? Is it always guaranteed that the first layer collection references embedded master_collection?

Which area are you referring to?
This patch updates ntree_hack_remap_pointers & scene_foreach_id uses ?BKE_library_foreach_ID_embedded for scene->master_collection.

The entire copy-on-write can/should be skipped on the frame change. Dependency graph's DEG_evaluate_on_framechange takes care of updating the frame value. Doing this Scene datablock free + duplicate + BKE_library_foreach_ID_embedded is not needed at all.

It could as simple as making it so Audio component returns false from its need_tag_cow_before_update, at least on a frame change. For volume changes we still might want to copy-on-write to grab the new value of volume.

This sounds more logical/correct behavior of frame change handling, and it solves the root of the problem stated in the description: the ID_RECALC_FRAME_CHANGE causing heavy scene copy-on-write.

Long story short: if copy-on-write is not needed, don't do it at all.

> My impression was this is using the API the way it's intended - as it's following a workaround used for many other embedded ID data-blocks. Yeah, something that i guess could have been described better in the comment above. Basically, the dependency graph handles the "embedded" IDs the same way as non-embedded onces. This means, for example, that node tree relation builder does not destinguish non-embedded node groups from embedded compositor/shader. And the copy-on-write takes care of remapping the "embedded" pointers to the, what appears to be non-embedded from the depsgraph perspective, ID. In practice this means that the graph builder and the copy-on-write update step needs to make the same (weak) assumptions. > Fair enough, however duplicating scene (the COW copy) always duplicates the master_collection so as far as I can see this will never share a pointer with the original data. I don't really understand how this works. First of all, in the case of embedded node trees we do not want the ID copy function to duplicate or allocate that pointer. The depsgraph takes care of assigning it to a proper evaluated state. The code in the patch temporarily assigns the `master_collection` to `nullptr`, and then calls the ID copy function, and restores the pointer back. If the ID copy function allocates `master_collection` then there is a memory leak. But in any case, the `master_collection` is restored to its original value. In the case of embedded node trees it is the `BKE_library_foreach_ID_link()` + `foreach_libblock_remap_callback()` which takes care of remapping the embedded pointer to an evaluated node ID pointer from the depsgraph. If the `master_collection` follows the same exact code as the embedded node trees and it has different pointer in the evaluated scene it means that the master collection somehow got created in the dependency graph. Thing is: unlike the embedded node trees for scene compositor and material shader the dependency graph builder does not access `scene->master_collection` at all. So, if there is no such explicit access where does the ID node for the `master_collection` is coming from? Which leads to the next point... > The embedded collection shouldn't be referenced by anything except for the scene (it's not stored in G_MAIN->collections for e.g.) So I tohught as well.. However. Without going deep into code or trying to find an actual design, lets use a trusty old `printf` way of checking the obvious suspect: `view_layer->layer_collections`. ``` diff --git a/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc b/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc index d7420b91db4..5fc316563a2 100644 --- a/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc +++ b/source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc @@ -53,6 +53,7 @@ void DepsgraphNodeBuilder::build_layer_collections(ListBase *lb) } if ((lc->flag & LAYER_COLLECTION_EXCLUDE) == 0) { build_collection(lc, lc->collection); + printf("%d\n", lc->collection == scene_->master_collection); } build_layer_collections(&lc->layer_collections); } ``` Sure enough, the output is: ``` sergey@ws-cheese ~/Developer/blender/build/release % ./bin/Blender.app/Contents/MacOS/Blender --factory-startup Device with name AMD Radeon PRO W6800X supports metal minimum requirements METAL API - DETECTED GPU: AMD Radeon PRO W6800X 1 0 ``` So is it by design? Is it always guaranteed that the first layer collection references embedded `master_collection`? > Which area are you referring to? > This patch updates ntree_hack_remap_pointers & scene_foreach_id uses ?BKE_library_foreach_ID_embedded for scene->master_collection. The entire copy-on-write can/should be skipped on the frame change. Dependency graph's `DEG_evaluate_on_framechange` takes care of updating the frame value. Doing this Scene datablock free + duplicate + `BKE_library_foreach_ID_embedded` is not needed at all. It could as simple as making it so `Audio` component returns `false` from its `need_tag_cow_before_update`, at least on a frame change. For volume changes we still might want to copy-on-write to grab the new value of volume. This sounds more logical/correct behavior of frame change handling, and it solves the root of the problem stated in the description: the `ID_RECALC_FRAME_CHANGE` causing heavy scene copy-on-write. Long story short: if copy-on-write is not needed, don't do it at all.
Campbell Barton force-pushed pr-scene-collection-skip-on-cow from 3e4349f036 to f9ddbce145 2023-03-02 07:12:49 +01:00 Compare
Author
Owner

Confirmed that skipping the COW tag for Audio components fixes the issue on frame change, so if COW isn't needed for anything else - this fixes #104798.

Skipping the collection copy still seems worthwhile though, practically all scene settings (some material shading settings) are causing the collection to be duplicated (so changing the render-resolution or scene samples duplicates the collection and rebuilds it's hash on each redraw - which can cause a noticeable slowdown).

I wasn't aware the master_collection can be referenced from LayerCollections. As long as the LayerCollection is part of the scene - I think this could be supported without causing problem however that needs to be investigated in more detail.


Updated the patch to change Audio tagging not to copy-on-write as suggested.


@blender-bot build linux

Confirmed that skipping the COW tag for Audio components fixes the issue on frame change, so if COW isn't needed for anything else - this fixes #104798. Skipping the collection copy still seems worthwhile though, practically all scene settings (some material shading settings) are causing the collection to be duplicated (so changing the render-resolution or scene samples duplicates the collection and rebuilds it's hash on each redraw - which can cause a noticeable slowdown). I wasn't aware the `master_collection` can be referenced from `LayerCollections`. As long as the `LayerCollection` is part of the scene - I think this could be supported without causing problem however that needs to be investigated in more detail. ---- Updated the patch to change Audio tagging not to copy-on-write as suggested. ---- @blender-bot build linux

The copy-on-write is only obviously not needed for the ID_RECALC_FRAME_CHANGE, as the dependency graph is taking care of the frame change. For other types the dependency graph does not ensure anything, so other tags might still need copy-on-write to happen. See my comment above where Iv'e mentioned this.

For example ID_RECALC_AUDIO_MUTE and ID_RECALC_AUDIO_LISTENER definitely need copy-on-write.

The ID_RECALC_AUDIO_VOLUME and ID_RECALC_AUDIO_FPS are a bit more tricky. If those flags will not imply copy-on-write nothing will immediately break. But! This is only because of they're used in combination with ID_RECALC_SEQUENCER_STRIPS. Arguably, this combination is wrong and the tag should only happen for VOLUME and FPS, and then the tag is to be flushed to the sequencer components. In any case, designing component copy-on-write component based on the current state of flags used to tag is weak, and we should not do so.

The copy-on-write is only obviously not needed for the `ID_RECALC_FRAME_CHANGE`, as the dependency graph is taking care of the frame change. For other types the dependency graph does not ensure anything, so other tags might still need copy-on-write to happen. See my comment above where Iv'e mentioned this. For example `ID_RECALC_AUDIO_MUTE` and `ID_RECALC_AUDIO_LISTENER` definitely need copy-on-write. The `ID_RECALC_AUDIO_VOLUME` and `ID_RECALC_AUDIO_FPS` are a bit more tricky. If those flags will not imply copy-on-write nothing will immediately break. But! This is only because of they're used in combination with `ID_RECALC_SEQUENCER_STRIPS`. Arguably, this combination is wrong and the tag should only happen for VOLUME and FPS, and then the tag is to be flushed to the sequencer components. In any case, designing component copy-on-write component based on the current state of flags used to tag is weak, and we should not do so.

For the record, it seems that Sergey had some remarks that haven't resulted in a change of this PR yet. I was actually waiting for that to happen before diving into the code myself.

For the record, it seems that Sergey had some remarks that haven't resulted in a change of this PR yet. I was actually waiting for that to happen before diving into the code myself.
Bastien Montagne requested changes 2024-02-12 10:38:49 +01:00
Bastien Montagne left a comment
Owner

Is this still relevant PR?

Is this still relevant PR?
Campbell Barton force-pushed pr-scene-collection-skip-on-cow from f9ddbce145 to 0a4907dae6 2024-02-14 07:50:50 +01:00 Compare
Author
Owner

The issue described in #104798 still exists.

Updated the PR & description with the change Sergey suggested (passing a recalc flag to AudioComponentNode::need_tag_cow_before_update).

This didn't fix the problem on it's own though, deg_graph_tag_parameters_if_needed also needed to exclude ID_RECALC_FRAME_CHANGE from clean_flags, which is correct as far as I can see since the depsgraph writes into the COW scenes current frame directly.

The issue described in #104798 still exists. Updated the PR & description with the change Sergey suggested (passing a recalc flag to `AudioComponentNode::need_tag_cow_before_update`). This didn't fix the problem on it's own though, `deg_graph_tag_parameters_if_needed` also needed to exclude `ID_RECALC_FRAME_CHANGE` from `clean_flags`, which is correct as far as I can see since the depsgraph writes into the COW scenes current frame directly.
Campbell Barton requested review from Bastien Montagne 2024-02-14 08:01:13 +01:00
Author
Owner

@blender-bot build

@blender-bot build
Sybren A. Stüvel reviewed 2024-02-20 11:33:21 +01:00
Sybren A. Stüvel left a comment
Member

Apart from a few small remarks (see inline notes) the code LGTM.

I'm having a harder time correlating the results to the behaviour described in #104798, though.

Without patch With patch
USE_SCENE_COLLECTION = False ~24 FPS ~57 FPS
USE_SCENE_COLLECTION = True ~23 FPS ~56 FPS

So yes, this patch seems to increase performance quite considerably 👍
I can't seem to reproduce the issue in the bug report, though, as for me both 'scene collection' and 'other collection' cases are equally fast.

I tested this PR against 8a4c45a202, which seems to be the parent commit when I pull this PR.

Update: the script to reproduce the issue from #104798 doesn't include audio, though, so I also don't feel that it is sufficient to test the changes in this PR. @ideasman42 how did you test? Can we reproduce that?

Apart from a few small remarks (see inline notes) the code LGTM. I'm having a harder time correlating the results to the behaviour described in #104798, though. | | Without patch | With patch | | -- | -- | -- | | `USE_SCENE_COLLECTION = False` | ~24 FPS | ~57 FPS | | `USE_SCENE_COLLECTION = True` | ~23 FPS | ~56 FPS | So yes, this patch seems to increase performance quite considerably :+1: I can't seem to reproduce the issue in the bug report, though, as for me both 'scene collection' and 'other collection' cases are equally fast. I tested this PR against 8a4c45a2025906e02268dbcd073c7d47ddfce20d, which seems to be the parent commit when I pull this PR. **Update:** the script to reproduce the issue from #104798 doesn't include audio, though, so I also don't feel that it is sufficient to test the changes in this PR. @ideasman42 how did you test? Can we reproduce that?
@ -241,1 +240,4 @@
/* Audio component. */
struct AudioComponentNode : public ComponentNode {
virtual bool need_tag_cow_before_update(IDRecalcFlag tag) override

const IDRecalcFlag flag

`const IDRecalcFlag flag`
ideasman42 marked this conversation as resolved
@ -242,0 +244,4 @@
{
/* Frame change doesn't require a copy of the scene, doing so can be a heavy operation
* especially when the collection contains many objects, see #104798. */
if (tag == ID_RECALC_FRAME_CHANGE) {

Personally I'd prefer return tag != ID_RECALC_FRAME_CHANGE;

No strong feelings, it mostly depends on how likely it is that other cases will be added. If you keep the if, I think it's better to put the explanation inside the conditional block.

Personally I'd prefer `return tag != ID_RECALC_FRAME_CHANGE;` No strong feelings, it mostly depends on how likely it is that other cases will be added. If you keep the `if`, I think it's better to put the explanation inside the conditional block.
ideasman42 marked this conversation as resolved
Campbell Barton force-pushed pr-scene-collection-skip-on-cow from 0a4907dae6 to 8541e4808e 2024-02-20 12:54:42 +01:00 Compare
Author
Owner

Interesting, I wasn't aware of this performance difference, when updating the patch I was just checking the slow case was faster, but the difference in performance doesn't depend much on USE_SCENE_COLLECTION anymore and there is a speedup in both cases on my system too.

Increasing performance for both cases, changed between 3.6 & 4.0 (where there is a big difference in performance in 3.6 but not in 4.0 and newer), noted in #104798.

@dr.sybren audio can be tested by manually adding it to the sequencer, scrubbing with the cursor & arrow keys works as before. Animated audio settings could depend on having a scene copy although this seems to be broken (animating the scenes volume never updates & looks to be set at the volume when playback starts, even the SOUND_OT_update_animation_flags operator doesn't seem to be working).

Interesting, I wasn't aware of this performance difference, when updating the patch I was just checking the slow case was faster, but the difference in performance doesn't depend much on `USE_SCENE_COLLECTION` anymore and there is a speedup in both cases on my system too. Increasing performance for both cases, changed between 3.6 & 4.0 (where there is a big difference in performance in 3.6 but not in 4.0 and newer), noted in #104798. @dr.sybren audio can be tested by manually adding it to the sequencer, scrubbing with the cursor & arrow keys works as before. Animated audio settings could depend on having a scene copy although this seems to be broken (animating the scenes volume never updates & looks to be set at the volume when playback starts, even the `SOUND_OT_update_animation_flags` operator doesn't seem to be working).
Campbell Barton changed title from Fix #104798: Slower frame-change with many objects in the scenes collection to Fix #104798: Slow frame-change & scrubbing with many objects 2024-02-21 07:30:25 +01:00
Campbell Barton force-pushed pr-scene-collection-skip-on-cow from 8541e4808e to d3490494bb 2024-04-05 08:38:46 +02:00 Compare
Author
Owner

@blender-bot build

@blender-bot build

@ideasman42 On the code side I think it is implementing all the things we've discussed before, and that part seems fine now. I run tests with some of anim files we've got, and all of them seem to work correctly.

I did not see performance impact on the production scenes, but with the setup from the original report (created by a script) there is quite measurable boost in FPS. And logically AUDIO should be causing copy-for-eval on frame change.

The thing which would be nice to clarify is this:

Animated audio settings could depend on having a scene copy although this seems to be broken

Do you mean it is already broken before this PR, or is it broken with this PR and more work is needed?

@ideasman42 On the code side I think it is implementing all the things we've discussed before, and that part seems fine now. I run tests with some of anim files we've got, and all of them seem to work correctly. I did not see performance impact on the production scenes, but with the setup from the original report (created by a script) there is quite measurable boost in FPS. And logically AUDIO should be causing copy-for-eval on frame change. The thing which would be nice to clarify is this: > Animated audio settings could depend on having a scene copy although this seems to be broken Do you mean it is already broken before this PR, or is it broken with this PR and more work is needed?
Author
Owner

Animated audio settings could depend on having a scene copy although this seems to be broken

Do you mean it is already broken before this PR, or is it broken with this PR and more work is needed?

It's already broken before this PR, reported the bug #120726.

It looks like SOUND_OT_update_animation_flags isn't called anywhere, yet the flags it sets are expected to be used for animated audio to work properly.

> > Animated audio settings could depend on having a scene copy although this seems to be broken > > Do you mean it is already broken before this PR, or is it broken with this PR and more work is needed? It's already broken before this PR, reported the bug #120726. It looks like `SOUND_OT_update_animation_flags` isn't called anywhere, yet the flags it sets are expected to be used for animated audio to work properly.
Sergey Sharybin approved these changes 2024-04-17 10:50:39 +02:00
Sergey Sharybin left a comment
Owner

Thanks for clarification. I don't think that issue should be holding off this PR.

Thanks for clarification. I don't think that issue should be holding off this PR.
Campbell Barton manually merged commit 7047715c6b into main 2024-04-18 05:52:21 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
Asset Browser Project
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
No Milestone
No project
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#104801
No description provided.