Attribute refactoring

Here is semi-major attribute refactoring.
I would be happy if someone review code and/or test changes before I push it.
https://github.com/urho3d/Urho3D/pull/2091

1. Arbitrary attribute metadata was added.

Attribute metadata is per-attribute VariantMap with custom values.
The only way to set metadata is to call SetMetadata on URHO3D_ATTRIBUTE -like macros. SetMetadata calls may be chained.

Metadata may be acquired via AttributeInfo::GetMetadata or AttributeInfo::GetMetadata<T> . It is also exposed to Angel Script.

2. Offset-based attributes were removed due to unsafety.

URHO3D_ATTRIBUTE and URHO3D_ENUM_ATTRIBUTE are aliased to accesor attributes now. So, offset can’t be used to identify attribute anymore. Use attribute name or metadata. See Constraint for example.

Attribute type hacking will get broken. If you lied with attribute type, code won’t compile. E.g. Light::CascadeParameters::splits_ were float[4] and serialized like Vector4 . Don’t lie to your compiler anymore.

3. Vector structure elements and corresponding defines were removed, @KonstantTom .

Use metadata variable AttributeMetadata::P_VECTOR_STRUCT_ELEMENTS instead. It shall be initialized with StringVector instead of const char*[] , no trailing zero is needed.

Can you compare the performance difference between these changes, especially the retiring the offset part.

These changes are fine to me. Arbitrary metadata is much more flexible. It looks good for me.

It looks good to me.

The only thing I don’t like is the new line for .SetMetadata but it’s just a matter of personal taste.

Performance is not affected. The test below take the same time with both old and new version (75++5 ms for Debug, 10-+0.5 ms for Release). I didn’t test metadata-related changes because they have very limited area.

#include <Urho3D/Scene/Serializable.h>
#include <Urho3D/IO/VectorBuffer.h>
#include <Urho3D/Core/StringUtils.h>
#include <chrono>

using namespace Urho3D;

#define URHO3D_NEW_ATTRIBUTE(name, typeName, variable, defaultValue, mode) URHO3D_ACCESSOR_ATTRIBUTE_FREE(name, [](const ClassName* classPtr) -> typename AttributeTrait<typeName >::ReturnType { return classPtr->variable; }, [](ClassName* classPtr, typename AttributeTrait<typeName >::ParameterType value) { classPtr->variable = value; }, typeName, defaultValue, mode)
#undef URHO3D_ATTRIBUTE
#define URHO3D_ATTRIBUTE(name, typeName, variable, defaultValue, mode) URHO3D_NEW_ATTRIBUTE(name, typeName, variable, defaultValue, mode)

class TestSerializable : public Serializable
{
   URHO3D_OBJECT(TestSerializable, Serializable);

public:
   TestSerializable(Context* context) : Serializable(context)
   {
      bool_ = !!Random(0, 1);
      int_ = Random(1, 100);
      float_ = Random(1.0f, 100.0f);
      string_ = String(Random(1, 100));
      vector_ = { Random(1, 100), Random(1, 100), Random(1, 100) };
      map_ = { { "1", Random(1, 100) }, { "2", Random(1, 100) }, { "3", Random(1, 100) } };
   }
   static void RegisterObject(Context* context)
   {
      context->RegisterFactory<TestSerializable>();

      URHO3D_ATTRIBUTE("Bool", bool, bool_, false, AM_DEFAULT);
      URHO3D_ATTRIBUTE("Int", int, int_, 0, AM_DEFAULT);
      URHO3D_ATTRIBUTE("Float", float, float_, 0.0f, AM_DEFAULT);
      URHO3D_ATTRIBUTE("String", String, string_, String::EMPTY, AM_DEFAULT);
      URHO3D_ATTRIBUTE("VariantVector", VariantVector, vector_, Variant::emptyVariantVector, AM_DEFAULT);
      URHO3D_ATTRIBUTE("VariantMap", VariantMap, map_, Variant::emptyVariantMap, AM_DEFAULT);
   }
private:
   bool bool_;
   int int_;
   float float_;
   String string_;
   VariantVector vector_;
   VariantMap map_;
};

