Datablock ID Properties
Needs ReviewPublic

Authored by Alexander Romanov (a.romanov) on Dec 16 2013, 7:36 PM.
Tags
None
Tokens
"Pterodactyl" token, awarded by AlexKowel."Love" token, awarded by Ikyuv."The World Burns" token, awarded by cardboard."Like" token, awarded by BYOB."Love" token, awarded by a.romanov."Like" token, awarded by disnel."Love" token, awarded by bliblubli.

Details

Summary

The absence of datablock properties "will certainly be resolved soon as the need for them is becoming obvious" said the Python Nodes release notes. So this patch allows Python scripts to create ID Properties which reference datablocks.
This functionality is implemented for PointerProperty and now such properties can be created with Python.

In addition to the standard update callback, PointerProperty can have a poll callback (standard RNA) which is useful for search menus. For details see the test included in this patch.

Original author: @Tom Edwards (artfunkel)

Alexander (Blend4Web Team)

Diff Detail

There are a very large number of changes, so older changes are hidden. Show Older Changes
source/blender/blenkernel/BKE_library_query.h
85

Should be renamed to BKE_library_id_can_use_idtype

source/blender/blenkernel/intern/idprop.c
457–458

Hrmmm… Why exactly do you need a lock here? What are the concurrent access cases? I would expect IDProp to always be only accessed (even more edited) from main thread?

And! Why use a global storage for that mapping? Couldn’t it fit much better in Main struct e.g.? Having a global var for something that is totally related to a given file and its datablocks looks odd and ugly to me…

528

Please do not use CamelCase-like names, IDP_ID_Unregister -> IDP_id_unregister, same for all funcs below.

source/blender/blenkernel/intern/library_query.c
113

Please do not do that kind of changes, especially not in a patch, adds useless noise.

314

No, there is a very good reason not to include last instruction semi-colon in macros, again please refrain from doing that kind of unrelated changes in patches anyway.

936–938

That kind of security check shall not be needed at that level of code.

946–948

This should be replaced by actual check whether id_owner do has actions, or not. ;)

950

Please elaborate, explaining why we need to shortcut those ID types (bNode->id can have virtually any type, and IDProps of bones for armatures).

951

id_type_owner, not id_owner! ;)

958–963

You do not need to handle nodetrees that way, it’s verbose and not particularly efficient.

You should rather add a generic check on all IDs at the beginning of this function, something like that:

if (ntreeFromID(id_owner)) {
    return true;
}

Same goes for all other cases below (materials, etc.).

1051–1089

Why the heck are you moving this here? Please keep all those sub-loopers utils *before* BKE_library_foreach_ID_link() definition. Should also avoids you to have to add forward declarations of functions...

1079

eeek! this has to be at the end of the function (typically last line of func definition)!

source/blender/blenkernel/intern/library_remap.c
446

Can be moved inside for loop now (as init part).

689

This actually won’t work, this is a callback called for *all* ID pointers found by BKE_library_foreach_ID_link

Think that is call is not needed at all, in fact (at least, should not be needed, if there was no global ghash register of Hell). Otherwise, should happen in BKE_libblock_relink_to_newid, not here, imho.

850–851

This should be inside the if (do_id_user) { check at start of the function. And again, only useful because of global ghash register of Hell, think we can get rid of this altogether.

source/blender/blenloader/intern/readfile.c
2228

This should be moved in a (new) generic static void lib_link_id(FileData *fd, ID *id) function called on all IDs, avoids manually calling it in all id-type specific liblinking func...

9106–9109

Hrmmm… do we really want to also allow node sockets to have ID IDProps? is there any usecase for that?

source/blender/editors/interface/interface_regions.c
1453

should be 2, not 3 (only two chars are used for IDtype code, a short).

source/blender/editors/object/object_relations.c
1933–1934

Again, there should not be any need for special handling here, generic BKE_libblock_relink_to_newid() should do the work imho.

From a design point of view, will it allow to link/append node trees from addons (like Luxblend for Luxcore or animation nodes) with needed textures, meshes and whatever can be referenced in a node tree?

So, besides bunch of comments below, I have two 'high-level' questions.

I) Why IDProp ID usages are not user-counted?
==================================

Can’t see any pros for that, and at least a big cons: if user only uses a given data-block in IDProps, then said data-block won't be saved and will vanish in thin air…

II) Why do we need the ID register (IDP_IDHashTable) at all?
============================================

Afaict, it’s only their to allow finding all IDProp using a given ID. But do we need that? BKE_library_foreach_ID_link and friends should be everything we need here, why make exceptions for IDProps ID usages?

