Bone selection sets not saving #94775

New Issue

Michael Jasoni · 2022-01-09T23:05:24+01:00

Michael Jasoni commented

2022-01-09 23:05:24 +01:00

System Information
Operating system: Windows-10-10.0.19044-SP0 64 Bits
Graphics card: NVIDIA GeForce GTX 1070/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 497.29

Blender Version
Broken: version: 3.0.0, branch: master, commit date: 2021-12-02 18:35, hash: blender/blender@f1cca30557
Worked: (newest version of Blender that worked as expected)

Addon Information
Name: Bone Selection Sets (2, 1, 1)
Author: Inês Almeida, Sybren A. Stüvel, Antony Riakiotakis, Dan Eicher

Short description of error
Blender is not saving any selection sets beyond the first set that I create. When I reopen the file, all selection sets are a duplicate of the first one created.

Exact steps for others to reproduce the error

Link an armature in a file and create a library override on the armature.
With the armature selected in post mode, create a selection set and add some bones to it.
Create an additional selection set and add different bones to it.
Save the file, close and reopen.
Both selection sets created are named the same and contain the bones added to the first set created.

**System Information** Operating system: Windows-10-10.0.19044-SP0 64 Bits Graphics card: NVIDIA GeForce GTX 1070/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 497.29 **Blender Version** Broken: version: 3.0.0, branch: master, commit date: 2021-12-02 18:35, hash: `blender/blender@f1cca30557` Worked: (newest version of Blender that worked as expected) **Addon Information** Name: Bone Selection Sets (2, 1, 1) Author: Inês Almeida, Sybren A. Stüvel, Antony Riakiotakis, Dan Eicher **Short description of error** Blender is not saving any selection sets beyond the first set that I create. When I reopen the file, all selection sets are a duplicate of the first one created. **Exact steps for others to reproduce the error** 1. Link an armature in a file and create a library override on the armature. 2. With the armature selected in post mode, create a selection set and add some bones to it. 2. Create an additional selection set and add different bones to it. 3. Save the file, close and reopen. 4. Both selection sets created are named the same and contain the bones added to the first set created.

Michael Jasoni commented

2022-01-09 23:05:24 +01:00

Added subscriber: @MAIGULL

blender-admin commented

2022-01-09 23:05:24 +01:00

#97109 was marked as duplicate of this issue

blender-admin commented

2022-01-09 23:05:24 +01:00

#96079 was marked as duplicate of this issue

Germano Cavalcante commented

2022-03-07 18:55:51 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Germano Cavalcante commented

2022-03-07 18:56:11 +01:00

Added subscribers: @BeornLeonard, @mano-wii, @BClark

Wayde Moss commented

2022-03-26 16:49:58 +01:00

Added subscribers: @mont29, @wbmoss_dev

Wayde Moss commented

2022-03-26 16:49:58 +01:00

@mont29 This diff fixes the issue but I don't know the library override code base well enough to know if everything is alright. Feel free to commit if its good enough.

The change at line 1932 for equals = equals && diff == 0; doesn't actually fix anything related to the task but seemed like a typo based on other uses of the *_diff() function return values. I have no idea what it affects. The rest of the patch does fix the main task issues. I'm uncertain because I don't know why there are originally +1 index offsets on non-reference indices and why there is a double lookup for the same source item.

diff --git a/source/blender/makesrna/intern/rna_rna.c b/source/blender/makesrna/intern/rna_rna.c
index 8f1847c00f4..7311249122e 100644
--- a/source/blender/makesrna/intern/rna_rna.c
+++ b/source/blender/makesrna/intern/rna_rna.c
@@ -1932,25 +1932,25 @@ int rna_property_override_diff_default(Main *bmain,
           }
           else if (is_id || is_valid_for_diffing) {
             if (equals || do_create) {
-              const int eq = rna_property_override_diff_propptr(bmain,
-                                                                ptr_a->owner_id,
-                                                                ptr_b->owner_id,
-                                                                &iter_a.ptr,
-                                                                &iter_b.ptr,
-                                                                mode,
-                                                                no_ownership,
-                                                                no_prop_name,
-                                                                override,
-                                                                rna_path,
-                                                                rna_path_len,
-                                                                PROP_COLLECTION,
-                                                                propname_a,
-                                                                propname_b,
-                                                                idx_a,
-                                                                idx_b,
-                                                                flags,
-                                                                r_override_changed);
-              equals = equals && eq;
+              const int diff = rna_property_override_diff_propptr(bmain,
+                                                                  ptr_a->owner_id,
+                                                                  ptr_b->owner_id,
+                                                                  &iter_a.ptr,
+                                                                  &iter_b.ptr,
+                                                                  mode,
+                                                                  no_ownership,
+                                                                  no_prop_name,
+                                                                  override,
+                                                                  rna_path,
+                                                                  rna_path_len,
+                                                                  PROP_COLLECTION,
+                                                                  propname_a,
+                                                                  propname_b,
+                                                                  idx_a,
+                                                                  idx_b,
+                                                                  flags,
+                                                                  r_override_changed);
+              equals = equals && diff == 0;
             }
           }
 
