Page MenuHome

Cycles: add support for rendering deformation motion blur from Alembic caches.
ClosedPublic

Authored by Kévin Dietrich (kevindietrich) on Nov 30 2016, 10:06 AM.

Details

Summary

This patch adds the ability to render motion blur from Alembic caches.
The motion blur data is derived from a velocity attribute whose name has
to be defined by the user through the MeshSequenceCache modifier, with a
default value of ".velocities", which is the standard name in Alembic
for the velocity property, although other software may ignore it and
write velocity with their own naming convention (e.g. "v" in Houdini).

Furthermore, a property was added to define how the velocity vectors
have to be interpreted as regard to time : frame or second. "Frame"
means that the velocity is already scaled by the time step and we do not
need to modify it for it to look proper. "Second" means that the unit
the velocity was measured in is in seconds and so has to be scaled by
some time step computed here as being the time between two frames (1 /
FPS, which would be typical for a simulation). This appears to be
common, and is the default behavior.

Another property was added to control the scale of the velocity to
further modify the look of the motion blur.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

So it's been a year since the last post in this thread. Still nothing? I really miss this feature.

So it's been a year since the last post in this thread. Still nothing? I really miss this feature.

Yeah, this shows how few active developers we have at the moment... :/

  • Attempt to update the code.
  • Finally make motion blur work!
Kévin Dietrich (kevindietrich) retitled this revision from Cycles: initial work to support motion blur from Alembic caches. to Cycles: add support for rendering deformation motion blur from Alembic caches..Oct 7 2019, 5:05 PM
Kévin Dietrich (kevindietrich) edited the summary of this revision. (Show Details)

This patch is nearing completion, with one remaining property to be added. Here are some examples:

with the alembic file for the second image (the first one is a bit huge, ~450Mb):

Here is another test with a file containing 3 different case studies (a fractured objet, a liquid sim, and a rotating object), all of which have custom velocities:

Shutter of 0.5:

Shutter of 1.0:

Blender and Alembic files:

I don't understand some of the design in the patch.

  • Why is there a change in the Scene RNA required? The fps and fps_base properties are already exposed in RNA, and having two ways to reach the same properties seems unnecessary.
  • Storing the current frame in DNA also doesn't seem right. This is something that should be given by the depsgraph, and not something to be stored per MeshSeqCacheModifierData datablock.
  • I don't know anything about Cycles, so I can't comment on the desirability of having MeshSequenceCacheModifier-specific code in there.

I don't understand some of the design in the patch.

  • Why is there a change in the Scene RNA required? The fps and fps_base properties are already exposed in RNA, and having two ways to reach the same properties seems unnecessary.

Sorry, I completely missed that, I was probably not looking at the right place.

  • Storing the current frame in DNA also doesn't seem right. This is something that should be given by the depsgraph, and not something to be stored per MeshSeqCacheModifierData datablock.

The reason is briefly mentioned in the description, let me explain some more. The only reason for that is to be able to lookup the data in the Alembic archive at the right time. However, to access the data from Cycles, we have to go through the RNA and the generated RNA accessors (for MeshSequenceCacheModifier_has_velocity and MeshSequenceCacheModifier_has_velocity_get) have a set interface, meaning we cannot have as many arguments to the RNA functions as we want, and therefore cannot pass e.g. the time information as a parameter to the RNA function call between Cycles and Blender. So this is why I put the time information in the MeshSeqCacheModifierData datablock: to be able to have more arguments. The depsgraph could do that, however we still can't access it in the MeshSequenceCacheModifier RNA function call, can we? Modifiers do not have a pointer to the depsgraph, nor do they have one to the scene anymore. So we still have to pass the time information between Cycles and Blender somewhat. Perhaps another approach would be to get the archive in Cycles and lookup the data from there.

  • I don't know anything about Cycles, so I can't comment on the desirability of having MeshSequenceCacheModifier-specific code in there.

This is ok, we also access modifiers e.g. for the liquid and smoke simulations. This is not really Cycles, but the Blender interface to Cycles, or the Cycles interface to Blender depending on the perspective.

