Remove ID refcounting handling from readfile.c #73630

New Issue

Bastien Montagne · 2020-02-06T10:49:09+01:00

Bastien Montagne commented

2020-02-06 10:49:09 +01:00

Over the years we manage, using library_query API, to remove massive/generic ID refcounting management from almost everywhere in Blender, except for readfile.c

Having that logic (which pointer is refcounting and which is not) in more than one place has been a nice source of bugs over the years, and we still have some inconsistencies in a few places (as e.g. ID pointers used by editors are not handled together with others, but have their own callback system...).

In readfile-rework-refcount-handling branch I started some WIP work towards that, but the Editors pointers especially are being an issue there, so would rather get some consensus on general plan below before going forward:

Move ID management from `<editor>_id_remap()` callbacks to libquery code (we'd still need those callbacks for extra updates needed when remapping/removing IDs).

** Probably make handling of editors' ID pointers optional? Not sure there are cases where we do not want them handled though...

Remove any use of `newlibadr_us()` and `newlibadr_real_us()` and use instead a single call to `BKE_main_id_refcount_recompute()` just after `lib_link_all()`.

** Only question here would be for deprecated ID pointers (old IPO thingy mostly), but I think it should not matter since they are supposed to be doversion'ed and freed anyway?

Notes:

Would not touch to blo_lib_link_restore() for now... This one is dealing with UI ID pointers.
We currently have a potentially broken situation with undo already calling BKE_main_id_refcount_recompute() on linked data, which would break e.g. usercount of linked GreasePencil ID's used by some editors...

*Note: Added 'UI' ID pointers handling to libquery in libquery-handle-ui-pointers branch now.//

Over the years we manage, using library_query API, to remove massive/generic ID refcounting management from almost everywhere in Blender, except for `readfile.c` Having that logic (which pointer is refcounting and which is not) in more than one place has been a nice source of bugs over the years, and we still have some inconsistencies in a few places (as e.g. ID pointers used by editors are not handled together with others, but have their own callback system...). In [readfile-rework-refcount-handling](https://developer.blender.org/diffusion/B/history/readfile-rework-refcount-handling/) branch I started some WIP work towards that, but the Editors pointers especially are being an issue there, so would rather get some consensus on general plan below before going forward: # Move ID management from `<editor>_id_remap()` callbacks to libquery code (we'd still need those callbacks for extra updates needed when remapping/removing IDs). ** Probably make handling of editors' ID pointers optional? Not sure there are cases where we do not want them handled though... # Remove any use of `newlibadr_us()` and `newlibadr_real_us()` and use instead a single call to `BKE_main_id_refcount_recompute()` just after `lib_link_all()`. ** Only question here would be for deprecated ID pointers (old IPO thingy mostly), but I think it should not matter since they are supposed to be doversion'ed and freed anyway? Notes: - Would not touch to `blo_lib_link_restore()` for now... This one is dealing with UI ID pointers. - We currently have a potentially broken situation with undo already calling `BKE_main_id_refcount_recompute()` on linked data, which would break e.g. usercount of linked GreasePencil ID's used by some editors... *Note: Added 'UI' ID pointers handling to libquery in [libquery-handle-ui-pointers](https:*developer.blender.org/diffusion/B/history/libquery-handle-ui-pointers/) branch now.//

Bastien Montagne commented

2020-02-06 10:49:09 +01:00

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