Page MenuHome

RNA: Test & refactor RNA path resolving [WIP]
Needs ReviewPublic

Authored by Julian Eisel (Severin) on Jul 27 2022, 7:17 PM.

Details

Summary

No user visible changes expected (aside of minor performance gains because of less allocations/copies).

NOTE: Requires D15540.

TODO: Support array properties & ID properties, test other resolving functions.

Main Changes

  • Adds extensive unit tests for RNA_path_resolve()
  • Completely refactor path resolving (details below).
  • Improve API documentation (attempt to explain better, document corner-cases)

All added unit test succeed for both the old and the new code.

Refactored Path Resolving

Split path parsing/tokenizing from resolving.
rna_path_resolve() combined both, and was very complex and overloaded. Instead, allow easy creation of a tokenized path (vector of RNAPathTokens), separate from path resolving. This can be useful for more than resolving, e.g. see changes to RNA_path_back().

Separate resolving the path from querying the resolved path

Now an iterator is provided that progressively resolves the path. Queries can use that to extract the wanted data. This is a much better separation of concerns, and provides more flexibility/extensibility (adding new queries hopefully just means adding, not modifying code).

Use StringRef over 0-terminated strings

StringRef is a way to reference a (sub)-string stored somewhere else. Instead of having to deal with 0-terminated strings, and hence allocating and copying into buffers for tokens, simply point into the original path buffer. Its API is also safer and (arguably) more readable than pointer arithmetic.

There's still one allocation per token left in the path resolving: RNA C-API functions need to be called for the resolving, which need 0-terminated strings. In fact these functions can be changed to use StringRefs as arguments too, with almost no additional changes. Requires C++ though.


Like said rna_resolve_path() was already hugely complex, with various rules like "if this parameter is set, this one most not be set and these parameters will return a value". I would have had to extend it for my use-case with another such pattern. I had a very hard time debugging it and understanding what it's supposed to do in certain cases. Hopefully this is improved significantly now.

Diff Detail

Repository
rB Blender
Branch
temp-rna-path-refactor (branched from master)
Build Status
Buildable 23182
Build 23182: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) requested review of this revision.Jul 27 2022, 7:17 PM
Julian Eisel (Severin) created this revision.
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Jul 27 2022, 7:29 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)

Since this is WIP, only initial feedback.

  • A contrived micro-benchmark calling RNA_path_resolve_full_maybe_null in a loop, it looks to be around twice as slow compared with master. see: P3113 (0.282 vs 0.136 sec in master), using the path: C.object.path_resolve('modifiers["Mirror"].mirror_object').
  • Using std::vector looks to be causing allocations, could this use blender::Vector which can use an inline buffer?
source/blender/makesrna/RNA_path.h
51

Perhaps we should settle on indentation or not in the style guide, personally I don't feel it helps but that may be because doxy-comments are highlighted in my editor.