I have some issues understanding the patch description.

Title says it all.

Only for small values of the word "all".

an attribute representing the velocities of the points is used to evaluate their positions through time.

Aren't the positions given by Alembic? I don't quite understand what you mean with the velocities being used to evaluate positions.

The velocity vectors are copied

From where to where?

while assuming that the destination buffer is composed of 4d vectors since Cycles float3 type is 4 float wide (see comment in code), so the API is only meant for Cycles at the moment.

I'm not familiar with Cycles code at all, but is it really a good idea to assume a float3 is actually 4 floats wide? Isn't it possible to use a sizeof() instead of hard-coding this assumption?

The velocity vectors are also scaled by the FPS (currently hardcoded) since some software (Houdini) stores acceleration (change in velocity) and not velocity (change in position) per se. This would need a custom property to be added at the archive level.

I don't see how one relates to the other. "Scaled by the FPS", does that mean "converted to acceleration" or "converted from acceleration to actual velocities"? What happens to the units when you change scale like that? I really don't agree with the approach that, because some software botches the specs and writes acceleration into a velocity property, ALL velocities are interpreted as acceleration. If we handle this at all, it shouldn't be done at the interface to a specific rendering engine.

source/blender/alembic/intern/alembic_capi.cc
1155 ↗(On Diff #18844)

No way we can hard-code 24 FPS. Also see this post on alembic-discussion and this bug report on USD for the general problems such an approach causes.

Kévin Dietrich (kevindietrich) planned changes to this revision.Oct 30 2019, 1:38 PM

Please ignore the velocity and acceleration discussion, that was a brain fart from me. The explanation is rather simple actually: the velocity property gives us dP when we need dPdt (so the values make sense in the "point in time" in which we use them). We might still need a user setting to make sure dt is the correct value for the velocities, instead of relying on the FPS (although in practice they are the same).

I will update the patch and modify or clarify the description later.

Kévin Dietrich (kevindietrich) edited the summary of this revision. (Show Details)

Update patch to master and implement missing features.

Brecht Van Lommel (brecht) requested changes to this revision.Jul 13 2020, 2:47 PM

Can we move all of these settings to the CacheFile datablock, and leave only velocity_scale per object for artistic tweaking?

I think it will be easier this way when importing an Alembic file with many objects. They are likely to all use the same convention for velocity attributes, and so it will be easier to specify it just once. Various existing settings shown in the modifier UI are also at the CacheFile level.

This revision now requires changes to proceed.Jul 13 2020, 2:47 PM
  • Cycles : add support for rendering motion blur from custom Alembic
  • move main velocity properties to the CacheFile datablock
Brecht Van Lommel (brecht) requested changes to this revision.Jul 15 2020, 2:07 PM
Brecht Van Lommel (brecht) added inline comments.
intern/cycles/blender/blender_mesh.cpp
1005–1009

Do we really need to do this in Cycles?

The mesh cache modifier can just store such values when it is evaluated by the depsgraph, and then return the velocity matching them.

For me this patch is also a temporary step towards unifying handling of such velocity for all geometry types, which should be done with the modifier adding a vector attribute named velocity. In that case such fps computation also needs to be done by the depsgraph, and the renderer will simply read the attribute.

source/blender/makesrna/intern/rna_modifier.c
6178–6192

Related to my above comment, these two properties should not be needed in the API if the depsgraph evaluation can handle this internally.

This revision now requires changes to proceed.Jul 15 2020, 2:07 PM
  • Cache time-related data during the last execution of the modifier.
  • Run clang-format
This revision is now accepted and ready to land.Jul 15 2020, 3:47 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Jul 16 2020, 11:47 AM

Great to see you back working on Alembic @Kévin Dietrich (kevindietrich) :)

source/blender/io/alembic/intern/alembic_capi.cc
876

How do you decide between employing using Alembic::Abc::PropertyHeader and using the full name Alembic::Abc::PropertyHeader? The code now uses a mixture of both approaches.

952

