Page MenuHome

AssetsBrowser: Add ID Properties to Asset Indexer
ClosedPublic

Authored by Jeroen Bakker (jbakker) on Oct 25 2021, 3:48 PM.

Details

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": ..,
  },
]
DNASerialize typeNote
IDProperty.nameStringValue
IDProperty.typeStringValue"IDP_STRING", "IDP_INT", "IDP_FLOAT", "IDP_ARRAY", "IDP_GROUP", "IDP_DOUBLE"
IDProperty.subtypeStringValue"IDP_INT", "IDP_FLOAT", "IDP_GROUP", "IDP_DOUBLE"
IDProperty.valueStringValueWhen type is IDP_STRING
IDProperty.valueIntValueWhen type is IDP_INT
IDProperty.valueDoubleValueWhen type is IDP_FLOAT/IDP_DOUBLE
IDProperty.valueArrayValueWhen type is IDP_GROUP. Recursively uses the same structure as described in this section.
IDProperty.valueArrayValueWhen 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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jeroen Bakker (jbakker) planned changes to this revision.Nov 1 2021, 9:16 AM
Jeroen Bakker (jbakker) marked an inline comment as done.
  • 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.

Jeroen Bakker (jbakker) marked 3 inline comments as done.

Refactored BKE_idprop_create.

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

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) marked 3 inline comments as done.
  • 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.
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) 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.

Hans Goudey (HooglyBoogly) requested changes to this revision.Dec 6 2021, 6:16 PM

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
83

Here too, static functions shouldn't have the BKE prefix.

97

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!

Merged with latest master.

Jeroen Bakker (jbakker) planned changes to this revision.Dec 14 2021, 8:21 AM

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

Reapplied patch on master as arcanist removed it.

Jeroen Bakker (jbakker) marked 8 inline comments as done.
  • 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
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)
  • Renamed CPP public functions.
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) added inline comments.
source/blender/makesdna/DNA_ID.h
159–160

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.

160

Is there any benefit to \brief? If not, seems better to avoid it.

Jeroen Bakker (jbakker) marked 2 inline comments as done.
  • Cleanup: remove /brief from enum values.
Sybren A. Stüvel (sybren) requested changes to this revision.EditedDec 20 2021, 5:47 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
Jeroen Bakker (jbakker) marked 8 inline comments as done.
  • 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.
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.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 18 2022, 11:18 AM
This revision was automatically updated to reflect the committed changes.