And imho, getting rid of this ghash would seriously simplify handling of ID IDProps, you could get rid of IDP_RelinkProperty, IDP_UnlinkIDProperty, etc.

On users not being counted, I'll quote the original patch notes:

We can't assume that the relevant script will be running to tell Blender what to do, so it's either on all the time or off all the time.
IMO off is the sensible choice: all ID types already have an established means to gain users, and it would be irritating if an obscure ID Prop somewhere made other IDs hang around forever.

The hash table was added following performance concerns from both Campbell and Brecht, particularly concerning the time required to loop over all properties and remove references to an ID that is being deleted.

From a design point of view, will it allow to link/append node trees from addons (like Luxblend for Luxcore or animation nodes) with needed textures, meshes and whatever can be referenced in a node tree?

This does not have anything to do with nodetrees really, those are not IDProperties, but plain data-blocks.

On users not being counted, I'll quote the original patch notes:

OK… I still think we do not user-count enough in Blender, especially now that we can easily and properly delete a data-block even if still used. But this is not a crucial question, we can always change that behavior later if we want to.

The hash table was added following performance concerns from both Campbell and Brecht, particularly concerning the time required to loop over all properties and remove references to an ID that is being deleted.

Yeah… Thing is, we are now already scanning the whole Main database for ID un-/re-linking/mapping, including IDProps in this patch, so having a special handling for IDProps is kind of defeated - and it breaks again generic aspect of BKE's library_… code.

Performances is already a problem in master, not really for deletion, but for more complex tasks like make_library_local. We need a more efficient way to find what are the users of a given ID, but this has to be done globally in BKE_library area, I’d really like to avoid specific handling of sub-areas. We are (painfully) getting out of such situation in ID management code, it proved to be unmanageable (with bunch of outdated/partially implemented/missing implementation pieces of code scattered all over BKE files), so would really, really not want to fall back into that in new code.

Bastien Montagne (mont29) requested changes to this revision.Dec 28 2016, 12:36 PM
This revision now requires changes to proceed.Dec 28 2016, 12:36 PM
Alexander Romanov (a.romanov) updated this object.
Alexander Romanov (a.romanov) marked 15 inline comments as done.

Changes:

  1. Added user counting
  2. Made some changes in accordance with @Bastien Montagne (mont29) comments

Left as is:

  1. Spinlock for the HashTable
  2. HashTable
source/blender/blenkernel/intern/idprop.c
457–458

It is not totally related to a given file, for example we can point to some linked object, and it will become a key in the hash map. If it is necessary, I can move it into main.
Now hashmap is useful in datablock releasing (see IDP_unlink_idlinks call). So I left it.

source/blender/blenkernel/intern/library_query.c
946–948

The following condition is sufficient? adt->action || adt->drivers

source/blender/blenkernel/intern/library_remap.c
850–851

Can't move it now, because of crash in datablock release code. If id is already freed, crash happens in id_us_min called from IDP_id_unregister for some other datablock which uses the first one.

source/blender/blenloader/intern/readfile.c
9106–9109

Yes, I think so. For example, it is useful for default value of socket with some ID type (Object, Texture etc). If socket is not linked we can allow to set such ID-stuff through GUI.

source/blender/editors/interface/interface_regions.c
1453

Found out this here: BKE_id_ui_prefix, see strcpy(name + 3, id->name + 2);

I would still get rid of that IDProp hash. Said it already in previous comment, but IDProps are not the only issue in that case, all ID usages are a problem since we have to loop over all of the IDs to check which one uses which one currently.

Note that this is not an issue for single operations (overhead is not really sensible in that case), but when you need to remove or remap a lot of data-blocks at once. Am thinking about a global ghash of all ID usages generated on-demand in that case, think it will be much simpler and much less heavy to manage than trying to keep a permanent mapping (and more useful, since it will be able to cover all usage cases).

But this is other topic really, again this shall not be handled at IDProp level imho.

source/blender/blenkernel/intern/library_query.c
946–948

I would rather just check whether the result from BKE_animdata_from_id() is NULL or not. Think we can assume that if an ID has anim data, it is susceptible to use any kind of ID pointer.

source/blender/blenloader/intern/readfile.c
9106–9109

Ok… It’s just that this can potentially become a huge time sink in ID management code, one ID pointer per node can already leads to a lot of work, now potentially one or more per socket… Guess users/dev will have to remain reasonable :P

source/blender/editors/interface/interface_regions.c
1453

Ugh… yes indeed, here we are working with 'UI' version of ID code prefix, with additional underscore… My mistake.

Updated to the current master state

minor fix: replace id_type_can_have_animdata -> BKE_animdata_from_id in BKE_library_id_can_use_idtype