size_t Test(Context* context)
{
   TestSerializable::RegisterObject(context);
   Vector<SharedPtr<TestSerializable>> objects;
   for (int i = 0; i < 10000; ++i)
      objects.Push(MakeShared<TestSerializable>(context));
   VectorBuffer output;
   output.Resize(1024 * 1024);

   auto t1 = std::chrono::high_resolution_clock::now();
   for (TestSerializable* object : objects)
      object->Save(output);
   auto t2 = std::chrono::high_resolution_clock::now();
   size_t us = (size_t)std::chrono::duration_cast<std::chrono::microseconds>(t2 - t1).count();

   File file(context);
   file.Open("C:/output.bin", FILE_WRITE);
   file.Write(output.GetData(), output.GetSize());
   file.Close();
   return us;
}

The idea is pretty simple here. Have you heard about “single line -
single declaration” rule in C++? Here is the same situation. Metadata is the declaration of the new variable inside the map, so it shall be placed on its own line.
The same is about variadic SendEvent : I’d like to put every event parameter on its own line.

In this test code you’re not really testing the performance of object->Save() , you’re testing the hundreds of memory allocations that are occurring in VectorBuffer, which are comparatively slow to serialization.

I can’t test it for myself now but recommend reserving space on the vector buffer before doing the test and see if it makes any difference.

output.Resize(sizeof(TestSerializable) * 10000);  // Not entirely sure how attributes contribute to the size of the serialized data, so maybe you need to increase this factor

Hundreds of memory allocations take only about 10% of overall time. New test results:

Debug Old: 65…75 ms;
Debug New: 70…76 ms;
Release Old: 8.8…10 ms;
Release New: 9.3…10 ms.

How are the meanings of these results? Does that mean Release Old use 8.8 to 10 ms while Release New use 9.3 to 10 ms?
Also VauleAnimation and ObjectAnimation relies on AttributeAnimation. We may need to test their performance differences before commiting such a fundamental code change as well.

Please come back often and give the valuable opinions. It may take you ten minutes but will save a lot of arguments and point the right direction for this amazing engine. Like the retiring the attribute offset thing, do you think it will affect the overall performance since AttributeAnimation, ValueAnimation and ObjectAnimation are all depending on it?

I don’t understand why do you care about performance at all.

1. offset attributes is an old hack that should be removed ASAP.
2. Performance can’t be worse than accessor attributes.
3. Only 25% of urho attributes are affected.

Debug Old: 65…75 ms;
Debug New: 70…76 ms;
Release Old: 8.8…10 ms;
Release New: 9.3…10 ms.

Judging from this test result, new performance is actually a bit slower, consistently in both debug and release.

You should treat these results as ‘accessor attributes are negligibly slower (~5% in release mode) than offset attributes due to two extra pointer-to-function calls’.
Then, my refactorig is obligatory migration to accessor attributes without breaking API for safety reasons.

I understand fixing Undefined Behavior with the offset usage (just use lambda accessor instead?).

I don’t understand why:

1. you need attribute meta info
2. overhead is introduced

I just wanted to add attribute metadata at some point and get rid of kludge named ‘vector structure elements’. When I tried to remove offset attributes, I understood that the point is now. See changes in Constraint.cpp in my PR.

What overhead do you mean?

5% slower in release mode consistently is not negligible for such a core feature because many other features are depending on it, like ValueAnimation, ObjectAnimation, Serialization, Deserialization and Networking. Using offset exists safely for many years and should not be changed just for so called safety at the price of performance. And this change cannot be opt out. I suggest only commit such a change with thorough test and ensure no negative performance introduced at all.

Looked at it a bit, undocumented and looks quite messy. I’m not sure I understand what is it used for (stores names of members encoded into a VariantVector?).
Also it only seems to be used in a single place in the editor, which is also undocumented.

Internally Urho uses URHO3D_ACCESSOR_VARIANT_VECTOR_STRUCTURE_ATTRIBUTE for two classes: StaticModelGroup and SplinePath.

