Details

Reviewers

Maniphest Tasks

Commits

rCd3720f188c52: Add update flags to node and sockettype
rB626201683ec0: Cycles: add update flags to Node and SocketType

Summary

Those flags are meant for detecting which socket has changed, so in the
future we can have more granular updates.

Node now stores an update_flags member which is modified every time
a socket is changed though Node::set. The flags are or-able bits
stored in SocketType instances. Each SocketType stores a unique bit
out of 64, for the 64 bits of an uint64_t; the bit
corresponds to the index of the socket in the Node's sockets array +
1, so the socket at index 5 will have the 6th bit set as its flag. This
limits us to 64 sockets per Node, which should be plenty for the current
set of Nodes that we have.

This does not change the behavior of other parts of Cycles.

This is part of T79131.

Diff Detail

Repository: rB Blender

Event Timeline

Kévin Dietrich (kevindietrich) requested review of this revision.Aug 19 2020, 11:32 PM

Kévin Dietrich (kevindietrich) created this revision.

Harbormaster completed remote builds in B9664: Diff 27943.Aug 19 2020, 11:33 PM

Kévin Dietrich (kevindietrich) added inline comments.Aug 19 2020, 11:40 PM

intern/cycles/graph/node.h
109	This is `need_update_` (with an underscore at the end) because other nodes have a `need_update` (with no trailing underscore) member which conflicts, it can be renamed or we can rename the members and remove the trailing underscore (the `need_update` members will be replaced by the flags anyway at some point in the future).

Brecht Van Lommel (brecht) mentioned this in T79131: Cycles/Blender API redesign.Aug 21 2020, 3:45 PM

It would be good to see one node updated to use this system as an example.

intern/cycles/graph/node.cpp
37	This could set all bits, indicating that everything needs to be updated after the node has been created. We could also leave this to the individual node types, but I'm not sure it's needed.
79	We should add code here comparing the new and old value, so that if nothing changed then nothing gets detected as modified. I guess the overhead of this will be small, and to keep first time export fast we can check if the update flag bit is already set and then avoid the comparison for arrays that might more costly to compare.
689	mark_processed -> clear_modified
intern/cycles/graph/node.h
104	No need to explicitly use `inline`.
115	Define it like this, making clear this is per socket and ensuring every place use the same amount of bits in case we want to increase it later: typedef uint32_t SocketModifiedFlags; SocketUpdateFlags socket_modified; We also use "modified" rather than "changed" in some places already, I'd remain consistent with that naming. Also I suggest "modified" rather than "update" since the node base class only tracks what has been modified, and what that then means in terms of updates is up to the specific node implementations and managers.

changes from review
modify the Camera as an example to demonstrate how update flags could replace the current tagging mechanism

Harbormaster completed remote builds in B9788: Diff 28152.Aug 25 2020, 11:08 PM

I increased the size of the flags to 64 bit as apparently I was in release build and the assertion inputs.size() < 32 was not triggered. The Camera class already has 43 sockets, and the Integrator 34.

I also modified the Camera class as an example (maybe you wanted it in another patch?), which removes the methods to check if the camera was modified based on a copy of its previous pre-synchronization data. Furthermore, Camera::type was renamed Camera::camera_type as it conflicts with Node::type in my macros to define getters and setters.

This change also reveals a few issues, one of them is about an infinite loop in interactive rendering, and the other is more an API issue.

For the infinite loop, the problem has to do with data synchronization between samples in interactive rendering, where the width and the height of the Camera are changed in turn by the Blender exporter and the Session. The Session changes it to support refined rendering, and the Blender exporter resets it, which keeps tagging the Camera as being modified, which keeps resetting the Scene (through Scene::need_reset). As a quick fix, I removed the modification of the width and height from the Blender exporter. It may not be the right fix, but it is quite useless to have that since we modify those properties no matter what in the Session, and it is the simplest one I could find. It's worth noting that I got such infinite loops with other nodes (like the Mesh or the Light) because the Blender exporter keeps modifying the data, but those ones are avoided by checking that the value has changed before updating the flag.

