Page MenuHome

Cycles: experimental integration of Alembic procedural in viewport rendering
ClosedPublic

Authored by Kévin Dietrich (kevindietrich) on Jan 25 2021, 3:54 PM.

Details

Summary

This patch exposes the Cycles Alembic Procedural through the MeshSequenceCache
modifier in order to use and test it from Blender.

To enable it, one has to switch the render feature set to experimental and
activate the Procedural in the modifier. An Alembic Procedural is then
created for each CacheFile from Blender set to use the Procedural, and each
Blender object having a MeshSequenceCache modifier is added to list of objects
of the right procedural.

The procedural's parameters derive from the CacheFile's properties which are
already exposed in the UI through the modifier, although more Cycles specific
options might be added in the future.

As there is currently no cache controls and since we load all the data at the
beginning of the render session, the procedural is only available during
viewport renders at the moment. When an Alembic procedural is rendered, data
from the archive are not read on the Blender side.

If a Cycles render is not active and the CacheFile is set to use the Cycles Procedural,
bounding boxes are used to display the objects in the scene as a signal that the
objects are not processed by Blender anymore. This is standard in other DCCs.
However this does not reduce the memory usage from Blender as the Alembic data
was already loaded either during an import or during a .blend file read.

This is mostly a hack to test the Cycles Alembic procedural until we have a
better Blender side mechanism for letting renderers load their own geometry,
which will be based on import and export settings on Collections (T68933).

Also included in this patch, although it will committed separately, is a
setting to always add a cache reader (either a MeshSequenceCache modifier or
a Transform Cache constraint) to the objects created from an Alembic archive.
This setting is mostly useful to reload every object from the archive when it
is updated without having to resort to re-importing the entire scene, but it is
also used here to ensure that every object created from Alembic will be rendered
through the Procedural.

Ref T79174, D3089

Diff Detail

Repository
rB Blender
Branch
abc_proxies_review_D10197 (branched from master)
Build Status
Buildable 15195
Build 15195: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Kévin Dietrich (kevindietrich) edited the summary of this revision. (Show Details)
  • do not use const for parameters passed by value
  • rename "Use Viewport Proxies" to "Use Cycles Procedural"
  • add a proper RNA callback to update the depsgraph when toggling "Use Cycles Procedural"
  • rename "Always Add Modifier" to "Always Add Cache Reader"
  • also add constraints when "Always Add Cache Reader" is enabled

@Sybren A. Stüvel (sybren), can you take another looking at this?

Note that the intent is to add this in the master branch, but only visible when Cycles debugging is enabled, until it can be integrated better.

Fix missing hair when the emitter is not shown.

The logic to synchronize hair particles and objects separately was broken, the particles would
only be synchronized if the emitter object is shown (b_instance.show_self()).

This makes it so hair particles are always synchronized again (if they are themselves shown).
As the Alembic importer does not create hair particles, those particles will be ones manually
added by the artists, so outside of the scope of the procedural.

Sybren A. Stüvel (sybren) requested changes to this revision.May 25 2021, 3:35 PM

I don't quite like the fact that Cycles is made part of the CacheFile API. Is there any reason why this would be Cycles-specific, and not just render-engine-agnostic? In the current state of the patch, all of a sudden there is Cycles-specific code, which spreads to the constraint & modifier systems as well.

It would be better to have some way in which the render engine can signal support for certain procedurals. This can then be used by the Alembic reader (and other areas of Blender). The fact that only Cycles would support this is of course totally fine.

This revision now requires changes to proceed.May 25 2021, 3:35 PM
Brecht Van Lommel (brecht) requested changes to this revision.May 25 2021, 6:10 PM

Kévin and I discussed this. We'll give it a generic name and make this a feature that can be supported by any RenderEngine.

Add an API to RenderEngine to signify support of Alembic procedurals,
rename "Cycles Procedural" to "Render (Engine) Procedural".

The API is only implemented by Cycles, but external render engines can
also implement it.

This required to pass the Scene to ModifierTypeInfo.dependsOnTime,
which in turn is used in BKE_cache_file_uses_render_procedural.

I couldn't find an explicit way of getting the RenderEngine from the
Scene, so a new RenderEngine is created from the RenderEngineType
referenced by name by the Scene. This will have to be reviewed, but I
don't know how.

Brecht Van Lommel (brecht) requested changes to this revision.May 31 2021, 4:45 PM

