Anim: add ID* cache of users to Action Bindings #123187

Merged
Sybren A. Stüvel merged 3 commits from dr.sybren/blender:anim/baklava-bindings-to-id-cache into main 2024-06-28 14:41:43 +02:00

Keep track of which IDs are animated by which Action Binding. This will be
necessary for display in the Action editor, where animation data that is
unrelated to the active object can be shown (when "show all bindings" is
on).

Note: animation evaluation will not be using this cache, at least not in
the near future. Potentially when we introduce animation-level constraints
this will change, but that's for the future.


This also adds an RNA function action.bindings["name"].debug_log_users(),
which can be used to print the list of users of that binding to stdout.
This is purely for debugging/testing this PR, and will be removed before
landing on main.

The user cache isn't actually used in this PR. It will be used soon in
#122672. That in itself is a big enough PR that I thought this aspect would
be nice to have as a separate one.

I've attached a blend file that's already set up for testing. Just press enter in the Python console, then check the terminal.

Keep track of which IDs are animated by which Action Binding. This will be necessary for display in the Action editor, where animation data that is unrelated to the active object can be shown (when "show all bindings" is on). Note: animation evaluation will not be using this cache, at least not in the near future. Potentially when we introduce animation-level constraints this will change, but that's for the future. -------------- This also adds an RNA function `action.bindings["name"].debug_log_users()`, which can be used to print the list of users of that binding to stdout. This is purely for debugging/testing this PR, and will be removed before landing on `main`. The user cache isn't actually used in this PR. It will be used soon in #122672. That in itself is a big enough PR that I thought this aspect would be nice to have as a separate one. I've attached a blend file that's already set up for testing. Just press enter in the Python console, then check the terminal.
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2024-06-13 15:29:09 +02:00
Sybren A. Stüvel added 1 commit 2024-06-13 15:29:18 +02:00
Anim: add ID* cache of users to Action Bindings
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
4ead1a001b
Keep track of which IDs are animated by which Action Binding. This will be
necessary for display in the Action editor, where animation data that is
unrelated to the active object can be shown (when "show all bindings" is
on).

Note: animation evaluation will not be using this cache, at least not in
the near future. Potentially when we introduce animation-level constraints
this will change, but that's for the future.

--------------

This also adds an RNA function `action.bindings["name"].debug_log_users()`,
which can be used to print the list of users of that binding to stdout.
This is purely for debugging/testing this PR, and will be removed before
landing on `main`.

