Details

Reviewers

Julian Eisel (Severin)
Sybren A. Stüvel (sybren)
Bastien Montagne (mont29)
Hans Goudey (HooglyBoogly)

Maniphest Tasks

T92306: AssetBrowser: Add IDProperties to Asset Indexer.

Commits

rB36068487d076: AssetsBrowser: Add ID Properties to Asset Indexer

Summary

Object/collection asset workflow would need the bounding box for snapping.
The bounding box is stored using ID properties in the scene. Currently ID properties
aren't stored in the asset index, what would break object snapping. For this reason
Asset Indexing is turned off in mater. This patch will introduce the indexing of ID
properties what will allow the indexing to be turned on again.

Data Mapping

For data mapping we store the internal structure of IDProperty to the indexer (including meta-data) to be able to deserialize it back.

[
  {
    "name":  ..,
    "value": ..,
    "type": ..,
    /* `subtype` and `length` are only available for IDP_ARRAYs. */
    "subtype": ..,
  },
]

DNA	Serialize type	Note
IDProperty.name	StringValue
IDProperty.type	StringValue	"IDP_STRING", "IDP_INT", "IDP_FLOAT", "IDP_ARRAY", "IDP_GROUP", "IDP_DOUBLE"
IDProperty.subtype	StringValue	"IDP_INT", "IDP_FLOAT", "IDP_GROUP", "IDP_DOUBLE"
IDProperty.value	StringValue	When type is IDP_STRING
IDProperty.value	IntValue	When type is IDP_INT
IDProperty.value	DoubleValue	When type is IDP_FLOAT/IDP_DOUBLE
IDProperty.value	ArrayValue	When type is IDP_GROUP. Recursively uses the same structure as described in this section.
IDProperty.value	ArrayValue	When type is IDP_ARRAY. Each element holds a single element as described in this section.

NOTE: IDP_ID and IDP_IDARRAY aren't supported. The entry will not be added.

Example

[
  {
    "name": "MyIntValue,
    "type": "IDP_INT",
    "value": 6,
  },
  {
    "name": "myComplexArray",
    "type": "IDP_ARRAY",
    "subtype": "IDP_GROUP",
    "value": [
        [
          {
            "name": ..
            ....
          }
        ]
    ]
  }
]

Considered alternatives

Add conversion functions inside asset_indexer; makes generic code part of a specific solution.
Add conversion functions inside BLI_serialize; would add data transformation responsibilities inside a unit that is currently only responsible for formatting.
Use direct mapping between IDP properties and Values; leads to missing information and edge cases (empty primitive arrays) that could not be de-serialized.

Diff Detail

Repository

rB Blender

Branch

T92306-asset-lib-indexing-custom-properties (branched from master)

Build Status

Buildable 18314
Build 18314: arc lint + arc unit

Event Timeline

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

Forgot to upload patch.
Refactorded BKE_idprop_create functions.