I couldn't find an explicit way of getting the RenderEngine from the
Scene, so a new RenderEngine is created from the RenderEngineType
referenced by name by the Scene. This will have to be reviewed, but I
don't know how.

It's not necessary to create a render engine. Querying the support should not be done through a function call, but rather an flag on the render engine type. See for example RE_USE_POSTPROCESS and bl_use_postprocess.

For Cycles this can just be hardcoded to True. Since if Blender is built with Alembic support, then Cycles is as well.

This revision now requires changes to proceed.May 31 2021, 4:45 PM

Use a flag to detect support for Alembic procedurals.

This revision is now accepted and ready to land.Jun 10 2021, 10:54 AM
Sybren A. Stüvel (sybren) requested changes to this revision.EditedJun 10 2021, 11:28 AM

Playing around with the feature a bit more, I see I may have accepted the patch too soon. It would seem that the "Use Render Engine Procedural" checkbox doesn't do anything until Cycles is actually chosen as render engine. This makes it rather confusing, as:

  • Without Cycles, the checkbox is enabled but doesn't do anything.
  • After switching to Cycles, the proxies are still not shown.
  • After changing the frame, or after disabling & enabling the checkbox, the proxies are shown
  • When setting the viewport shading mode to Rendered, the proxies are rendered in Cycles (instead of the Alembic geometry).
  • Only after setting the Cycles feature set to Experimental, do I actually see the rendered Alembic geometry in the viewport.

In my opinion, the checkbox should either be removed from the UI or disabled with the appropriate explanation on why it is disabled, unless the required conditions for it to be useful are met.

This revision now requires changes to proceed.Jun 10 2021, 11:28 AM

The checkbox used to only be shown if Cycles was set as the render engine, but it was decided in an earlier review to always show it. Perhaps we should always show it but only enable it if the RenderEngine supports the feature, and add a message in the modifier's UI to if the currently selected render engine does not support it?

Then I do not mind Cycles rendering the proxies instead of Alembic geometry if experimental mode is not set, as this is communicated in the tooltip. Maybe it could be clearer?

The checkbox used to only be shown if Cycles was set as the render engine, but it was decided in an earlier review to always show it. Perhaps we should always show it but only enable it if the RenderEngine supports the feature, and add a message in the modifier's UI to if the currently selected render engine does not support it?

That sounds good to me!

Then I do not mind Cycles rendering the proxies instead of Alembic geometry if experimental mode is not set, as this is communicated in the tooltip. Maybe it could be clearer?

I think what you describe above is clear: the checkbox greyed-out and not doing anything when the render engine doesn't support the feature. Cycles in non-experimental mode to me counts as "a render engine that doesn't support the feature". I don't think it's wise to have a third option (checkbox enabled when Cycles is chosen, but still non-functional if experimental features are not enabled).

  • Remove dependency on D10196
  • Only enable use_render_procedural if the active render engine supports Alembic procedurals. If the engine is Cycles, we also check that the experimental feature set is enabled.
  • Add messages to the modifier's UI to inform about whether the engine has procedural support, and in case of Cycles, to enable experimental feature set.
Sybren A. Stüvel (sybren) requested changes to this revision.Jun 14 2021, 12:34 PM

Apart from code style issues, there are also some issues updating the viewport when things change.

  • Change from Cycles/Experimental to EEVEE. The boxes are still shown. Change the scene frame, and the Alembic mesh is shown properly.
  • Go back to Cycles/Experimental, and the Alembic mesh is still shown. Change the scene frame, and the selection overlay shows the box outline in orange, and Cycles renders the procedural.
source/blender/blenkernel/BKE_cachefile.h
71

const

source/blender/blenkernel/intern/constraint.c
5325

The (const int) cast is unnecessary.

source/blender/blenkernel/intern/object.c
5402–5405

dag_eval_mode should be const. Likely the same for the other functions where int dag_eval_mode is added as a parameter.

source/blender/editors/interface/interface_templates.c
7294

Determining whether the current render engine supports render procedurals should *not* be part of the UI code. I would expect this to be handled by some RenderEngineType function.

This revision now requires changes to proceed.Jun 14 2021, 12:34 PM
Kévin Dietrich (kevindietrich) marked 4 inline comments as done.

Rebase on master.

Tag CacheFiles for an update in ED_render_engine_changed to ensure that
modifiers are recomputed or disconnected from the CacheFiles when switching
render engine. This solves some of the update issues.