The user cache isn't actually used in this PR. It will be used soon in
#122672. That in itself is a big enough PR that I thought this aspect would
be nice to have as a separate one.
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel added 1 commit 2024-06-13 15:32:32 +02:00
Sybren A. Stüvel requested review from Nathan Vegdahl 2024-06-13 15:36:36 +02:00
Sybren A. Stüvel requested review from Bastien Montagne 2024-06-13 15:36:37 +02:00
Hans Goudey reviewed 2024-06-13 16:34:41 +02:00
@ -618,1 +622,4 @@
Binding::~Binding()
{
MEM_SAFE_FREE(this->binding_runtime);
Member

This has to use MEM_delete because the runtime struct is non-trivial

This has to use `MEM_delete` because the runtime struct is non-trivial
dr.sybren marked this conversation as resolved
@ -635,1 +644,4 @@
BindingRuntime &Binding::runtime()
{
if (!this->binding_runtime) {
Member

It would be good to do this consistently with how we manage runtime data for other C++ wrappers of DNA types. Typically the runtime struct is created when reading the file. It should be allocated always basically. The C++ API for the struct can basically abstract away the fact that there's a separate runtime struct.

It would be good to do this consistently with how we manage runtime data for other C++ wrappers of DNA types. Typically the runtime struct is created when reading the file. It should be allocated always basically. The C++ API for the struct can basically abstract away the fact that there's a separate runtime struct.
dr.sybren marked this conversation as resolved
@ -636,0 +650,4 @@
return *this->binding_runtime;
}
Set<ID *> &Binding::users()
Member

Can this return a const set maybe?

Can this return a const set maybe?
Author
Member

No, because it's used by users_add() and users_remove(), but I can make it protected.

Yes I can :)

~~No, because it's used by `users_add()` and `users_remove()`, but I can make it `protected`.~~ Yes I can :)
dr.sybren marked this conversation as resolved
Sybren A. Stüvel changed title from Anim: add `ID*` cache of users to Action Bindings to WIP: Anim: add `ID*` cache of users to Action Bindings 2024-06-13 17:10:24 +02:00
Sybren A. Stüvel added 1 commit 2024-06-13 17:17:01 +02:00
Sybren A. Stüvel added 2 commits 2024-06-13 17:21:33 +02:00
Add a debug print for ease of reviewing, will remove before landing
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
51f59529d3
Sybren A. Stüvel changed title from WIP: Anim: add `ID*` cache of users to Action Bindings to Anim: add `ID*` cache of users to Action Bindings 2024-06-13 17:22:17 +02:00
Hans Goudey reviewed 2024-06-13 17:26:58 +02:00
@ -383,6 +401,7 @@ static void read_bindings(BlendDataReader *reader, animrig::Action &anim)
for (int i = 0; i < anim.binding_array_num; i++) {
BLO_read_struct(reader, ActionBinding, &anim.binding_array[i]);
anim.binding_array[i]->wrap().runtime_allocate();
Member

I feel less strongly about this than my previous comment, and maybe this is verging on a nitpicky comment, but I do wonder a bit why the runtime allocation is wrapped in a member function? Typically the file reading code is so low level already that IMO it isn't really worth abstracting things like that.

There are other ways to structure this, for example curves_blend_read_data calls a blend_read function on bke::CurvesGeometry. Searching for ->runtime = gives more examples too.

Anyway, just thought it was worth mentioning to build consensus about how to handle this.

I feel less strongly about this than my previous comment, and maybe this is verging on a nitpicky comment, but I do wonder a bit why the runtime allocation is wrapped in a member function? Typically the file reading code is so low level already that IMO it isn't really worth abstracting things like that. There are other ways to structure this, for example `curves_blend_read_data` calls a `blend_read` function on `bke::CurvesGeometry`. Searching for `->runtime = ` gives more examples too. Anyway, just thought it was worth mentioning to build consensus about how to handle this.
Author
Member

I feel less strongly about this than my previous comment, and maybe this is verging on a nitpicky comment, but I do wonder a bit why the runtime allocation is wrapped in a member function? Typically the file reading code is so low level already that IMO it isn't really worth abstracting things like that.

I'm happy to explain. I consider the runtime struct to be something internal to the animrig module, and so I wrote things so that users of the blender::animrig::Binding class don't need to know anything about the BindingRuntime class. All manipulation of the Binding goes either via its owning Action or via functions on the Binding, and I don't really want to expand that API surface area to yet another class.

There are other ways to structure this, for example curves_blend_read_data calls a blend_read function on bke::CurvesGeometry. Searching for ->runtime = gives more examples too.

That looks pretty nice too, although there blender::bke contains the runtime class, which is what I was trying to avoid. Of course I could write a similar blend_read() function on the animrig::Action class, but that would imply a bigger refactor, moving all the blend read/write code from bke to animrig.

Anyway, just thought it was worth mentioning to build consensus about how to handle this.

I agree it would be good to have some uniformity here. I do feel that that would be outside the scope of this PR though.

What I could do is something in the middle. I could rename runtime_allocate() to blend_read_post(). That way it's less specific about allocating the runtime, and it's more generically "handle the fact that this Binding was just loaded from a blend file". What would you think about that?

> I feel less strongly about this than my previous comment, and maybe this is verging on a nitpicky comment, but I do wonder a bit why the runtime allocation is wrapped in a member function? Typically the file reading code is so low level already that IMO it isn't really worth abstracting things like that. I'm happy to explain. I consider the runtime struct to be something internal to the `animrig` module, and so I wrote things so that users of the `blender::animrig::Binding` class don't need to know anything about the `BindingRuntime` class. All manipulation of the `Binding` goes either via its owning `Action` or via functions on the `Binding`, and I don't really want to expand that API surface area to yet another class. > There are other ways to structure this, for example `curves_blend_read_data` calls a `blend_read` function on `bke::CurvesGeometry`. Searching for `->runtime = ` gives more examples too. That looks pretty nice too, although there `blender::bke` contains the runtime class, which is what I was trying to avoid. Of course I could write a similar `blend_read()` function on the `animrig::Action` class, but that would imply a bigger refactor, moving all the blend read/write code from `bke` to `animrig`. > Anyway, just thought it was worth mentioning to build consensus about how to handle this. I agree it would be good to have some uniformity here. I do feel that that would be outside the scope of this PR though. What I could do is something in the middle. I could rename `runtime_allocate()` to `blend_read_post()`. That way it's less specific about allocating the runtime, and it's more generically "handle the fact that this Binding was just loaded from a blend file". What would you think about that?
Author
Member

I've just gone ahead and done that, because I think it's cleaner too.

I've just gone ahead and done that, because I think it's cleaner too.
dr.sybren marked this conversation as resolved
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel added 1 commit 2024-06-14 10:03:02 +02:00
Sybren A. Stüvel added 1 commit 2024-06-14 10:07:18 +02:00
Fix test failures by setting G.main in unit tests
Some checks reported errors
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
a3e796e17a
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel added 1 commit 2024-06-14 10:13:52 +02:00
Rename Binding::runtime_allocate() to Binding::blend_read_post()
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
9d7f34636c
Author
Member

@blender-bot build

@blender-bot build
Nathan Vegdahl requested changes 2024-06-14 12:22:07 +02:00
Dismissed
Nathan Vegdahl left a comment
Member

Looks good to me, aside from my comments about Set.

Looks good to me, aside from my comments about `Set`.
@ -633,6 +663,41 @@ bool Binding::has_idtype() const
return this->idtype != 0;
}
Set<ID *> &Binding::users()
Member

A Set seems overkill to me. The expected number of users is 1, and the presumably overwhelmingly common case will be a small number. So I think it's better to optimize for that. A Vector would be a good fit. And then we can just return a Span as well.

(Note that Vector and Span also have a contains() method that just does a simple linear scan, so checking for membership will still be convenient. And Vector has various methods that make implementing set-like operations trivial.)

A `Set` seems overkill to me. The expected number of users is 1, and the presumably overwhelmingly common case will be a small number. So I think it's better to optimize for that. A `Vector` would be a good fit. And then we can just return a `Span` as well. (Note that `Vector` and `Span` also have a `contains()` method that just does a simple linear scan, so checking for membership will still be convenient. And `Vector` has various methods that make implementing set-like operations trivial.)

To performance topic: #118848 (comment)

To performance topic: https://projects.blender.org/blender/blender/pulls/118848#issuecomment-1134506
dr.sybren marked this conversation as resolved
@ -0,0 +80,4 @@
void rebuild_binding_user_cache()
{
/* TODO: see if we can get rid of this G_MAIN usage. */
Member

I think we should also add a TODO for handling library linked data.

I think we should also add a TODO for handling library linked data.
Author
Member

Not really, as that works fine. This will give you some debug prints, in case you want to try:

diff --git a/source/blender/animrig/intern/action_runtime.cc b/source/blender/animrig/intern/action_runtime.cc
index c06f257f401..ab7567b6181 100644
--- a/source/blender/animrig/intern/action_runtime.cc
+++ b/source/blender/animrig/intern/action_runtime.cc
@@ -34,8 +34,10 @@ static void rebuild_binding_user_cache_for_bmain(Main &bmain)
   // Note to reviewers: this is just to make the refresh visible. I'll remove this before landing.
   printf("rebuild_binding_user_cache_for_bmain()\n");
 
+  printf("  - Clearing out Actions\n");
   /* Loop over all Actions and clear their bindings' user cache. */
   LISTBASE_FOREACH (bAction *, dna_action, &bmain.actions) {
+    printf("    - %s\n", dna_action->id.name);
     Action &action = dna_action->wrap();
     for (Binding *binding : action.bindings()) {
       BLI_assert_msg(binding->runtime, "Binding::runtime should always be allocated");
@@ -52,6 +54,7 @@ static void rebuild_binding_user_cache_for_bmain(Main &bmain)
    * duplicating more of that function than I (Sybren) am comfortable with. */
   BindingRuntime::is_users_dirty = false;
 
+  printf("  - Looping over animated IDs.\n");
   /* Loop over all IDs to cache their binding usage. */
   ListBase *ids_of_idtype;
   ID *id;
@@ -72,10 +75,12 @@ static void rebuild_binding_user_cache_for_bmain(Main &bmain)
 
       Binding &binding = *action_binding->second;
       binding.users_add(*id);
+      printf("    - %s → %s / %s\n", id->name, action_binding->first->id.name, binding.name);
     }
     FOREACH_MAIN_LISTBASE_ID_END;
   }
   FOREACH_MAIN_LISTBASE_END;
+  printf("  - Done\n");
 }
 
 void rebuild_binding_user_cache()

I'll also attach an example blend file that uses linking.

Not really, as that works fine. This will give you some debug prints, in case you want to try: ```diff diff --git a/source/blender/animrig/intern/action_runtime.cc b/source/blender/animrig/intern/action_runtime.cc index c06f257f401..ab7567b6181 100644 --- a/source/blender/animrig/intern/action_runtime.cc +++ b/source/blender/animrig/intern/action_runtime.cc @@ -34,8 +34,10 @@ static void rebuild_binding_user_cache_for_bmain(Main &bmain) // Note to reviewers: this is just to make the refresh visible. I'll remove this before landing. printf("rebuild_binding_user_cache_for_bmain()\n"); + printf(" - Clearing out Actions\n"); /* Loop over all Actions and clear their bindings' user cache. */ LISTBASE_FOREACH (bAction *, dna_action, &bmain.actions) { + printf(" - %s\n", dna_action->id.name); Action &action = dna_action->wrap(); for (Binding *binding : action.bindings()) { BLI_assert_msg(binding->runtime, "Binding::runtime should always be allocated"); @@ -52,6 +54,7 @@ static void rebuild_binding_user_cache_for_bmain(Main &bmain) * duplicating more of that function than I (Sybren) am comfortable with. */ BindingRuntime::is_users_dirty = false; + printf(" - Looping over animated IDs.\n"); /* Loop over all IDs to cache their binding usage. */ ListBase *ids_of_idtype; ID *id; @@ -72,10 +75,12 @@ static void rebuild_binding_user_cache_for_bmain(Main &bmain) Binding &binding = *action_binding->second; binding.users_add(*id); + printf(" - %s → %s / %s\n", id->name, action_binding->first->id.name, binding.name); } FOREACH_MAIN_LISTBASE_ID_END; } FOREACH_MAIN_LISTBASE_END; + printf(" - Done\n"); } void rebuild_binding_user_cache() ``` I'll also attach [an example blend file that uses linking](https://projects.blender.org/attachments/da27311e-d454-420e-9e1c-82c88e9d28a6).
dr.sybren marked this conversation as resolved
@ -0,0 +25,4 @@
*
* \note This is NOT thread-safe.
*/
Set<ID *> users;
Member

Same as my other comment: I think we should just use a Vector.

Same as my other comment: I think we should just use a `Vector`.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel added 2 commits 2024-06-14 15:21:46 +02:00
Nathan Vegdahl approved these changes 2024-06-14 15:49:13 +02:00
Nathan Vegdahl left a comment
Member

Had one more comment that I discussed with @dr.sybren in person, regarding making the mutable variant of users() internal-only. But otherwise looks good to me!

Had one more comment that I discussed with @dr.sybren in person, regarding making the mutable variant of `users()` internal-only. But otherwise looks good to me!
Hans Goudey reviewed 2024-06-14 15:55:56 +02:00
@ -475,1 +483,4 @@
/** Return the set of IDs that are animated by this Binding. */
Vector<ID *> &users();
const Vector<ID *> &users() const;
Member

This can return Span<ID *> :)