They seem to use VariantVector to store known ID type + size count as the first element, which doesn’t make sense (just use a vector<T> of the known type).
Is it because serialization doesn’t support vector or something?

To me it seems this kludge should be completely removed and replaced with a proper solution.
If meta info suppose to replace it, it still doesn’t solve the underlying kludge that caused it in the first place.

5% slower in release mode consistently is not negligible for such a core feature

So, it’s the problem of accessor attributes introduced six years ago. Accessor attributes were designed to make code more simple and safe despite some performance loss. Do you think it was bad decision? Why nobody cared about it for six years?

should not be changed just for so called safety at the price of performance

Undefined Behavior must not be used. Consider this as axiom.

Think the opposite: there is no reason to use unsafe code unless it is noticeably faster.
Now the difference is too small to care about. Note that random performance fluctuation is much bigger than performace loss. So, performace loss is negligibly small.

100 tries, test duration measured in microseconds.

You should label charts.

Also there are benchmarking frameworks for getting statistically sound results.
https://github.com/nickbruun/hayai
https://github.com/DigitalInBlue/Celero
https://github.com/google/benchmark

What do you call ‘proper solution’? Metadata is designed to be something that is not used by the engine core.

stores names of members encoded into a VariantVector?

It stores display names of VariantVector elements.

On the performance of attribute animations, regardless of whether set accessor or offset access is used, the path to getting the value into the intended destination is complex and ugly. If there are 100’s or 1000’s of scene objects which need to animate this way, so that the time taken to apply the animations is significant, then optimizing to an explicit update could be beneficial. Also note the call to ApplyAttributes() which is necessary according to how the attributes work and how they may have late-applied side-effects. In most cases when you know what you’re animating, it should be unnecessary, but the animation system cannot decide that.

What I understood from:

I just wanted to add attribute metadata at some point and get rid of kludge named ‘vector structure elements’.

is that metadata suppose to replace vector structure elements .
What I’m saying is to begin with vector structure elements was added to handle a kludge of using VariantVector instead of vector<T> .
Solving original vector kludge -> vector structure elements isn’t needed -> nothing to replace.

Extra name info is only used by the editor, for displaying things.
That means that the naming info can be implemented in a way which is completely external to the engine.

Or just display vector<T> without naming each element entry, just indenting and/or with some default name like “element”, so no extra naming info is needed. Though that might degrade UX.

Or just display vector without naming each element entry, just indenting and/or with some default name like “element”, so no extra naming info is needed

It sux.

That means that the naming info can be implemented in a way which is completely external to the engine

It was implemented inside an Editor year ago. If you need to hack Editor code to make the component attributes work, the architecture sux.

Solving original vector kludge
How to do it? What’s T for BillboardSet?

I was reluctant about this, especially regarding if this will cause loss in performance. However, this statement:

want to give this a benefit of doubt.

@cadaver May you elaborate how Constraint attributes work?
There is Position attribute. When it is set via serializable interface, otherPosition_ is updated. When it is set via SetPosition , otherPosition_ is untouched.
What’s the point?

Also it’s used by BillboardSet in which each billboard has position, size, uv, color and other parameters. So, vector structure elements are designed for cases such as BillboardSet billboards.

With BillboardSet, you got a vector of Billboard 's, T is Billboard .
I didnt mean just display “count”.
For something like StaticModelGroup, you have NodeIDs, so it will say something like “NodeIDs count/size”.
We’re suppose to be serializing vector<T> as an attribute here, so it should already have a name. We also don’t just serialize some random set of unnamed attributes for each element, we’re serializing T. Information about T’s attributes should be provided, including their names.
So it’s more like a nested type attributes thingy. And there’s no need for editor-side stuff.
A good example for nested type serialization thingy is cereal library . Now that we can use C++11 we could do something similar.