Changes:

  1. The HashTable has been removed
  2. Scene copying has been improved

Known issues:
HashTable removal causes issues (crashes) with gui idproperty pointers. Probable reason is that some gui elements have it's own pointer properties which are not remaped e.g. after undo.

Some quick notes:

  • You can this as targets for 2.79, I think it’s ready-enough to be a target.
  • Restoring UI ID pointers after undo/redo is handled in blenloader's readfile.c, in blo_lib_link_screen_restore(). Not sure this is the best place, though, still need to reproduce and investigate properly the issue.

My previous thoughts were not correct. The crash was during scene switch with TemplateID after undo. This happened in RNA_property_pointer_set where RNA and IDProp were set simultaneously. This led to a bad pointer in C.screen["scene"] after undo. At the same time C.screen.scene was restored correctly.

So, I think, now the patch mach more ready for some use cases in which all developers are interested.

Bastien Montagne (mont29) requested changes to this revision.Sat, Mar 18, 4:57 PM

Mostly cleanup notes below, can you publish a public branch on blender repo so that we can work on finalizing it? Will be simpler to address directly minor details in code, rather than reporting them here.

Also, main problems still not addressed:

  • BKE_library_foreach_ID_link still does not handles all non-ID IDProps (you did it for nodes, but not for nodes' sockets, not for bone's IDProps ... and there may be more of those though). Same issue in readfile.c too.
  • There are some suspicious changes in UI and RNA code, these shall not be needed here. :/

Also, did not went into detail over bpy/RNA yet (also have to check what happens for IDProps of UI items)… this is a huge patch. But think there is no huge blocker anymore, mainly a few refinement iterations still needed to get it master-ready.

source/blender/blenkernel/BKE_idprop.h
84–89

To be removed.

95–96

To be removed

source/blender/blenkernel/BKE_scene.h
101 ↗(On Diff #8353)

This should be nuked away, and what it’s doing should be done directly in ED_object_single_users.

(ultimately/ideally all this should be replaced by some calls to library_remap.c function, but this is TODO out-of-scope of this patch).

source/blender/blenkernel/intern/idprop.c
39

To be removed

48

To be removed

source/blender/blenkernel/intern/library_query.c
238–239

Eeeek! no, absolutely not, dtar->id is a target, not an owner, here, it shall absolutely not be processed here. To be removed.

source/blender/blenkernel/intern/library_remap.c
750–751

What is this doing here? this should *absolutely* not be needed.

Call to BKE_libblock_relink_ex() below is supposed to do the work - if requested by caller only.

877

Tsst, please avoid those useless changes in patch ;)

source/blender/blenloader/intern/readfile.c
1959–2004

This whole section looks like missed merge or something? see no reason for this to be different from master?

2014–2051

Again, why those changes?

source/blender/editors/interface/interface_regions.c
815

should rather be size_t offset, since it’s operating on pointer

source/blender/editors/object/object_relations.c
2095–2106

AFAIK old_scene is not used at all now? can be nuked then.

source/blender/makesrna/RNA_define.h
169

That kind of change sounds suspicious, what’s the point here? we nearly never use type * const ptr (i.e. const pointer to variable data, const type *ptr being variable pointer to const data) in Blender…

source/blender/makesrna/intern/rna_access.c
644

Still not clear why those tests have been removed?

2989

Why not use IDP_ID() macro here, instead of 'low-level' access to idprop->data.pointer?

4298–4355

Again, don’t seem right, that kind of changes should not be needed, why are they here?

source/blender/makesrna/intern/rna_define.c
1266

This is wrong, we are talking about 'real' RNA properties here, not IDProps. We do not support arrays of pointers in RNA properties afaik.

This revision now requires changes to proceed.Sat, Mar 18, 4:57 PM
Alexander Romanov (a.romanov) marked 11 inline comments as done.Mon, Mar 20, 2:48 PM

I will update the patch and then will create a branch.