This can return `Span<ID *>` :)
dr.sybren marked this conversation as resolved
Bastien Montagne requested changes 2024-06-14 16:21:38 +02:00
Dismissed
Bastien Montagne left a comment
Owner

Some minor notes below, have more concerns about things like global variables, but will talk about this IRL first

Some minor notes below, have more concerns about things like global variables, but will talk about this IRL first
@ -473,6 +481,37 @@ class Binding : public ::ActionBinding {
/** Return whether this Binding has an `idtype` set. */
bool has_idtype() const;
/** Return the set of IDs that are animated by this Binding. */

Not a set, but a vector ;)

Not a set, but a vector ;)
Author
Member

The container type is Vector, but it's using set semantics ;-)

The container type is Vector, but it's using set semantics ;-)
dr.sybren marked this conversation as resolved
@ -0,0 +27,4 @@
*
* Note that this is a vector for simplicity, as the majority of the bindings
* will have zero or one user. Semantically it's treated as a set: order
* doesn't matter, and it has no duplicate entires.

entires -> entries

`entires` -> `entries`
dr.sybren marked this conversation as resolved

Talked with Sybren IRL, usage of global Main and 'dirty flag' will be worked on. Also 'runtime editing' of the Binding data when writing on disk will be fixed.

Talked with Sybren IRL, usage of global Main and 'dirty flag' will be worked on. Also 'runtime editing' of the Binding data when writing on disk will be fixed.
Sybren A. Stüvel added 2 commits 2024-06-14 17:27:16 +02:00
Sybren A. Stüvel added 1 commit 2024-06-17 16:08:08 +02:00
Sybren A. Stüvel added 5 commits 2024-06-17 18:00:48 +02:00
Refactor: Anim filtering, avoid passing bDopesheet *ads parameter
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
736d391818
In the animation filtering code, avoid passing `bDopesheet *ads` as a
separate parameter. Instead, pass `bAnimContext *ac`. This already
contains the same pointer. Some carefully placed asserts help to verify
that these pointers were indeed the same.

