traverse() and parameters_changed() enhanced

[Speierers]

Overview

The primary goal of this PR is to enhance the traversal mechanism to prevent unnecessary updates of the object parameters.

This is achieved by adding an optional argument to the parameters_changed(), carrying the list of names (a.k.a. keys) of the object's attributes that have being modified since the last call to this function.

These names correspond to the names given in the traverse() method.

When keys is empty, it should be assumed that any parameters might have been modified.

The implementation of the parameters_changed methods in every plugin can then be fine-tuned to avoid unnecessary computation when only a subset of the attributes have changed. For instance, we shouldn't recompute the optix BVH every time a mesh's BSDF reflectance value is modified (children trigger parameters_changed() on their parents).

keys might also contain the key "parent" (we could define a more "unique" key for this) which specifies that parameters_changed() has been called from a parent object. E.g., when a shape's geometry is modified, the attached area emitter (if any) should be notified so it can recompute some internal variables (here m_inv_surface_area).

Remaining questions

1. Objects defined at the root of the XML and later referenced will only be children of Scene and not children of the objects they are referenced by. This might cause some issues as "parents" might not be updated properly.
2. Should we be able to flag some parameters as const in traverse()? In which case ParametersMap.__set_item__() would throw an error / warning.

TODOs

• Scene should upate its bounding sphere when some geometry is modified

[wjakob]

Maybe "Explicitly register this shape as the parent of the provided sub-objects (emitters, sensors, etc.)"

[wjakob]

This looks great -- one question I am wondering about is what happens when one of the changed parameters is a sub-object? Is this just handled using the (potentially auto-generated) ID?

The following is worthy of some discussion:

keys might also contain the key "parent" (we could define a more "unique" key for this) which specifies that parameters_changed() has been called from a parent object. E.g., when a shape's geometry is modified, the attached area emitter (if any) should be notified so it can recompute some internal variables (here m_inv_surface_area).

IIRC the current python code calls functions from child to parent. If there are other update directions that must be considered, I think it would be better not to go through the parameters_changed interface but rather have some alternative API (like set_shape or s.th. like that, which would address the use case you mention). This way, we can also avoid potential future issues with infinite recursion when parameters_changed can go both up and down in a tree.

[Speierers]

one question I am wondering about is what happens when one of the changed parameters is a sub-object?

In this case keys will contain the same string as the one provided in the traverse(). For instance, if a bsdf is modified, the corresponding shape's parameters_changed() function will be called with keys = ["bsdf"].

Does that answer your question?

[Speierers]

IIRC the current python code calls functions from child to parent. If there are other update directions that must be considered, I think it would be better not to go through the parameters_changed interface but rather have some alternative API (...)

I first used the set_shape method of the emitter (or Mesh::set_children()) in the shape's parameters_changed method to notify the children of an update. The reason why I didn't stick to this method is that it may have other purposes (e.g. in area.cpp it ensures the light is only bound to a single shape). Moreover I found it cleaner to handle everything related to updates in a single function (parameters_changed()).

In the current implementation, the convention is that the python ParametersMap updates from child to parent, and parameters_changed() can only update the internal state of an object or it's children. As parameters_changed() can't notify an update from child to parent itself, there is no risk of infinite recursion. Although I agree with you this assumption is quite fragile as the design itself doesn't really prevent that.

At some point I was thinking of a more generic solution, where the Object class would store the list of children (rather than having it explicitly in the plugins), and we would add the following methods:

• Object::set_parent(..): called in the parent 's constructor to assotiate parent with a child
• Object::update_children(): called in the parent's parameters_changed() to notify children
• Object::parent_changed(): called by the parent's update_children() method for every child

At then end, I realized that this relation parent->children only really happens a couple times in the entire codebase, so I decided to go with the simpler solution of using a "parent" keyword.

Truth is, in the future, as the codebase grows it might be necessary to handle this properly.

Happy to discuss this futher..

[wjakob]

This looks awesome, extremely minor comments at this point.

[wjakob]

wrong indentation?

[wjakob]

I love auto, but let's reserve its use for complicated types like std::some_complex_stl_type<>::iterator and spell out types explicitly in all other cases to aid readability. No big deal in this case, but I thought I'd point it out for the future.

[wjakob]

nice!