The other issue has to do with using the sockets and structures not known to the type system. For example, Camera::viewplane is a BoundBox2D structure, but there is no socket type for it, rather we have sockets for individual members of the structures. To be able to tag for a modification when changing such properties, we need to modify the individual members (somewhat defeating the purpose of the structure). I think we could add some sort of STRUCTURE socket type allowing to define a socket for structures (we would still need to create sockets for individual members of those structures). This would need to record some extra data (like struct size or struct name) to assert that the type is the expected one.

intern/cycles/graph/node.h

119–123

Those functions are here to avoid typo errors while writing redundant code, and basically implement the logic to set the value, replacing the body of Node::set. I think Node::set could be a template instead, we can use some template magic to handle the runtime assertions, e.g. :

template <typename T>
struct SocketDataTypeInfo;

template <>
struct SocketDataTypeInfo<int> {
    static bool is_value_of_type(SocketType::Type type)
    {
        return type == SocketType::INT;
    }
};

template <>
struct SocketDataTypeInfo<float3> {
    static bool is_value_of_type(SocketType::Type type)
    {
        return type == SocketType::COLOR || type == SocketType::POINT ||
               type == SocketType::VECTOR || type == SocketType::NORMAL;
    }
};

template <typename T>
void Node::set(const SocketType &input, T value)
{
    assert(SocketDataTypeInfo<T>::is_value_of_type(input.type));

    if (value != get_socket_value(this, input)) {
        return;
    }

    get_socket_value(this, input) = value;
    socket_modified |= input.modified_flag_bit;
}

Those SocketDataTypeInfo could also be used to have type safety for getting or setting structures in one go if we choose to have structure sockets.

Kévin Dietrich (kevindietrich) edited the summary of this revision. (Show Details)Aug 25 2020, 11:33 PM

In D8644#212428, @Kévin Dietrich (kevindietrich) wrote:

For the infinite loop, the problem has to do with data synchronization between samples in interactive rendering, where the width and the height of the Camera are changed in turn by the Blender exporter and the Session. The Session changes it to support refined rendering, and the Blender exporter resets it, which keeps tagging the Camera as being modified, which keeps resetting the Scene (through Scene::need_reset). As a quick fix, I removed the modification of the width and height from the Blender exporter. It may not be the right fix, but it is quite useless to have that since we modify those properties no matter what in the Session, and it is the simplest one I could find. It's worth noting that I got such infinite loops with other nodes (like the Mesh or the Light) because the Blender exporter keeps modifying the data, but those ones are avoided by checking that the value has changed before updating the flag.

That seems reasonable, if width and height are not treated as sockets but rather private members set by Cycles itself.

The other issue has to do with using the sockets and structures not known to the type system. For example, Camera::viewplane is a BoundBox2D structure, but there is no socket type for it, rather we have sockets for individual members of the structures. To be able to tag for a modification when changing such properties, we need to modify the individual members (somewhat defeating the purpose of the structure). I think we could add some sort of STRUCTURE socket type allowing to define a socket for structures (we would still need to create sockets for individual members of those structures). This would need to record some extra data (like struct size or struct name) to assert that the type is the expected one.

I'm not sure this is necessary, adding the individual members of the viewplane and boundbox seems good enough to me, I would not complicate the node type systems for those cases at least.

intern/cycles/graph/node.h
33	I wouldn't add `public` as part of the macro, since it affects further struct members in a hidden way.
34–48	Constructing a `ustring` and calling `find_input` for every `set` or `get` seems too slow to me. Maybe just using `static const SocketType *socket` is enough? It would only need to be initialized once for each socket type, and checking if it has been initialized afterwards is probably done pretty efficiently by the compiler. I would then also put static variable in a shared function, so we don't have 3 copies.
109	Maybe rename `need_update_()` to `is_modified()`? Just to keep the naming consistent.

changes from review

Harbormaster completed remote builds in B9878: Diff 28303.Aug 30 2020, 12:58 AM

Looks good, just one minor comment.

intern/cycles/render/session.cpp
921–926 ↗	(On Diff #28303)	`resolution` should also not be a public struct member, and setting width/heigh/resolution and tagging update could be wrapped in a function rather than using `friend class Session`?