Not only do I think this approach is cleaner, it is a necessary step for
upcoming PRs #123187 and #122672. Those will need a `Main *bmain`
pointer, which is already available in the `bAnimContext` struct.

No functional changes.
The only use of `G_MAIN` is in `BKE_animdata_free()`. I couldn't find a
clean place to otherwise clear this flag.
Author
Member

Thanks @mont29. I've addressed the review comments. The only place where I couldn't really find a clean way to avoid G_MAIN was in BKE_animdata_free(). It's called in very generic places where there's also no bmain available. No longer accurate, see comment below :)


Old ponderings:

An alternative would be to add a call to animrig::Binding::users_invalidate(*bmain); in id_free() (lib_id_delete.cc). To me that looked a bit too specific for such a generic function, though.

Another alternative could be to pass the bmain pointer to BKE_libblock_free_datablock(), which then can pass it to idtype_info->free_data(id);.

And a third alternative would be a more elaborate change. Currently BKE_animdata_free() is typically called from the IDType callback. So every ID type has to 'know' whether it's animatable or not, and has to free its animdata. We could lift this to a more generic place, and then we won't have to pass the bmain pointer to everything that might need to free an ID.

Thanks @mont29. I've addressed the review comments. ~~The only place where I couldn't really find a clean way to avoid `G_MAIN` was in `BKE_animdata_free()`. It's called in very generic places where there's also no `bmain` available.~~ No longer accurate, see comment below :) ----------------- Old ponderings: An alternative would be to add a call to `animrig::Binding::users_invalidate(*bmain);` in `id_free()` (`lib_id_delete.cc`). To me that looked a bit too specific for such a generic function, though. Another alternative could be to pass the `bmain` pointer to `BKE_libblock_free_datablock()`, which then can pass it to `idtype_info->free_data(id);`. And a third alternative would be a more elaborate change. Currently `BKE_animdata_free()` is typically called from the IDType callback. So every ID type has to 'know' whether it's animatable or not, and has to free its animdata. We could lift this to a more generic place, and then we won't have to pass the `bmain` pointer to everything that might need to free an ID.
Sybren A. Stüvel added 1 commit 2024-06-17 18:12:12 +02:00
entires → entries
Some checks failed
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
af7c4bbfa3
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel added 1 commit 2024-06-18 10:38:02 +02:00
Merge remote-tracking branch 'origin/main' into anim/baklava-bindings-to-id-cache
Some checks failed
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
589b701eac
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel added 1 commit 2024-06-18 13:41:28 +02:00
Merge branch 'main' into anim/baklava-bindings-to-id-cache
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
6e80fb0fe8
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel added 1 commit 2024-06-18 14:12:01 +02:00
Sybren A. Stüvel added 2 commits 2024-06-18 14:44:36 +02:00
Sybren A. Stüvel added 1 commit 2024-06-18 14:47:43 +02:00
Hide more behind #ifdef WITH_ANIM_BAKLAVA
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
63e484da3c
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel force-pushed anim/baklava-bindings-to-id-cache from 0e321073c0 to d017821be1 2024-06-18 15:26:34 +02:00 Compare
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel requested review from Bastien Montagne 2024-06-18 15:56:12 +02:00
Sybren A. Stüvel added 1 commit 2024-06-19 10:23:15 +02:00
Author
Member

