Thanks for all the comments. I'll split out the alembic code into it's own file and make the style notes you suggested. Is there a style guide online for Cycles developers?

r.e. The ProceduralNode comment, is this to help make alembic more of a first-class entity within Blender? I hadn't really thought of that much. For now why don't I reorganize my code a bit to make it more general purpose and not so tied to cycles_xml.

w.r.t Materials. I used the <state shader=""> syntax (was a little tricky to code!) simply because that was the current style and I wanted to be consistent. Certainly something like this is easier:

<alembic file="file.abc">
      <object path="*"  shader="redMtl" />
      <object path="/some/path/obj*" shader="blueMtl" />      // Override some objects
</alembic>

Is there some way I can replace the diff associated with this ticket?

In D3089#73134, @Brecht Van Lommel (brecht) wrote:

Thanks for the explanation!

Maybe it's beyond the scope of this patch, but it's not actually that complicated. It would be best if there was a ProceduralNode base class, that would have a virtual void generate(Scene *scene) = 0; method. The scene would have a list of such procedural nodes, and before rendering call each of them to create nodes.

So then you'd have an AlembicNode with e.g. filepath and filter string sockets. The advantage of this would be that it's extensible with more procedurals, and file read/write would not need to know anything about Alembic. Then it's also easier to use another file format, send it over a network, inspect, etc.

<alembic filepath="myfile.abc" filter="path/to/object/*"/>

Material binding is also a complex topic on it's own, but perhaps the default way to do it should be to use the shader name from AbcMaterial? If you for example have a complex scene with many instances it's not so practical to sort them by material to put a <state> tag around. Ideally it's also possible to replace the .abc file with one from another frame, with perhaps some objects added or removed, and still have the XML file be valid. Having a way to override the shaders from the file could be good though, but maybe not with the <state> tag.

In D3089#73123, @Bob Mercier (bobmercier) wrote:

The "correct" way to do this is to lazy-load the alembic file, like other renderers do. The XML file would contain bounding box data and, after the first ray hits the bounding box, the alembic file would be loaded. I know nothing of cycles internals so don't have any idea how difficult this would be.

We don't support this, but note some renderers stopped doing this or don't advise to do it anymore. In practice global illumination rays fly all over the place and you end up loading everything when rendering the first pixels, which is slower than reading the meshes ahead of time.

Some minor comments left in the code, did not look in detail.

In D3089#73144, @Bob Mercier (bobmercier) wrote:

Is there a style guide online for Cycles developers?

We mostly follow the Blender code style, main difference is we don't use a space between if(.
https://wiki.blender.org/index.php/Dev:Doc/Code_Style

If in doubt look at surrounding Cycles code.

r.e. The ProceduralNode comment, is this to help make alembic more of a first-class entity within Blender? I hadn't really thought of that much. For now why don't I reorganize my code a bit to make it more general purpose and not so tied to cycles_xml.

It's not so much for Blender, we already have Alembic integration there and probably will continue to load Alembic files through that to ensure consistent results in the viewport and final render.

Having it as a node is about making Cycles itself easier to extend and maintain. If Alembic is not tied to XML files it's easier to change file formats, and also usable in IPR for example. Here is some code to implement the basics: P628. This should be enough to support Alembic in Cycles standalone.

Is there some way I can replace the diff associated with this ticket?

There's some explanation here, basically you upload the diff again and attach it to this revision. Or you can use arc o do it all from the command line.
https://wiki.blender.org/index.php/Dev:Doc/Tools/Code_Review#Upload_a_Diff

Is there some way I can replace the diff associated with this ticket?

There's some explanation here, basically you upload the diff again and attach it to this revision. Or you can use arc o do it all from the command line.
https://wiki.blender.org/index.php/Dev:Doc/Tools/Code_Review#Upload_a_Diff

I tend to use the 'Update Diff' function in the upper right corner of this page, you pretty much paste in a new diff and attach it to this revision.

In D3089#73147, @Ray Molenkamp (LazyDodo) wrote:

I tend to use the 'Update Diff' function in the upper right corner of this page, you pretty much paste in a new diff and attach it to this revision.

Ah, much easier, I updated the wiki page to say that.

In D3089#73149, @Brecht Van Lommel (brecht) wrote:

In D3089#73147, @Ray Molenkamp (LazyDodo) wrote:

I tend to use the 'Update Diff' function in the upper right corner of this page, you pretty much paste in a new diff and attach it to this revision.

Ah, much easier, I updated the wiki page to say that.

might not hurt to update the wiki with a basic arc workflow, last time i tried to use it to update a patch, it submitted a new one without asking, haven't trusted it since , but we're getting awfully off topic here.

I think nested object tags can work, but it should be in a way that can be represented by the nodes, like this:

class AlembicObject : public Node {
    ...
};

class AlembicProcedural {
    ...
    array<AlembicObject*> objects;
}