Looking at Urho I see that the AttributeInfo type is limited to the hardcoded VariantType 's. That means that adding “nested type serialization” support will require a refactor to support any type. I’m assuming other things rely on attributes to use Variant , so it may also require further refactoring of other code to be more generic.
The rabbit hole goes deeper. Perhaps it will be better to find the most low level part that requires refactoring and start from there, and work our way up to attributes. To me it seems to be usage of Variant where generic templates could’ve been used.

I don’t have a full overview about what attributes are used for, and what they require to be able to do. If I’m missing something let me know (documentation’s welcome!).

@KonstantTom Thanks, forgot to search for the MIXED macro version. This post also addresses your point.

void AttributeAnimationInfo::ApplyValue(const Variant& newValue)
{
Animatable* animatable = static_cast<Animatable*>(target_.Get());
if (animatable)
{
animatable->OnSetAttribute(attributeInfo_, newValue);
animatable->ApplyAttributes();
}
}
If attribute are to be redesigned and improved, I think it should take this use case into consideration. Each update will need to set the attribute by using a Variant, while the type information of the variant is already known when creating the animation. That actually waste a lot of execution time. Can this variant be taken away at compile time, say, by using template?

We couldn’t. We will never be able to mix templates and polymorphism. Unless we drop all virtual cascades and do serialization in the header, we cannot perserve real type in any way and get rid of Variant. And it’s not the point of this topic because of it.

We often lose execution time to make code usage convinient. The more generic engine is, the more performance is lost.

I think the solution provided is relatively clean. The mixing of macro and method chaining is a bit on the ugly side but should do.

The performance test appears to have been done only for saving ? How relevant is that performance test on the average run-time of a game?

Yes. The type is known at compile time. AttributeAnimationInfo should be templated to animate T and use
ApplyValue(const T& newValue) . The question is what else needs to be refactored.

Can you better explain what’s the problem with Cereal’s approach?
The topic started with trying to fix code smell. I think it’s better to dig deep, find the root cause, and fix it.

This is wrong.
Again, look at Cereal - much simpler and convenient code usage than Urho’s serialization, without losing any execution time, in fact it’s even faster because things are being done at compile time.
And it’s more generic than Urho’s serialization - it can handle any type.

You’re not trading off execution time for usage connivance.
You’re trading off proper problem solving and design for an easier problem (which is not inherently a bad thing, common practice when considering resource constraints).

If anyone which is familiar with Attribute and/or Attribute animation can give me complete requirements for what they need to technically do (instead of me guessing and missing stuff by looking at code), I could try to prototype a system that uses templates to see if it’s feasible.
Meanwhile I can start prototyping based on what I already understand.

In general documentation for developers could be nice - overviews of how systems in the engine are designed, what problem they’re trying to solve and how. It’s much faster and safer than trying to guess and assume based on code.

I will vote for this try.

So for now I can prototype:

• serialize attribute of any type, using compile time information.
• animate attribute value, using compile time information.

AttributeAnimationInfo should be templated to animate T and use
ApplyValue(const T& newValue). The question is what else needs to be refactored

It will give you exactly nothing. ValueAnimation is the Resource that stores Variant-s. You will have to cast this variant somehow before applying it to the attribute. Now this cast is performed inisde accessor. If you templatize anything, you will just make the architecture more compliacted and move this cast at the level of AttributeAnimationInfo or ValueAnimationInfo. You will not get any benefit unless you templatize ValueAnimation Resource itself.

Can you better explain what’s the problem with Cereal’s approach?

Just compare the syntax of Cereal and Urho. I can’t imagine how to match it without breaking the API and resource compatibility.

Again, look at Cereal - much simpler and convenient code usage than Urho’s serialization

And much less powerful.

If anyone which is familiar with Attribute and/or Attribute animation can give me complete requirements for what they need to technically do (instead of me guessing and missing stuff by looking at code), I could try to prototype a system that uses templates to see if it’s feasible

My current experience tell me that it’s not feasible. Of course, you could try. Here are requrements, good luck:

1. Don’t break Serializable API, including these macros. Urho and user components shall work without changes.
2. Don’t break compatibility of serialized XMLs and Jsons;