Naming wise I think read is more appropriate than get. This may be a personal thing, but I would expect get to just return a pointer to a cache, whereas this function actually reads the entire cache into a buffer. Naming the function ABC_read_velocity_cache() would also make it consistent with ABC_read_mesh().

953

values is a return parameter, so its name should be prefixed with r_ and be the last parameter.

953

values doesn't convey any information about what the returned data represents. Something like r_vertex_velocities would be better.

953

The caller is responsible for allocating memory for these values. This is done based on the number of vertices in the Blender mesh, which is not guaranteed to be the same as the size of the velocities property in Alembic. This will cause crashes when Alembic files aren't written perfectly.

Pass the size of the buffer to the ABC_get_velocity_cache() function, so that it can prevent buffer overflows.

961

The caller should be notified that the values in the buffer are invalid. I think returning the number of velocities read would be a good indicator here, as that could also be used to detect cases where there are less velocities in the ABC file than there are vertices in the Blender mesh. Making the return type signed means that -1 can be returned in case of errors like this.

source/blender/makesdna/DNA_cachefile_types.h
56–59

This needs a comment that explains what it's used for.

91

This needs a comment that explains this contains the name of a property in an Alembic file.

source/blender/makesdna/DNA_modifier_types.h
2075

This needs a comment that explains what 'delta' means. It's a difference, of course, but right now it's undocumented between which two things that difference is computed.

2076

This needs a comment to explain what it's used for, and to define which units the time is expressed in.

source/blender/makesrna/intern/rna_cachefile.c
177

This block defines multiple RNA attributes, but I'm guessing the comment is singular because it referes to the Alembic attribute. This isn't clear, so probably it's better to name the block "Alembic Velocity Attribute" or something along those lines.

181

"Name of the Alembic attribute"

198

"have to be" → "are"

source/blender/makesrna/intern/rna_modifier.c
6181

Consistency: capitalise the comment.

This revision now requires changes to proceed.Jul 16 2020, 11:47 AM
Kévin Dietrich (kevindietrich) marked 12 inline comments as done.Jul 16 2020, 3:53 PM

I have done most of the requested changes apart from the ones to properly handle cases for mismatching array lengths. See inline comments.

source/blender/io/alembic/intern/alembic_capi.cc
876

When writing C++ code I tend to always fully qualify the names. However, Alembic namespaces are quite long to type, so I employ using X::Y::Z instead. This is just an oversight, I probably typed the fully qualified name because there was no using Alembic::Abc::PropertyHeader directive in the file, and forgot to cleanup.

953

I do not think that it is possible to have more input parameters in the Cycles side as the generated RNA wrappers for array getters only have one argument for the pointer to the array. Unless that can be controlled somewhat, and we can have an arbitrary number of arguments.

I think we could temporarily store the velocity vectors in the modifier, and define some RNA functions to have a proper C++ class with a length() property like the fluid modifier is doing. That way we can check from Cycles if the number of elements in the array matches the number of vertices. In the future this approach could perhaps be reused for a generic velocity attribute that would exist on various objects as @Brecht Van Lommel (brecht) envisions. Then we would need to have APIs to allocate the array when needed and to deallocate it when not needed anymore.

961

Same as above, we would not be able to check the return value from Cycles.

source/blender/io/alembic/intern/alembic_capi.cc
953

We can indeed copy the fluid modifier here.

If that's a lot of work, alternatively we postpone this patch to 2.91, I finish D8200: Geometry: add .attributes in the Python API for Mesh, Hair and Point Cloud and we add a velocity attribute instead.

Kévin Dietrich (kevindietrich) marked an inline comment as done.
  • store the velocity vectors in the modifier, and access them in Cycles via an iterator

Thanks for the changes. The patch looks good to me apart from one missing bit of documentation.

source/blender/io/alembic/ABC_alembic.h
137

It's probably a good idea to document that r_vertex_velocities should point to a preallocated array of num_vertices floats.

  • add missing bit of documentation
This revision is now accepted and ready to land.Jul 28 2020, 5:50 PM