NODE_DEFINE(AlembicObject)
{
    NodeType* type = NodeType::add("alembic_object", create);
    ...
}

NODE_DEFINE(AlembicProcedural)
{
    ...
    SOCKET_NODE_ARRAY(objects, "Objects", AlembicObject::node_type);
}

We haven't used this so far by I want to do it for render passes as well. The thing I'm not entirely sure about is the syntax in the XML file, since it's not a natural fit. The most verbose syntax would be like this:

<alembic filepath="file.abc" frame="5">
      <objects>
          <alembic_object path="/pTorus1/*" shader="blueMtl" />
      </objects>
       ...
</alembic>

But we could leave out the <objects> tags for brevity, if we ever need to distinguish between two arrays with the same object type we could require then. The only difference then would be <alembic_object> instead of <object>.

I also noticed there is a bug in the code for SOCKET_NODE_ARRAY that can be fixed like this:

--- a/src/graph/node_type.h
+++ b/src/graph/node_type.h
@@ -217,7 +217,7 @@ const NodeType *structname::register_type()
 #define SOCKET_NODE_ARRAY(name, ui_name, node_type, ...) \
        { \
            static Node *defval = NULL; \
-               assert(SOCKET_SIZEOF(T, name) == sizeof(Node*)); \
+               assert(SOCKET_SIZEOF(T, name) == sizeof(array<Node*>)); \
                type->register_input(ustring(#name), ustring(ui_name), SocketType::NODE_ARRAY, SOCKET_OFFSETOF(T, name), &defval, NULL, node_type, ##__VA_ARGS__); \
        }

I've made most of these changes, and am testing. I started adding support for alembic Curves, adding them as Mesh curves, then noticed the curve.h/curve.cpp files in the render/ directory. This seems to be a more complete hair/fur system but it doesn't seem like the api is fully exposed in the standalone version of cycles yet. Is this correct? What's the best way to go about importing curves? Generally speaking, the curve control points should be treated as catmull-rom spline, interpolating the first and last control points and being either quadratic or cubic smooth in between.

In D3089#73247, @Brecht Van Lommel (brecht) wrote:

I think nested object tags can work, but it should be in a way that can be represented by the nodes, like this:

class AlembicObject : public Node {
    ...
};

class AlembicProcedural {
    ...
    array<AlembicObject*> objects;
}

NODE_DEFINE(AlembicObject)
{
    NodeType* type = NodeType::add("alembic_object", create);
    ...
}

NODE_DEFINE(AlembicProcedural)
{
    ...
    SOCKET_NODE_ARRAY(objects, "Objects", AlembicObject::node_type);
}

We haven't used this so far by I want to do it for render passes as well. The thing I'm not entirely sure about is the syntax in the XML file, since it's not a natural fit. The most verbose syntax would be like this:

<alembic filepath="file.abc" frame="5">
      <objects>
          <alembic_object path="/pTorus1/*" shader="blueMtl" />
      </objects>
       ...
</alembic>

But we could leave out the <objects> tags for brevity, if we ever need to distinguish between two arrays with the same object type we could require then. The only difference then would be <alembic_object> instead of <object>.

I also noticed there is a bug in the code for SOCKET_NODE_ARRAY that can be fixed like this:

--- a/src/graph/node_type.h
+++ b/src/graph/node_type.h
@@ -217,7 +217,7 @@ const NodeType *structname::register_type()
 #define SOCKET_NODE_ARRAY(name, ui_name, node_type, ...) \
        { \
            static Node *defval = NULL; \
-               assert(SOCKET_SIZEOF(T, name) == sizeof(Node*)); \
+               assert(SOCKET_SIZEOF(T, name) == sizeof(array<Node*>)); \
                type->register_input(ustring(#name), ustring(ui_name), SocketType::NODE_ARRAY, SOCKET_OFFSETOF(T, name), &defval, NULL, node_type, ##__VA_ARGS__); \
        }

curves.h has a CurveSystemManager which contains some global quality settings for how curves are rendered, but these are settings that wouldn't be part of an Alembic file I think.

Curves are created in a mesh object mainly. There is an array of curve key points, and an array of curves. Each curve specifies which curve key point is the first point of that curve. So for example if you had curve key points ABCDEFGH and curves with first key 0 and 4, you'd have two curves ABCD and EFGH.

Cycles uses a cardinal spline with tension parameter c = 0.71, inherited from Blender. It might be possible to transform the curve key points to this basis (I think?), if not we'll have to add support in the kernel for some different types of basis.

I couldn't build with this patch. It's not applying cleanly on the latest Cycles repository master, and referencing files named alembic.h and alembic.cpp which are not included?

It seems Cycles is rendering curves as line segments by default, you can change that by setting:

scene->curve_system_manager->primitive = CURVE_SEGMENTS;

We should change that.

Thanks for the updates, it builds for me at least but I did not look more deeply into the code since it's WIP anyway.

You're correct about the way motion samples are stored.

Ideally we should have a more powerful attribute system that can support motion keys in a generic way. It possible to just do it on the XML side for now, with a syntax like <mesh P="..." motion_keys="3">. Then just store a longer array in P with all the motion keys and copy them to mesh->verts and ATTR_STD_MOTION_VERTEX_POSITION as needed.

In D3089#73786, @Bob Mercier (bobmercier) wrote:

Deforming meshes must have an odd# of motion steps. The center set of vertices is copied to mesh->verts.

Sounds good.

The transform node matrix, translate and rotate attributes can have either 1 or 3 entries, corresponding to MotionTransform's pre, mid and post matrices. There are new examples demonstrating the syntax.

Makes sense. Arbitrary number of motion steps for the transform matrix will be in the Cycles repository soon by the way

instead of exposing "curve_key" and "curve_first_key". I believe this is more consistent with the mesh syntax and more intuitive. Let me know if you agree.

Makes sense, I agree.

I forgot to mention that during viewport renders I try to detect if there is a Cycles viewport enabled to avoid loading Alembic data on the Blender side. The approach I am using is not so nice, IMO, not sure of a better solution.

@Sybren A. Stüvel (sybren), setting you as a reviewer here, at least for the Blender side. The main changes are an option to the importer to add a modifier to every objects even if there are not animated so we can detect them through the Cycles Blender exporter, and a "default radius" for curves (and points in the future). The current code is setting an arbitrary radius of 1.0f to the curves if they do not have a widths property in the Alembic file. I guess it's better to parameterize this.

In D3089#243100, @Kévin Dietrich (kevindietrich) wrote:

The main changes are an option to the importer to add a modifier to every objects even if there are not animated so we can detect them through the Cycles Blender exporter

What is the advantage of doing it like this? Is importing data from Alembic in Cycles faster than getting the same data from Blender?

and a "default radius" for curves (and points in the future).

Why is this necessary now, and not before?

In D3089#243171, @Sybren A. Stüvel (sybren) wrote:

In D3089#243100, @Kévin Dietrich (kevindietrich) wrote:

The main changes are an option to the importer to add a modifier to every objects even if there are not animated so we can detect them through the Cycles Blender exporter

What is the advantage of doing it like this? Is importing data from Alembic in Cycles faster than getting the same data from Blender?

I would say it is bit faster to load as we don't go through the RNA, but not so much for static objects as they are not updated frame to frame anyway. There are a few reasons:

• detect objects that have just an animated transforms, as I use the MeshSequenceCache modifier for this. I think I could add a detection through the constraints
• support more features from Alembic as Blender is limiting and limited at the moment: instances, custom attributes, better subdivision support (corner crease, face holes, etc.).
• it would allow to modify those objects in the archive, and have them updated in Blender or Cycles when reloading it, I know that this type of workflow is not so supported in Blender but we would have to start thinking about it at some point

and a "default radius" for curves (and points in the future).

Why is this necessary now, and not before?

I am pretty sure it was necessary before, for convenience to avoid having to manually set the radiuses on all objects. Maybe people don't care or complain. I cannot tell you why I haven't parameterized this when I worked on the importer though.

The bigger picture here is that there is Cycles standalone and integrated into other applications. Supporting direct loading of geometry through Alembic and USD is a standard feature for production renderers nowadays, also for supporting Hydra delegates. And with hdCycles we will get a USD procedural as well.

But we're not ready to expose this, so it's experimental. This may only really make sense when we have something like I/O collections or native Hydra support in Blender.

The current implementation using check_rendered_viewport_visible is clearly unreliable in general, but helps testing the functionality until we have a more complete integration. It should also be commented in the code/description that this is merely for testing.

I still need to review the code, both in Cycles and the Blender integration.

In D3089#243389, @Kévin Dietrich (kevindietrich) wrote:

I would say it is bit faster to load as we don't go through the RNA, but not so much for static objects as they are not updated frame to frame anyway. There are a few reasons:

• detect objects that have just an animated transforms, as I use the MeshSequenceCache modifier for this. I think I could add a detection through the constraints

Yes, IMO this should be done via the constraints system. There is no need to add a modifier to the object for this.

• support more features from Alembic as Blender is limiting and limited at the moment: instances, custom attributes, better subdivision support (corner crease, face holes, etc.).

Personally I would suggest to improve Blender if such features are needed. But then again, my view is of course Blender-oriented and not Cycles-oriented, so I'll defer to you guys on this.

• it would allow to modify those objects in the archive, and have them updated in Blender or Cycles when reloading it, I know that this type of workflow is not so supported in Blender but we would have to start thinking about it at some point

This would be nice to have for Blender as well, I know there's often requests for this.

I am pretty sure it was necessary before, for convenience to avoid having to manually set the radiuses on all objects. Maybe people don't care or complain. I cannot tell you why I haven't parameterized this when I worked on the importer though.

Fine by me to add such a thing. I'd do that in a separate patch, though, as I don't think it has much to do with Cycles per se.