Details

Reviewers

Bastien Montagne (mont29)
Campbell Barton (campbellbarton)
Brecht Van Lommel (brecht)
Tom Edwards (artfunkel)

Maniphest Tasks

T37754: Patch: DatablockProperty and DatablockVectorProperty

Commits

rBSa7b3047cefcb: Datablock ID Properties
rBa7b3047cefcb: Datablock ID Properties
rBa3f48d65df19: Datablock ID Properties

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Bastien Montagne (mont29) added inline comments.Mar 18 2017, 4:57 PM

source/blender/blenkernel/BKE_idprop.h
95–96	To be removed
source/blender/blenkernel/BKE_scene.h
101	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 ↗	(On Diff #8353)	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 ↗	(On Diff #8353)	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 ↗	(On Diff #8353)	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
171	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

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 ↗	(On Diff #8353)	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
171	Actually don't remember how it's happened. You're right.

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.

Isaac Weaver (wisaac) added a subscriber: Isaac Weaver (wisaac).Mar 21 2017, 11:55 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:

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.
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

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...

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

In D113#60552, @Alexander Romanov (a.romanov) wrote:

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.

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.

In D113#60585, @Bastien Montagne (mont29) wrote:

In D113#60552, @Alexander Romanov (a.romanov) wrote:

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:

logfile10 KBDownload

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

In D113#60612, @Bastien Montagne (mont29) wrote:

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:

logfile_asan18 KBDownload

logfile845 KBDownload

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

In D113#60619, @Bastien Montagne (mont29) wrote:

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 :)

Bastien Montagne (mont29) mentioned this in rBe5fa738ce972: UI cleanup: simplify Icon handling of uiDefAutoButR for PROP_POINTER..Mar 31 2017, 12:16 PM

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.

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

I will make some speed tests to measure slowdowns.

Salvador Ureña (Asticles) removed a subscriber: Salvador Ureña (Asticles).Apr 5 2017, 2:20 PM

In D113#60861, @Campbell Barton (campbellbarton) wrote:

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.

JT Nelson (jta) added a subscriber: JT Nelson (jta).Apr 9 2017, 4:52 PM

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 ↗

(On Diff #8546)

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

point to correct test file in cmake config
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

pep8 cleanup

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
1 ↗	(On Diff #8613)	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.
318–323 ↗	(On Diff #8613)	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

Harbormaster completed remote builds in B554: Diff 8619.Apr 12 2017, 7:29 PM

Remove red alert (will be added in separate commit)

fix harmless mistake in copy paste

Closed by commit rBa7b3047cefcb: Datablock ID Properties (authored by Alexander Romanov (a.romanov)). · Explain WhyApr 13 2017, 11:36 AM

This revision was automatically updated to reflect the committed changes.

Alexander Romanov (a.romanov) mentioned this in T51198: Crash with new Datablock ID Properties.Apr 14 2017, 3:10 PM

mic lack30 (miclack) awarded a token.Apr 14 2017, 8:39 PM

Alexander Romanov (a.romanov) mentioned this in T51260: Forward compatibility after D113.Apr 20 2017, 10:49 AM

Alexander Romanov (a.romanov) mentioned this in rBf5bc8ad4ce87: Add red alert in UI controls for datablock pointer properties.Apr 27 2017, 5:34 PM

Kerim Borchaev (warkid) added a subscriber: Kerim Borchaev (warkid).Apr 30 2017, 11:50 PM

Campbell Barton (campbellbarton) mentioned this in rB5cdd94a58e6b: Fix PyAPI crash assigning/deleting id-properties.May 6 2017, 3:53 PM

Brecht Van Lommel (brecht) mentioned this in T51452: EnumProperty fill dynamically using callback.May 11 2017, 7:33 PM

Simon Wendsche (BYOB) mentioned this in T53509: Datablock ID Properties attached to bpy.types.Material are not saved (or loaded).Dec 8 2017, 12:10 PM

Datablock ID Properties
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 383

source/blender/blenkernel/BKE_idprop.h

source/blender/blenkernel/BKE_scene.h

source/blender/blenkernel/intern/blender.c

source/blender/blenkernel/intern/idprop.c

source/blender/blenkernel/intern/library.c

source/blender/blenkernel/intern/object.c

source/blender/blenkernel/intern/scene.c

source/blender/blenlib/BLI_threads.h

source/blender/blenlib/intern/threads.c

source/blender/blenloader/intern/readfile.c

source/blender/editors/interface/interface.c

source/blender/editors/interface/interface_layout.c

source/blender/editors/interface/interface_utils.c

source/blender/editors/object/object_relations.c

source/blender/makesdna/DNA_ID.h

source/blender/makesrna/RNA_define.h

source/blender/makesrna/intern/rna_ID.c

source/blender/makesrna/intern/rna_access.c

source/blender/makesrna/intern/rna_define.c

source/blender/makesrna/intern/rna_internal.h

source/blender/makesrna/intern/rna_internal_types.h

source/blender/python/generic/idprop_py_api.c

source/blender/python/intern/bpy_props.c

source/blender/python/intern/bpy_rna.h

source/blender/python/intern/bpy_rna.c

source/creator/creator.c

Datablock ID PropertiesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 383

source/blender/blenkernel/BKE_idprop.h

source/blender/blenkernel/BKE_scene.h

source/blender/blenkernel/intern/blender.c

source/blender/blenkernel/intern/idprop.c

source/blender/blenkernel/intern/library.c

source/blender/blenkernel/intern/object.c

source/blender/blenkernel/intern/scene.c

source/blender/blenlib/BLI_threads.h

source/blender/blenlib/intern/threads.c

source/blender/blenloader/intern/readfile.c

source/blender/editors/interface/interface.c

source/blender/editors/interface/interface_layout.c

source/blender/editors/interface/interface_utils.c

source/blender/editors/object/object_relations.c

source/blender/makesdna/DNA_ID.h

source/blender/makesrna/RNA_define.h

source/blender/makesrna/intern/rna_ID.c

source/blender/makesrna/intern/rna_access.c

source/blender/makesrna/intern/rna_define.c

source/blender/makesrna/intern/rna_internal.h

source/blender/makesrna/intern/rna_internal_types.h

source/blender/python/generic/idprop_py_api.c

source/blender/python/intern/bpy_props.c

source/blender/python/intern/bpy_rna.h

source/blender/python/intern/bpy_rna.c

source/creator/creator.c

Datablock ID Properties
ClosedPublic
Actions

Revision Contents
Changeset List