I suppose it’s ok to break OnSetAttribute hooks (8 Urho components used it). It is smelly hack and have to be removed at some point.

backward compatibility isn’t a requirement for what the system suppose to do.
I mean things like “be able to add value animations dynamically”.

Value animation will require complete rethinking. That’s why I suggested earlier to start refactoring from the bottom - so you’ll know what you’re building on.

Did you read the comment?

    // Convenience for editing static constraints: if not connected to another body, adjust world position to match local
    // (when deserializing, the proper other body position will be read after own position, so this calculation is safely
    // overridden and does not accumulate constraint error

It’s somewhat of a kludge. Feel free to remove or refactor.

It is a requirement for Urho API (with exceptions listed in poritng notes)

That’s why I suggested earlier to start refactoring from the bottom - so you’ll know what you’re building on

Refactoring != breaking anything and building from scratch. What’s the point of such changes? Especially when benefit is so impalpable. You won’t win any FPS. You will win few milliseconds for binary serialization. And you will lose all users, beacuse (I suppose) such PR will never be merged.

Okay, here are functional requirements:

1. Of course, The System shall be able to serialize variant attributes of the object. Either all attributes simultaneously or one-by-one.

2. The System shall store and provide access to attribute information: attribute name, enum names, default values, attribute flags and metadata.

3. The System shall allow to iterate over attributes.

4. The System shall support attributes with custom setters and getters.

5. The System shall support XML, Json and binary formats.

6. The System shall support user-friendly enums for XML and Json.

Why did you save enum names? I think, enum names can be added as metadata too.

Enum names are important for Urho core mechanisms. I don’t want to use metadata for obligatory attribute parameters. However, I may change my opinion later…

Did some initial prototyping for generic serialization.
Note that it’s a simple prototype showing generic serialization(only) implementation, and not drop-in replacement for Urho’s attributes.

Simple introduction example for variadic templates usage .
Example of generic serialization to file .

Will you try to make your serialization meet at least these weak requirements?

That’s what we need to fix.

You’re confusing serialization and other features related to attributes.
Serialization is just about reading and writing, it isn’t responsible all the other things u listed.
Serialization can use setters and getters, you can specialize the type’s serialization for input and output, and call set / get in each.

And the core problem is the use of variant, which create limitations that lead to kludges (already explained how).
Naming, default values, etc don’t use variant therefore there’s no problem to fix here.

There are only 2 things that I know of that use variant - attribute serialization and animation.

I meant, serialize types supported by variant.

You’re confusing serialization and other features related to attributes
Serialization is just about reading and writing, it isn’t responsible all the other things u listed

Urho don’t need plain serialization like cereal per se.
Urho need attribute system that allows to treat object as group of named and parameterized attributes.
Serialization is just an usage of this attribute system.
Have you tried to prototype such attribute system without Variant? It’s hard for me to even imagine it.

It’s trivial.
First of all in the example I automatically generate serialization for arithmetic types, that includes all the kinds of integers and floats.
I updated the example with a Vector3 class. Serializing it is this simple:

template<typename Stream>
void serialize(Stream& f, Vector3& value) {
	read_or_write(f, value);
}

We don’t need to completely replace all of Urho’s attributes system. Just get rid of the use of variant where it causes limitations and can be replaced with a better solution.

Variant is the basement of Urho attributes. How do you replace it with templates?

In the Tundra project we had attributes with templates / subclasses.

Not necessarily prettier. Another part of using Variant is that you gain freeform data for scripting.

• Identify what are the problems Variant is used for.
• Prototype a better solution in isolation so you don’t have to deal with external complexity.
• Integrate the better solution to replace the previous solution.

Again, I only came across variant being used for serialization and animation, and in both cases I don’t see any necessity to use variant instead of generic templates.
So the next step is to prototype a generic solution for attribute animation. That may be more complicated since animations are dynamically added.

Later on we need to see how they’re integrated into Urho - that may lead to more refactoring as other code that interacts with the previous systems uses variant.

If we make hierarchy like in Tundra
AttributeInfo <- AttributeInfoImpl