@mont29 I've removed the final use of G_MAIN from the code :)

@mont29 I've removed the final use of `G_MAIN` from the code :)
Sybren A. Stüvel added 1 commit 2024-06-19 10:25:11 +02:00
Remove unintended formatting change
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
95f4ca407b
Author
Member

@blender-bot build

@blender-bot build
Bastien Montagne approved these changes 2024-06-20 15:43:32 +02:00
Dismissed
Bastien Montagne left a comment
Owner

Moving cacheclear to the foreach_id code is likely a good idea, but doing it systematically does not sounds right. Think we need to discuss this again IRL asap. :)

Moving cacheclear to the `foreach_id` code is likely a good idea, but doing it systematically does not sounds right. Think we need to discuss this again IRL asap. :)
@ -218,0 +223,4 @@
* that may force a `return` from this function.
*
* IDWALK_RECURSE implies IDWALK_READONLY so we have to check for both. */
const bool is_readonly = flag & (IDWALK_READONLY | IDWALK_RECURSE);

There is no need to check for IDWALK_RECURSE here, background code in lib_query ensures that IDWALK_READONLY is set when caller sets IDWALK_RECURSE.

There is no need to check for `IDWALK_RECURSE` here, background code in `lib_query` ensures that `IDWALK_READONLY` is set when caller sets `IDWALK_RECURSE`.
dr.sybren marked this conversation as resolved
@ -218,0 +224,4 @@
*
* IDWALK_RECURSE implies IDWALK_READONLY so we have to check for both. */
const bool is_readonly = flag & (IDWALK_READONLY | IDWALK_RECURSE);
if (!is_readonly) {

This whole cache clear should not happen systematically in non-readonly case. this should only happen when a binding-affecting pointer is actually remapped. Which I'm not even sure is actually happening currently, looking at code of action_foreach_id? Need to talk to you again asap I guess.

Note that macros like BKE_LIB_FOREACHID_PROCESS_FUNCTION_CALL and such are only there to make code easy and less verbose, when needed the actual underlying functions can be called explicitly instead, to avoid automatic return when further processing is needed (see e.g. handling of active workspace in window_manager_foreach_id)

This whole cache clear should not happen systematically in non-readonly case. this should only happen when a binding-affecting pointer is actually remapped. Which I'm not even sure is actually happening currently, looking at code of `action_foreach_id`? Need to talk to you again asap I guess. Note that macros like `BKE_LIB_FOREACHID_PROCESS_FUNCTION_CALL` and such are only there to make code easy and less verbose, when needed the actual underlying functions can be called explicitly instead, to avoid automatic return when further processing is needed (see e.g. handling of active workspace in `window_manager_foreach_id`)
dr.sybren marked this conversation as resolved
@ -218,0 +230,4 @@
*
* Since this code is to handle the ID remapping scenario, and all calls to
* `BKE_library_foreach_ID_link()` in `blendfile.cc` pass a `bmain` pointer, I (Sybren) hope we
* can assume that in this case the `bmain` pointer is not NULL, and thus we can ignore cases

Assuming cache clear can be ignored when bmain is null should be valid indeed, this is not expected to happen when dealing with data in Main (either G_MAIN or a temp main) typically.

Assuming cache clear can be ignored when `bmain` is null should be valid indeed, this is not expected to happen when dealing with data in Main (either `G_MAIN` or a temp main) typically.
dr.sybren marked this conversation as resolved
Bastien Montagne requested changes 2024-06-20 15:44:14 +02:00
Dismissed
Bastien Montagne left a comment
Owner

Grrr, clicked on approve by mistake.

Grrr, clicked on approve by mistake.
Author
Member

Thanks for the feedback @mont29 , let's talk when I'm back in the office!

Thanks for the feedback @mont29 , let's talk when I'm back in the office!
Sybren A. Stüvel added 4 commits 2024-06-26 16:55:52 +02:00
Sybren A. Stüvel added 1 commit 2024-06-26 17:01:37 +02:00
Author
Member

@blender-bot build

@blender-bot build
Sybren A. Stüvel requested review from Bastien Montagne 2024-06-26 17:08:30 +02:00
Author
Member

@blender-bot build

@blender-bot build
Bastien Montagne requested changes 2024-06-28 12:08:19 +02:00
Dismissed
Bastien Montagne left a comment
Owner

This looks pretty good now, mainly details noted in comments below.

Note that I'd also like to have the 'should have no effect' indirect changes in lib_query.cc, lib_remapp.cc and readfile.cc committed separately. These should not have any effect on existing code - better have an easy-to-bisect commit to find out if they do have unexpected side-effects.

This looks pretty good now, mainly details noted in comments below. Note that I'd also like to have the 'should have no effect' indirect changes in `lib_query.cc`, `lib_remapp.cc` and `readfile.cc` committed separately. These should not have any effect on existing code - better have an easy-to-bisect commit to find out if they do have unexpected side-effects.
@ -214,2 +214,4 @@
{
animrig::Action &action = reinterpret_cast<bAction *>(id)->wrap();
/* This function is called when Blender is remapping IDs. With such use, the IDWALK_READONLY flag

I would not talk about remapping specifically here (only notice it as the main example of non-readonly processes that may change ID pointers). Any call which does not specify IDWALK_READONLY can change ID pointers.

I would not talk about remapping specifically here (only notice it as the main example of non-readonly processes that may change ID pointers). Any call which does not specify `IDWALK_READONLY` can change ID pointers.
dr.sybren marked this conversation as resolved
@ -216,0 +217,4 @@
/* This function is called when Blender is remapping IDs. With such use, the IDWALK_READONLY flag
* will be cleared.
*
* Rempaping may change the binding-to-ID pointers, but that change is not guaranteed to be

Typo: Remapping

Typo: `Remapping`
dr.sybren marked this conversation as resolved
@ -216,0 +222,4 @@
* neither means that the new Mesh is animated with the same Action, nor that the old Mesh is no
* longer animated by that Action. In other words, the best that can be done is to invalidate the
* cache.
*

About the block below (thanks gitea for not allowing new comments on an already commented line...):

I pushed recently a cleanup/clarification commit (a50ab48709), which clearly states that any form of 'early break' is only allowed in readonly case. So I think this can be simplified into something like that:

 * NOTE: early-returns are forbidden in non-readonly cases (see #IDWALK_RET_STOP_ITER documentation).
About the block below _(thanks gitea for not allowing new comments on an already commented line...)_: I pushed recently a cleanup/clarification commit (a50ab48709), which clearly states that any form of 'early break' is only allowed in readonly case. So I think this can be simplified into something like that: ``` * NOTE: early-returns are forbidden in non-readonly cases (see #IDWALK_RET_STOP_ITER documentation). ```
dr.sybren marked this conversation as resolved
@ -216,1 +229,4 @@
const int flag = BKE_lib_query_foreachid_process_flags_get(data);
const bool is_readonly = flag & IDWALK_READONLY;
const int idwalk_flags = IDWALK_CB_NEVER_SELF | IDWALK_CB_LOOPBACK;

could be a constexpr

could be a `constexpr`
dr.sybren marked this conversation as resolved
@ -444,3 +506,3 @@
/*id_code*/ ID_AC,
/*id_filter*/ FILTER_ID_AC,
/*dependencies_id_types*/ 0,
/*dependencies_id_types*/ FILTER_ID_ALL,

It would be good to get the filter_id_animatable thing we discussed the other day addressed asap? Not blocking for this patch, but would rather avoid usage of FILTER_ID_ALL in IDTypeInfo as much as possible...

It would be good to get the `filter_id_animatable` thing we discussed the other day addressed asap? Not blocking for this patch, but would rather avoid usage of `FILTER_ID_ALL` in IDTypeInfo as much as possible...
Author
Member

Note that I'd also like to have the 'should have no effect' indirect changes in lib_query.cc, lib_remapp.cc and readfile.cc committed separately. These should not have any effect on existing code - better have an easy-to-bisect commit to find out if they do have unexpected side-effects.

I'll split those out into separate commits, then do a force-push so this PR actually reflects those two commits.

> Note that I'd also like to have the 'should have no effect' indirect changes in `lib_query.cc`, `lib_remapp.cc` and `readfile.cc` committed separately. These should not have any effect on existing code - better have an easy-to-bisect commit to find out if they do have unexpected side-effects. I'll split those out into separate commits, then do a force-push so this PR actually reflects those two commits.
Sybren A. Stüvel force-pushed anim/baklava-bindings-to-id-cache from f6d0f805e6 to 140dc672bf 2024-06-28 12:35:31 +02:00 Compare
Author
Member

I've split up the PR into three commits, and removed the rna_ActionBinding_debug_log_users() function we used for debugging.

I've split up the PR into three commits, and removed the `rna_ActionBinding_debug_log_users()` function we used for debugging.
Sybren A. Stüvel requested review from Bastien Montagne 2024-06-28 12:36:21 +02:00
Author
Member

@blender-bot build

@blender-bot build
Bastien Montagne approved these changes 2024-06-28 13:11:50 +02:00
Bastien Montagne left a comment
Owner

Thanks, LGTM

Thanks, LGTM
Sybren A. Stüvel merged commit c4a7c4e2a1 into main 2024-06-28 14:41:43 +02:00
Sybren A. Stüvel deleted branch anim/baklava-bindings-to-id-cache 2024-06-28 14:41:47 +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
5 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#123187
No description provided.