I could not answer some points because of fabricator bugs (callstack: http://pasteall.org/310079)

source/blender/blenkernel/intern/library_remap.c
750–751

Currently we should remap id to NULL regardless do_id_user, when we will try to remove some other block, which point to this one with idprop, the attempt to free will cause crash because of bad pointer on freed id. Else we should pass do_id_user to IDP_FreeProperty

source/blender/makesrna/RNA_define.h
169

Actually don't remember how it's happened. You're right.

Alexander Romanov (a.romanov) marked an inline comment as done.
Alexander Romanov (a.romanov) edited the summary of this revision. (Show Details)

Atavistic changes in test_pointer_array have been reverted.
Handling idprops for sockets and bones in BKE_library_foreach_ID_link has been added.
Cleanup has been made.

Bastien Montagne (mont29) requested changes to this revision.Tue, Mar 28, 4:05 PM

Did a bunch of cleanup & co in branch, can you please update the diff? @Campbell Barton (campbellbarton) maybe you'd like to have a look at the bpy part of this too? can do it, but not that familiar with it…

@Alexander Romanov (a.romanov) I mainly have two pending questions currently:

  1. Still not clear why you always remap in BKE_libblock_free_ex, https://developer.blender.org/D113#inline-25153 does not help me much here… I consider this a show stopper, we cannot accept this in master.
  2. Why do we have an IDP_IDPARRAY type??? I’d expect IDP_ARRAY do be able to handle arrays of IDP_ID properties just as well?

I think #1 is actually related to the fact we have 'unlink' for IDP_ID, when we have freeing for all other types… imho there is no need for special naming here, even if we do not actually free anything in IDP_ID case, whould still be handled exactly as any other IDP freeing function.

Was on the verge of making changes in the branch to address #1 when I realized #2, so… Thought I’d ask why before going into too much refactoring. ;)

This revision now requires changes to proceed.Tue, Mar 28, 4:05 PM
  1. Still not clear why you always remap in BKE_libblock_free_ex, https://developer.blender.org/D113#inline-25153 does not help me much here… I consider this a show stopper, we cannot accept this in master.

You can reproduce the issue if you will run the test in Blender, compilled with asan, without libblock_remap_data call. It will crash during Main release. Passing do_id_user to IDP_FreeProperty is verbose. Using of some global flag is dirty hack...

  1. Why do we have an IDP_IDPARRAY type??? I’d expect IDP_ARRAY do be able to handle arrays of IDP_ID properties just as well?

Don't actually know about the reason of differentiation of this two types. Probably, with checking prop->subtype they can be merged.

Updated with changes from datablock_idprops branch

  1. Still not clear why you always remap in BKE_libblock_free_ex, https://developer.blender.org/D113#inline-25153 does not help me much here… I consider this a show stopper, we cannot accept this in master.

You can reproduce the issue if you will run the test in Blender, compilled with asan, without libblock_remap_data call. It will crash during Main release. Passing do_id_user to IDP_FreeProperty is verbose. Using of some global flag is dirty hack...

Can’t confirm that here, with current branch both 'idprop' tests pass OK with this line disabled for me.

  1. Why do we have an IDP_IDPARRAY type??? I’d expect IDP_ARRAY do be able to handle arrays of IDP_ID properties just as well?

Don't actually know about the reason of differentiation of this two types. Probably, with checking prop->subtype they can be merged.

OK, will merge this back into IDP_ARRAY then.

  1. Still not clear why you always remap in BKE_libblock_free_ex, https://developer.blender.org/D113#inline-25153 does not help me much here… I consider this a show stopper, we cannot accept this in master.

You can reproduce the issue if you will run the test in Blender, compilled with asan, without libblock_remap_data call. It will crash during Main release. Passing do_id_user to IDP_FreeProperty is verbose. Using of some global flag is dirty hack...

Can’t confirm that here, with current branch both 'idprop' tests pass OK with this line disabled for me.

Are you sure you have asan build? I have no crash without asan.
Here is asan report:

I still cannot reproduce… you’r using CTest right? ctest --output-on-failure -R "idprop" … works fine for me :/

I still cannot reproduce… you’r using CTest right? ctest --output-on-failure -R "idprop" … works fine for me :/

No, you use different test which was added recently.
I use the test from the patch:
bin/blender -P ../blender/tests/python/bl_pyapi_datablock_idprop.py

yeah, my commandline includes bl_pyapi_datablock_idprop test…

I've patched id_us_min to print the name of objects

void id_us_min(ID *id)
{
	if (id) {
		fprintf(stderr, "%s\n", id->name);
		const int limit = ID_FAKE_USERS(id);
....

Here are results:


Thinking you are not using up-to-date branch? IDP_id_unregister() has been removed (see rBaae70f182b1477dc9f ).

Thinking you are not using up-to-date branch? IDP_id_unregister() has been removed (see rBaae70f182b1477dc9f ).

last commit:

commit 171c39cc196d088153fcd20561fa85dfe04813b1
Merge: aae70f1 48fa2c8
Author: Bastien Montagne <montagne29@wanadoo.fr>
Date:   Wed Mar 29 16:25:09 2017 +0200

    Merge branch 'master' into datablock_idprops

Still have the same crash

arrrrg! Ahd forgotten to comment again that stupid line in library_remap before testing your command… now can reproduce the crash, sorry for that :(