1. AttributeInfo shall have virtual Save/Load XML/JSon/Binary for serialization (PImpl-ed);
2. AttributeInfo shall have virtual SetVariant to work with ValueAnimation resource (just postpone the problem for now);
3. We shall expose all AttributeInfoImpl to AS and update Editor code.

Here is one more important change.
I replaced attribute accessor with function pointers and used lambda for each attribute macro.

Pros:

• Removed AttributeTrait , MixedAttributeTrait and the whole hierarchy of AttributeAccessor ;
• Removed one virtual call on attribute set/get;
• Things are more generic now, accessors don’t have to match the exact type anymore
• So, URHO3D_MIXED_ACCESSOR_ATTRIBUTE is deprecated (not removed, of course), use URHO3D_ACCESSOR_ATTRIBUTE instead.

Cons:

• ??? things have changed, maybe

Getting rid of Variant in attributes will make script API more complicated and cause a lot of work, so I am not going to work on it. Despite the fact that serialization could became faster without Variant casting.
You could try to do it on your own if you have enough passion.

Instead of function pointers you can use template functors. It will allow inlining lambdas and functions. Example

How will it make things more complicated?

This is just a guess.
Variant is simple. It’s plain type, that is easy to pass to function, return from function or facade to script API. Once you replace straightforward value type with virtual hierarchy, it causes wide scale refactoring. Try to get attributes from the Tundra and fit them into Urho-s ScriptInstance .

I cannot because AttributeInfo is non-template.

It isn’t a simple type, it’s like a union. You have to manually handle each case for each type.

Why use virtual hierarchy as a replacement?

Of course it would cause wide scale refactoring.
If we can’t find a way to fix things without causing performance regression, I suggest dropping this issue for now.
The current attributes system isn’t broken. It’s better to focus on something else.

Probably I misunderstood something. Where do you see performance regression?

earlier you reported your fix reduced performance

My fix is unrelated here.
In release mode accessor attributes are negligibly slower than offset attributes probably due to two extra pointer-to-function calls. Negligibly means that measurement error is greater than the difference.

I can not get the idea of better attribute refactoring out of my head, btw…

I have to agree with Enhex on this. If ain’t broken then don’t fix it. Or perhaps don’t do it now until you have more overview or more comfortable with the code base.

At a quick glance on the last commit in the master branch, I don’t feel it has went through all the basic use cases yet. I could be wrong though, but it looks like the refactoring work is incomplete as it has only done on those cases where the Serializable::OnSetAttribute() are being overridden. Also, it does not feel that right (at least to me) to have so many almost identical accessor methods. It bloats the code base unnecessarily.

I think the last commit should be reverted back first and keep it in a new topic branch. The work can be continued in the branch if there is really such need. And when the whole refactoring is done then a pull request is made to ask the lib users to evaluate, before actually merging. I think Lasse would do it like this for fundamental change.

At a quick glance on the last commit in the master branch, I don’t feel it has went through all the basic use cases yet. I could be wrong though, but it looks like the refactoring work is incomplete as it has only done on those cases where the Serializable::OnSetAttribute() are being overridden.

Commit in the master is not heavily tied to this attribute refactoring, I wanted to do it anyway.
If the Component need an action on attribute set, it shall explicitly state this action.
if-cascades in OnSetAttribute are a bit too ugly.
I could (ok, you could ) revert this commit if you don’t like the implementation, but I don’t want to keep attribute hooks in OnSetAttribute . They looks like they came from the pre-accessor epoch when all logic had to be implemented inside OnSetAttribute .

I believe all the related commits for a fundamental change should go in together as a whole instead of piecemeal. The last commit appears to be a small part of the whole work to me.

Now I have an idea how to get rid of the hooks without such amouth of methods…
I reverted last commit. It was big and unpleasant, I agree.

The accessor hooks are / were a tool, because they were available, they got used, for better or worse. Whatever was most convenient.