Add an RNA function to Scene which is called from the Cycles addon when
modifying the feature set. This calls ED_render_engine_changed and ensures
that CacheFiles and dependant modifiers (and maybe other data) are updated
when switching between support and experimental feature set.

Added RE_engine_supports_alembic_procedural to check if the render engine
does support the procedural, with proper checks for the Cycles feature set.
This is also called from BKE_cachefile_uses_render_procedural as simply
checking the flag there did not properly detect that the render engine was
not supporting (anymore) the procedural.

I think this resolves the remaining update issues.

Added const qualifiers, however removed the ones for the Scene as
RNA_id_pointer_create (called in RE_engine_supports_alembic_procedural)
does not take a const ID. (Those were added by this patch anyway.)

Note that the boxes are visible for the selection overlay during Cycles
renders since the Overlay engine does not support the Alembic procedural.

Sybren A. Stüvel (sybren) requested changes to this revision.Jul 2 2021, 11:55 AM

There are still a bunch of unaddressed review notes. Please either address them, or let me know why you don't think they should be addressed.

source/blender/editors/render/render_update.c
210

Use LISTBASE_FOREACH(CacheFile *, cachefile, &bmain->cachefiles) instead.

This revision now requires changes to proceed.Jul 2 2021, 11:55 AM
Kévin Dietrich (kevindietrich) marked an inline comment as done.Jul 2 2021, 2:06 PM

There are still a bunch of unaddressed review notes. Please either address them, or let me know why you don't think they should be addressed.

What would this bunch of unaddressed review notes be? I went again through the discussion and inlined comments, and everything seems to be done. The latest discussions were about viewport/render engine update issues and after retrying the issues, those appear to be fixed.

I am gonna update the patch as there were some conflicting changes in master, and I also made some other small changes (including your latest note).

  • Rebase with master.
  • Add some comment.
  • Missing const in some dependsOnTime callback.
  • Unnecessary cast.
  • Use LISTBASE_FOREACH.

For example the "const is not necessary in the declaration" ones. Or the one where I ask:

Why is this correct? What will happen when CacheFile properties are animated? Why should this not be evaluated by Blender? Wouldn't this present incorrect (i.e. unanimated) data in the UI?

Just scroll down to the actual diff, and press the "K/N Comments" button at the top to cycle through comments not yet marked as "Done".

Kévin Dietrich (kevindietrich) marked 9 inline comments as done.
  • Rebase with master.
  • Remove const from function declaration
source/blender/depsgraph/intern/builder/deg_builder_relations.cc
2624–2626

Tagging this as done as the original code was removed, the explanation is in the comments right below this one.

Sybren A. Stüvel (sybren) requested changes to this revision.Jul 6 2021, 11:22 AM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/render/RE_engine.h
248

I understand why you added the r_is_cycles parameter, but I think it's out of place API-wise. Determining which render engine is selected is different from checking whether it supports the Alembic procedural, and thus should be in different functions.

source/blender/render/intern/engine.c
146–153

I'm not too familiar with the render engine code, but this seems out of place to me. IMO knowledge of exactly what is supported in a specific render engine should be kept to that render engine, and thus be implemented in some function of RenderEngineType or RenderEngine.

Furthermore, the query "is the experimental feature set of Cycles enabled?" should be implemented in a separate function, as it's different from determining whether Alembic procedurals are supported.

150

Where does this 1 come from? Magic constants should not be used.

This revision now requires changes to proceed.Jul 6 2021, 11:22 AM

Let's add a BKE_scene_uses_cycles_experimental_features(scene) function (there already exists BKE_scene_uses_cycles) , that can then also be reused in the subsurf modifier.

It's not ideal to have render engine specific features like this hardcoded in Blender, but it's difficult to implement in another way and Cycles is a native Blender feature.

That looks like an acceptable approach to me.

Kévin Dietrich (kevindietrich) marked 3 inline comments as done.

Add BKE_scene_uses_cycles_experimental_features.

Remove is_cycles parameter.

Add an enumeration to avoid using magic numbers for the enum value of the experimental feature set. This could perhaps use a string comparison but all the functions comparing enum identifiers string are taking a bContext.

Rebase with master, check that cache file type is Alembic when checking for procedural usage.

Please go ahead and commit this. I think all of @Sybren A. Stüvel (sybren)'s comments have been addressed, he's on vacation for a while, and more than a month has passed already. If there are further issues we can address them in master.

This revision is now accepted and ready to land.Aug 17 2021, 4:44 PM