source/blender/blenkernel/BKE_idprop.h
260 ↗	(On Diff #43974)	Seems like I didn't update the patch. Will do after resolving the other comments.
295–312 ↗	(On Diff #43974)	The current C-api can allocate the idprop for a certain number of elements, without filling the data or allocate the idprop for a certain number of elements and copy the given values over. I didn't want to change this behavior with this patch. But I see that the API for only allocating is not usable due to the function overloading happening on the `values` field. Will remove the `std:optional` to reduce the confusion and only support to allocate with copying.

Refactored BKE_idprop_create.

Harbormaster completed remote builds in B18361: Diff 44195.Nov 1 2021, 11:51 AM

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Nov 1 2021, 11:56 AM

Replaced pointers with Span.

Harbormaster completed remote builds in B18362: Diff 44197.Nov 1 2021, 12:16 PM

Looking mostly good. Not much to add, just some suggestions.

I can't currently apply the patch however.

source/blender/blenkernel/intern/idprop_serialize.cc
171	Can use `typename ValueType = typename PrimitiveType::value_type` here, so the second template parameter doesn't have to be explicitly passed for containers like `Span`.
535	There's plenty of boilerplate here, with slight differences. If you want to can turn this into a single function with compile time logic. Something like this: template<typename ValueType> static IDProperty BKE_idprop_array_from_value(EntryReader &entry_reader) { BLI_assert(entry_reader.get_type().value() == IDP_ARRAY); std::optional<std::string> name = entry_reader.get_name(); if (!name.has_value()) { return nullptr; } std::optional<Vector<ValueType>> extracted_value; if constexpr (std::is_same_v<ValueType, int32_t>) { BLI_assert(entry_reader.get_subtype().value() == IDP_INT); extracted_value = entry_reader.get_array_int_value(); } else if constexpr (std::is_same_v<ValueType, float>) { BLI_assert(entry_reader.get_subtype().value() == IDP_FLOAT); extracted_value = entry_reader.get_array_float_value(); } ... if (!extracted_value.has_value()) { return nullptr; } return BKE_idprop_create(name->c_str(), extracted_value); } This doesn't need to be limited to the array types only. Also I wouldn't use the `BKE_` prefix for internal functions.
source/blender/blenkernel/intern/idprop_serialize_test.cc
334	You can make such strings more readable using C++11 raw string literals: test_convert_idprop_from_value(R"([{"name":"MyStringName","type":"IDP_STRING","value":"MyString"}])",
source/blender/makesdna/DNA_ID.h
160	Doxygen.

Jeroen Bakker (jbakker) edited projects, added BF Blender (3.1); removed BF Blender (3.0).Nov 8 2021, 1:10 PM

Merge branch 'temp-T91406-asset-lib-indexing' into T92306-asset-lib-indexing-custom-properties
Use raw string literals.
Fixed spelling in comment.
Removed prefix from private functions.
Use template functions to reduce boiler plating.
Use doxygen comments.

Harbormaster completed remote builds in B18890: Diff 45132.Nov 23 2021, 10:14 AM

Jeroen Bakker (jbakker) mentioned this in rBebe5a5eca813: AssetBrowser: Disable asset indexing..Nov 24 2021, 8:43 AM

Jeroen Bakker (jbakker) added inline comments.Nov 28 2021, 12:23 PM

source/blender/blenkernel/intern/idprop_serialize.cc
171	I don't get it. PrimitiveType can be an `int32_t`. `int32_t` doesn't know `::value_type` so how does this work?

Julian Eisel (Severin) accepted this revision.Nov 30 2021, 12:01 PM

Julian Eisel (Severin) added inline comments.

source/blender/blenkernel/intern/idprop_serialize.cc
171	I can check on this once this is in master if you want, no biggie.
source/blender/editors/asset/intern/asset_indexer.cc
80–84	Merge conflict markers :)
source/blender/makesdna/DNA_ID.h
160	No need to put the `\brief` here (we usually don't do it). Nothing against keeping it either, I'll leave it up to you.

Julian Eisel (Severin) mentioned this in rBdbeab82a89c4: T91406: Asset Library Indexing.Dec 6 2021, 12:50 AM

A few more general comments beside the comments inline:

The patch description isn't up to date anymore (still talks about "Add CPP-only section to blenkernel/BKE_idprops.h.")
It feels a bit strange for BKE_idprop.hh to include BLI_serialize.hh, since I expect many uses of IDProperties won't be concerned with serializing them. Maybe a BKE_idprop_serialize.hh header would make sense? Maybe you've already thought about that though.
- "value" is a very generic name for the two functions that deal with serialization. (another reason to put them in a separate header maybe?)
Function naming: the bke and idprop namespaces are redundant with the BKE_idprop prefix. In other C++ code in the kernel, just the namespace has been used, without the prefix. Here's what I'd suggest:
- BKE_idprop_create -> bke::idprop::create
- BKE_idprop_create_group -> bke::idprop::create_group
- BKE_idprop_to_value -> bke::idprop::to_serialize_value or some variant of that.

source/blender/blenkernel/BKE_idprop.hh
28	No need for `struct` here in a C++ header.
28	"properties" is plural here, but the name in the implementation is "prop". I'd suggest "property" maybe? Might be good to do a clang tidy pass on this patch if you're compiling on linux.
29	No need to manually specify the `blender::` namespace here.
31	Since `StringRefNull` is passed by value here, there is no need for const in the header (it can still be in the declaration in the cc file).
source/blender/blenkernel/intern/idprop_create.cc
82 ↗	(On Diff #45132)	Here too, static functions shouldn't have the `BKE` prefix.
96 ↗	(On Diff #45132)	Might as well use a `static_assert` in the function then? Since this function is local to the file, it shouldn't have the `BKE_idprop` prefix.

This revision now requires changes to proceed.Dec 6 2021, 6:16 PM

Would be nice to get this in before everybody leaves for holidays, so it won't drag for longer than it needs to. It's so close already!

Julian Eisel (Severin) mentioned this in T94031: Asset Browser: 3.1 (and some 3.2) Targets.Dec 13 2021, 6:03 PM

Merged with latest master.

Harbormaster completed remote builds in B19411: Diff 46009.Dec 14 2021, 8:11 AM

Argh! arc did a bad thing and removed the patch :-(

Reapplied patch on master as arcanist removed it.

Harbormaster completed remote builds in B19412: Diff 46010.Dec 14 2021, 8:48 AM

Removed struct keyword from CPP header file.
Silenced clang-tidy warnings.
Use same parameter names in definition and implementation.
Moved documentation to header file.
Removed const from definition where parameter is passed by value.
Remove BKE_ prefix from private methods and members.
Added static_assert to check for matching template parameters

Harbormaster completed remote builds in B19414: Diff 46012.Dec 14 2021, 10:27 AM

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Dec 14 2021, 10:31 AM

Renamed CPP public functions.

Harbormaster completed remote builds in B19416: Diff 46014.Dec 14 2021, 10:45 AM

Jeroen Bakker (jbakker) added inline comments.Dec 14 2021, 10:45 AM

source/blender/blenkernel/BKE_idprop.hh
28	`IDProperty` is implemented is a very strange none-descriptive structure. The passed IDProperty is a linked list that contains multiple IDProperties. Return type is Array. IMO properties would be the best name for it. I will reflect it in the implementation. `idprop` is the prefix for IDProperties.

I didn't rename the headerfile as it contains creation and conversion functions. When needed we can add more functions and split them up later.

Hans Goudey (HooglyBoogly) accepted this revision.Dec 14 2021, 4:30 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/makesdna/DNA_ID.h
160	Is there any benefit to `\brief`? If not, seems better to avoid it.
161	I think it's better to add this as a define right after the enum, so it doesn't have to be handled in switch cases. There's only one switch that uses this enum name directly, but I'd expect more to be added in the future Handling `IDP_NUMTYPES` in the switch is ugly, and adding `default` to avoid it has other downsides.

Cleanup: remove /brief from enum values.

Harbormaster completed remote builds in B19509: Diff 46160.Dec 17 2021, 1:30 PM

I keep finding ObjectValue confusing. This is technically out of scope of this patch, this patch to me exposes the confusion.

ObjectValue is documented in BLI_serialize.hh as:

* - `ObjectValue`: represents an object (key value pairs where keys are strings and values can be
*   of different types.

so that means it's not talking about Blender's concept of "object", also not Python's, but the JSON/JavaScript one.

I would strongly suggest to rename this type to something like DictionaryValue or CompoundValue.

There is a lot of code duplication in idprop_serialize.cc, and many different switches going over IDP_xxx values. Is there no way to use some polymorphism and let the compiler deal with this?

source/blender/blenkernel/BKE_idprop.hh
28	This is not really consistent with the function declaration. The function name is `convert_to_serialize_value`, which is singular, but the comment says multiple values are returned. If the property values are returned as a single array, this should be documented.
30	Why are they ignored?
30	What does "When inside" mean? When what is inside of what? And how does this function know about that?
36	This isconsistent with the documentation, which says it produces a `Value`.
42	For all the following functions: who owns the returned pointer? If it's the caller, why not use `std::unique_ptr<>` to make that clear?
source/blender/blenkernel/intern/idprop_serialize.cc
40	End in question mark if it's a question/query.
116	This doesn't seem to actually store the value of the property. As such I feel that this doesn't really convert this `IDProperty` into a complete `ObjectValue`; maybe it's worth it to document that this produces an incomplete/partial instance? At a minimum, change "Fill the instance..." to "Only fill the instance..." to indicate it's mentioning a limitation.
305	Document what this struct is/means/is for.
source/blender/blenkernel/intern/idprop_serialize_test.cc
334	Why are these functions testing a JSON list with a single element? How can an array-of-strings be validly deserialised to a single string?
354	This should actually be testing the extra precision `double` can provide; i.e. higher than `float` can do.
384	These lines are way too long. Having some line breaks in there will make it easier to read, and also introduce some varying JSON notations in the mix to test those as well.

This revision now requires changes to proceed.Dec 20 2021, 5:47 PM

RedMser (RedMser) added a subscriber: RedMser (RedMser).Dec 25 2021, 3:56 PM

Renamed ObjectValue to DictionaryValue.
Reduce switch statements using classes.
Rebased with latest master.
Reshuffled code.
convert_to_serialize_values -> convert_to_serialize_values.
Use unique_ptr for creation functions.
Renamed EntryReader.

Harbormaster completed remote builds in B19725: Diff 46536.Jan 3 2022, 12:44 PM

Jeroen Bakker (jbakker) added inline comments.Jan 3 2022, 12:44 PM

source/blender/blenkernel/BKE_idprop.hh
30	They are ignored because they aren't supported. Will add a comment why they aren't supported. (because they cannot be serialized).
source/blender/blenkernel/intern/idprop_serialize_test.cc
334	IDProp is a mess. A single IDProperty is already a linked list. But it could also be an array. Or both.The outer [] for supporting the linked list. Hence the confusion in the naming of the functions and the requirement of the []. We don't have the need to convert a single element as that would limit the linked lists and there isn't a public api designed for handling a single element.
354	I would not add that to this set of test cases but rather to `BLI_serialize_test.cc`

Jeroen and I chatted about this today. We only seem to be fine-tuning naming, documentation and code style at this point. Design/functionality seems to be fine, but the fine-tuning stalled review quite a bit. Plus, review points seem to be addressed already. So I'm fine with merging this into master now, if there are more tweaks to be done, we can do them there still. If Bastien is in the office tomorrow Jeroen and him can have a look together first though.

LGTM

This revision was not accepted when it landed; it landed in state Needs Review.Jan 18 2022, 11:18 AM

Closed by commit rB36068487d076: AssetsBrowser: Add ID Properties to Asset Indexer (authored by Jeroen Bakker (jbakker)). · Explain Why

This revision was automatically updated to reflect the committed changes.

Jeroen Bakker (jbakker) added a commit: rB36068487d076: AssetsBrowser: Add ID Properties to Asset Indexer.

AssetsBrowser: Add ID Properties to Asset Indexer
ClosedPublic
Actions

Details

Data Mapping

Considered alternatives

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 44112

source/blender/blenkernel/BKE_idprop.hh

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/idprop_serialize.cc

source/blender/blenkernel/intern/idprop_serialize_test.cc

source/blender/editors/asset/CMakeLists.txt

source/blender/editors/asset/ED_asset_indexer.h

source/blender/editors/asset/intern/asset_indexer.cc

source/blender/editors/asset/intern/asset_list.cc

source/blender/editors/include/ED_file_indexer.h

source/blender/editors/space_file/CMakeLists.txt

source/blender/editors/space_file/file_indexer.h

source/blender/editors/space_file/file_indexer.cc

source/blender/editors/space_file/filelist.h

source/blender/editors/space_file/filelist.c

source/blender/editors/space_file/space_file.c

source/blender/makesdna/DNA_ID.h

AssetsBrowser: Add ID Properties to Asset IndexerClosedPublicActions

Details

Data Mapping

Considered alternatives

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 44112

source/blender/blenkernel/BKE_idprop.hh

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/idprop_serialize.cc

source/blender/blenkernel/intern/idprop_serialize_test.cc

source/blender/editors/asset/CMakeLists.txt

source/blender/editors/asset/ED_asset_indexer.h

source/blender/editors/asset/intern/asset_indexer.cc

source/blender/editors/asset/intern/asset_list.cc

source/blender/editors/include/ED_file_indexer.h

source/blender/editors/space_file/CMakeLists.txt

source/blender/editors/space_file/file_indexer.h

source/blender/editors/space_file/file_indexer.cc

source/blender/editors/space_file/filelist.h

source/blender/editors/space_file/filelist.c

source/blender/editors/space_file/space_file.c

source/blender/makesdna/DNA_ID.h

AssetsBrowser: Add ID Properties to Asset Indexer
ClosedPublic
Actions

Revision Contents
Changeset List