@@ -2592,12 +2592,16 @@ bool rna_property_override_apply_default(Main *UNUSED(bmain),
           if (opop->subitem_local_name != NULL && opop->subitem_local_name[0] != '\0') {
             /* Find from name. */
             int item_index_src, item_index_ref;
-            if (RNA_property_collection_lookup_string_index(
-                    ptr_src, prop_src, opop->subitem_local_name, &item_ptr_src, &item_index_src) &&
-                RNA_property_collection_lookup_int(
-                    ptr_src, prop_src, item_index_src + 1, &item_ptr_src) &&
-                RNA_property_collection_lookup_string_index(
-                    ptr_dst, prop_dst, opop->subitem_local_name, &item_ptr_ref, &item_index_ref)) {
+            if (RNA_property_collection_lookup_string_index(ptr_src,
+                                                             prop_src,
+                                                             opop->subitem_local_name,
+                                                             &item_ptr_src,
+                                                             &item_index_src) &&
+                RNA_property_collection_lookup_string_index(ptr_dst,
+                                                            prop_dst,
+                                                            opop->subitem_reference_name,
+                                                            &item_ptr_ref,
+                                                            &item_index_ref)) {
               is_valid = true;
               item_index_dst = item_index_ref + 1;
             }
@@ -2605,10 +2609,10 @@ bool rna_property_override_apply_default(Main *UNUSED(bmain),
           if (!is_valid && opop->subitem_local_index >= 0) {
             /* Find from index. */
             if (RNA_property_collection_lookup_int(
-                    ptr_src, prop_src, opop->subitem_local_index + 1, &item_ptr_src) &&
+                    ptr_src, prop_src, opop->subitem_local_index, &item_ptr_src) &&
                 RNA_property_collection_lookup_int(
-                    ptr_dst, prop_dst, opop->subitem_local_index, &item_ptr_ref)) {
-              item_index_dst = opop->subitem_local_index + 1;
+                    ptr_dst, prop_dst, opop->subitem_reference_index, &item_ptr_ref)) {
+              item_index_dst = opop->subitem_reference_index + 1;
               is_valid = true;
             }
           }

@mont29 This diff fixes the issue but I don't know the library override code base well enough to know if everything is alright. Feel free to commit if its good enough. The change at line 1932 for ` equals = equals && diff == 0;` doesn't actually fix anything related to the task but seemed like a typo based on other uses of the *_diff() function return values. I have no idea what it affects. The rest of the patch does fix the main task issues. I'm uncertain because I don't know why there are originally +1 index offsets on non-reference indices and why there is a double lookup for the same source item. ``` diff --git a/source/blender/makesrna/intern/rna_rna.c b/source/blender/makesrna/intern/rna_rna.c index 8f1847c00f4..7311249122e 100644 --- a/source/blender/makesrna/intern/rna_rna.c +++ b/source/blender/makesrna/intern/rna_rna.c @@ -1932,25 +1932,25 @@ int rna_property_override_diff_default(Main *bmain, } else if (is_id || is_valid_for_diffing) { if (equals || do_create) { - const int eq = rna_property_override_diff_propptr(bmain, - ptr_a->owner_id, - ptr_b->owner_id, - &iter_a.ptr, - &iter_b.ptr, - mode, - no_ownership, - no_prop_name, - override, - rna_path, - rna_path_len, - PROP_COLLECTION, - propname_a, - propname_b, - idx_a, - idx_b, - flags, - r_override_changed); - equals = equals && eq; + const int diff = rna_property_override_diff_propptr(bmain, + ptr_a->owner_id, + ptr_b->owner_id, + &iter_a.ptr, + &iter_b.ptr, + mode, + no_ownership, + no_prop_name, + override, + rna_path, + rna_path_len, + PROP_COLLECTION, + propname_a, + propname_b, + idx_a, + idx_b, + flags, + r_override_changed); + equals = equals && diff == 0; } } @@ -2592,12 +2592,16 @@ bool rna_property_override_apply_default(Main *UNUSED(bmain), if (opop->subitem_local_name != NULL && opop->subitem_local_name[0] != '\0') { /* Find from name. */ int item_index_src, item_index_ref; - if (RNA_property_collection_lookup_string_index( - ptr_src, prop_src, opop->subitem_local_name, &item_ptr_src, &item_index_src) && - RNA_property_collection_lookup_int( - ptr_src, prop_src, item_index_src + 1, &item_ptr_src) && - RNA_property_collection_lookup_string_index( - ptr_dst, prop_dst, opop->subitem_local_name, &item_ptr_ref, &item_index_ref)) { + if (RNA_property_collection_lookup_string_index(ptr_src, + prop_src, + opop->subitem_local_name, + &item_ptr_src, + &item_index_src) && + RNA_property_collection_lookup_string_index(ptr_dst, + prop_dst, + opop->subitem_reference_name, + &item_ptr_ref, + &item_index_ref)) { is_valid = true; item_index_dst = item_index_ref + 1; } @@ -2605,10 +2609,10 @@ bool rna_property_override_apply_default(Main *UNUSED(bmain), if (!is_valid && opop->subitem_local_index >= 0) { /* Find from index. */ if (RNA_property_collection_lookup_int( - ptr_src, prop_src, opop->subitem_local_index + 1, &item_ptr_src) && + ptr_src, prop_src, opop->subitem_local_index, &item_ptr_src) && RNA_property_collection_lookup_int( - ptr_dst, prop_dst, opop->subitem_local_index, &item_ptr_ref)) { - item_index_dst = opop->subitem_local_index + 1; + ptr_dst, prop_dst, opop->subitem_reference_index, &item_ptr_ref)) { + item_index_dst = opop->subitem_reference_index + 1; is_valid = true; } } ```

Sybren A. Stüvel commented

2022-03-28 11:59:12 +02:00

Added subscriber: @dr.sybren

Sybren A. Stüvel commented

2022-03-28 11:59:12 +02:00

In #94775#1330407, @wbmoss_dev wrote:
@mont29 This diff fixes the issue but I don't know the library override code base well enough to know if everything is alright. Feel free to commit if its good enough.

The change at line 1932 for equals = equals && diff == 0; doesn't actually fix anything related to the task but seemed like a typo based on other uses of the *_diff() function return values. I have no idea what it affects. The rest of the patch does fix the main task issues. I'm uncertain because I don't know why there are originally +1 index offsets on non-reference indices and why there is a double lookup for the same source item.

rna_property_override_diff_propptr() is an overly complex function without documenting its return value, so it's understandable to get confused. From what I can see, it should have been declared as returning bool, as it seems to just return "is different".

Instead of equals = equals && diff == 0; I would even just write equals &= (diff == 0); for clarity.

As for the rest, I can't help you here :/

> In #94775#1330407, @wbmoss_dev wrote: > @mont29 This diff fixes the issue but I don't know the library override code base well enough to know if everything is alright. Feel free to commit if its good enough. > > The change at line 1932 for ` equals = equals && diff == 0;` doesn't actually fix anything related to the task but seemed like a typo based on other uses of the *_diff() function return values. I have no idea what it affects. The rest of the patch does fix the main task issues. I'm uncertain because I don't know why there are originally +1 index offsets on non-reference indices and why there is a double lookup for the same source item. `rna_property_override_diff_propptr()` is an overly complex function without documenting its return value, so it's understandable to get confused. From what I can see, it should have been declared as returning `bool`, as it seems to just return "is different". Instead of `equals = equals && diff == 0;` I would even just write `equals &= (diff == 0);` for clarity. As for the rest, I can't help you here :/

blender-admin commented

2022-04-20 11:05:54 +02:00

This issue was referenced by blender/blender@5b8a3ccd37

This issue was referenced by blender/blender@5b8a3ccd374913d9f81db9e01f64fd51f1296582

Bastien Montagne commented

2022-04-20 11:17:59 +02:00

@wbmoss_dev thanks, your fix is correct (for some reason, this default apply code was not updated when ways insertion in collections was reworked in rBrB33c5e7bcd5e5 ...).