Datablock ID Properties
ClosedPublic

Authored by Alexander Romanov (a.romanov) on Dec 16 2013, 7:36 PM.
Tags
None
Tokens
"Love" token, awarded by miclack."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

Repository
rB Blender
There are a very large number of changes, so older changes are hidden. Show Older Changes
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 ↗(On Diff #8353)

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.Mar 18 2017, 4:57 PM
Alexander Romanov (a.romanov) marked 11 inline comments as done.Mar 20 2017, 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.Mar 28 2017, 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.Mar 28 2017, 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 :(

OK, so finally understood that stupid freeing issue, indeed, correct solution is to pass do_id_user to IDP freeing functions, which makes sense, since they handle ID usages too now…

Pushed the fix, re idparray in fact will let that aside for now, this is unrelated with the patch anyway (my mistake).

So will give it another round of global checkup this afternoon, but think it’s nearly mergeable now :)

OK, pushed final changes from my side, considering the branch ready for merge (can you please update again the diff?).

Unless others want to check (again, only quickly skimmed over bpy changes…), only remaining question is: is it safe to merge this in master (for upcoming 2.79), or should we rather keep this for 2.8, @Sergey Sharybin (sergey)? @Campbell Barton (campbellbarton)?
Wondering, since I had to make changes to (and fix) a lot of things in branch during review, and this affects rather core parts of the code (and easy crashers too, ID pointers...). So am worry about relative short time before 2.79 (we already postponed bcon3 two weeks).

Checked the patch and generally seems OK.

Some questions:

  • Did anyone test this patch on a complex file? Are there slowdowns, if so how much?:
    • Deleting an object in a file with ~1k objects.
    • Deleting all objects.
    • Undo.
  • Are there any issues with an ID referencing it's self in collection?
  • Does this work with library data?
    • Does making an indirectly linked data-block directly linked work correctly on reload for eg?
    • Will linking in a data-block follow its id-properties too?
source/blender/python/intern/bpy_props.c
402

redundant cast.

source/blender/python/intern/bpy_rna.c
3274

style issues

tests/python/bl_pyapi_datablock_idprop.py
1 ↗(On Diff #8507)

We have bl_pyapi_idprop.py currently, could be called bl_pyapi_idprop_datablock.py ?

27–52 ↗(On Diff #8507)

Looks like this could use Python's unittest module, see bl_pyapi_idprop.py.

Not pushing to rewrite for release, just think it could be done a bit more cleanly.

  1. Updated from the datablock_idprops branch
  2. Taken into account the comments of @Campbell Barton (campbellbarton)

I will make some speed tests to measure slowdowns.

Checked the patch and generally seems OK.

Some questions:

  • Did anyone test this patch on a complex file? Are there slowdowns, if so how much?:
    • Deleting an object in a file with ~1k objects.
    • Deleting all objects.
    • Undo.

Here are my results: https://docs.google.com/spreadsheets/d/1S_vwTViBw0IPcyeN6Og7htXr38qLn722q38Gxk1n6Mk/edit?usp=sharing
Source: P460

  • Are there any issues with an ID referencing it's self in collection?

Did not notice. Could you point to some test-case?

  • Does this work with library data?
    • Does making an indirectly linked data-block directly linked work correctly on reload for eg?

Did not notice incorrect behavior.

  • Will linking in a data-block follow its id-properties too?

Yes, it follows.

I can add additional tests.

  • Cleanup: quiet unused warning
Campbell Barton (campbellbarton) requested changes to this revision.Apr 10 2017, 10:15 AM
Campbell Barton (campbellbarton) added inline comments.
tests/python/CMakeLists.txt
97

This file doesn't exist, even when corrected tests are raising an error for me:

Traceback (most recent call last):
  File "/src/blender/tests/python/bl_datablock_idprop.py", line 328, in <module>
    main()
  File "/src/blender/tests/python/bl_datablock_idprop.py", line 319, in main
    test0()
  File "/src/blender/tests/python/bl_datablock_idprop.py", line 186, in test0
    lamp_us = bpy.data.objects["Lamp"].data.users
KeyError: 'bpy_prop_collection[key]: key "Lamp" not found'
This revision now requires changes to proceed.Apr 10 2017, 10:15 AM

Correct mistake, last patch made against soc-2016-pbvh-painting

  1. point to correct test file in cmake config
  2. fix test portability by replacing read_home_file with read_factory_settings

Updated for the current state of master branch

  • Use pyrna_id_CheckPyObject instead of attempting to convert

Made some fix for Py API checking ID-properties, otherwise LGTM.

(Some comments on tests but not blocking issues)

tests/python/bl_pyapi_idprop_datablock.py
2

Note this test doesn't really check for correct behavior - just that it doesn't give some exception, dont think its a blocker for the patch but would prefer something closer to how bl_pyapi_idprop.py works.

319–324

these names are fairly meaningless, please use: test_*what_the_test_does*

  • fix tests' names + comments corrections

fix incorrect indentation in abort_if_false + lowercase start symbol of comments

  • Remove red alert (will be added in separate commit)
  • fix harmless mistake in copy paste
This revision was automatically updated to reflect the committed changes.