In this case the price of removing them is the introduction of (more) functions needed only for the manipulation of attributes. I don’t see a way around this, at least immediately. Ideally the attribute accessors would be just the same as the component’s public API, but in case of ID attributes and such, it needs those “fictional” functions just for the attributes. A “deeper” solution might transform pointers to / from IDs and resource refs directly.

I see one more problem here. Public API do the work immediatelly, but attribute setters postpone heavy actions for ApplyAttributes. E.g. CollisionShape::SetShapeType updates shape and rigid body, but attribute accessor just set the recreateShape_ = true flag. This is also dirty: two separate mechanisms of doing the same work.

For example, the action could be explicitly specified in the attribute declaration. So it would be explicitly stated that attribute A setup cause MarkSomethingDirty, attribute B setup cause MarkSomethingElseDirty and attribute C setup cause ValidateThisThisAndThis.

However, it doesn’t fix generic problem with GetSmthAttr+GetSmth pairs.

I’m talking idealistically now, so in practice any changes might be unfeasible or cause performance loss, but it seems a bit of an antipattern that there even is a difference between the public API and attribute access, in regard to this “late” setting of heavy state. After all, if you’re instantiating something purely in code, you’re going to make several public API calls to set the state anyway.

For example collision shape recreation or other heavy operations could be made to happen on demand before next Bullet update if needed, etc. regardless of which route the change was made.

But of course, this is just daydreaming, there’s surely a lot of improvements to Urho that could be made that are more important than this.

Just my 2 cents
Since you already moved to C++11.
Take a look at the following:

It looks neat but I wonder about its performance. As we all know the reflection approach is usually slower.

However, there are semi-optimal solutions that both not very heavy and and help to avoid public API bloating.

Getter and setter could be written as lambda function inside RegisterObject even now, but it’s not the shortest and the simplest solution.

It migth be better to specify epilogue (post-set) action in the non-accessor attribute declaration. Such changes will require some design effort, but the attributes will be more straightforward than with OnSetAttr hooks. I’ll try to implement this.

Someone recently pointed me to awesome lib that may be relevant in this case. Maybe you people should check it out: http://www.rttr.org/

I have to go back to this task, so here is new revision.
https://github.com/urho3d/Urho3D/pull/2091
I temporarily added some code in HelloWorld to test things, you could check it for example.

Important changed:

• offset_ is removed, all attributes are working via accessors now.

• So, identifying attribute inside OnSetAttribute by offset doesn’t work anymore.

• I added URHO3D_ATTRIBUTE_EX and URHO3D_ENUM_ATTRIBUTE_EX that call specified member function when attribute is set (similar to accessor attributes, but this function doesn’t have input arguments). Metadata or attribute name could also be used to identify the attribute, but I decided to avoid this for now. Could be changed.

• I replaced all URHO3D_*_FREE functions with URHO3D_CUSTOM_ATTRIBUTE and URHO3D_CUSTOM_ENUM_ATTRIBUTE that work with any getter/setter functional objects. Note that these new functions works with Variant type due to performance reasons. Could be changed later.

Benefits:

• No UB in attributes anymore!

• No extra calls (or even less calls) comparing to old version. Every attribute is set or get via the following chain of calls: AttributeAccessor::Set(virtual)->[internal lambda](probably inline)->member access or setter/getter call

• No extra memory allocation even for lambda getters/setters

• No extra copying. Attribute actions are performed directly with Variant came from Serializable code.

• Exact type matching is not required anymore. It’s enough for member attribure to be convertible to corresponding Variant type. Same for getters and setters, no need in mixed accessors anymore.

• No more tons of accessors and traits. Compiler resolves all type stuff on its own.

• Any lambda could be passed as attribute accessors. Note: this lambda should work with Variant directly.

• Attribute with explicit post-set action is more straightforward than custom logic in OnSetAttribute.

Disadvantages:

• Old offset-based attribute identifying doesn’t work anymore.

• Old type hacks like for Light cascade splits don’t work enymore.

• Someone may dislike macro magic used to make lambdas. Probably will change it a bit…

• Attribute identifying may be considered less readable. Feel free to discuss.