Page MenuHome

Alembic: add settings to control radius of imported curves and points
Changes PlannedPublic

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

Details

Summary

This adds a couple of settings to control the radius of imported curves and points:

  • A default radius for Alembic files, to fall back to when none is specified.
  • A radius scale per Alembic mesh cache modifier, to later alter the radius.

Since curve and point support in the modifier is limited due to how it works (by always
outputting a Mesh), those settings are only available when a render time Alembic
procedural is used. They will be enabled for use in the modifier when improvements
are made in this area, most likely by outputting a Geometry Set instead of a Mesh
from the modifier.

Since for now this is only for the Cycles procedural, to test this one has to set Cycles as the current render engine, using the experimental feature set.

Here's a test file with a radius property, which can be tweaked per curve using the radius scale (as the curves are not animated, cache modifiers are not added unless "Always Add Cache Reader" is turned on in the import options, so the .blend file already has modifiers on all curves):

Here's one without radius, which would need to be set in the import settings:

Ref D3089.

Event Timeline

Brecht Van Lommel (brecht) requested review of this revision.Jan 25 2021, 3:53 PM
Brecht Van Lommel (brecht) created this revision.

Also @Kévin Dietrich (kevindietrich), feel free to commandeer back this and the other code review.

This revision is now accepted and ready to land.Feb 19 2021, 5:04 PM

The code LGTM. Do you have an Alembic file I can use to test with?

Since the Mesh Sequence Modifier converts Curves to Meshes it's not really possible to test this since the radius is not taken into account when the conversion happens (?). If I recall correctly, Curves in Blender would need constructive modifiers to properly import them from Alembic (and USD)? Otherwise, I don't know how to test this outside of Cycles with D10197 (it was split from this patch). I may have lost such knowledge.

In any case, here's a file with a radius property, but it can be tweaked per curve using the radius scale (the .blend file has modifiers on all curves, which would require D10197 to avoir doing this manually):

Here's one without radius, which would need to be tweaked upon imports:

Also, I noticed that the default_radius set in the import operator is not inherited by the CacheFile.

  • pass default_radius to the CacheFile

This patch has an approved status yet it was not committed yet. So I'm assuming the other reviewers are blocking. Updating the reviewers list to reflect that. This way the patch show as pending still.

This revision now requires review to proceed.Mar 26 2021, 5:24 PM
  • rebase against latest master
Sybren A. Stüvel (sybren) requested changes to this revision.Jun 10 2021, 11:54 AM

This patch doesn't build without D10197:

Align pointer error in struct (size_native 8): CacheFile *handle
Align pointer error in struct (size_64 8): CacheFile *handle
Align pointer error in struct (size_native 8): CacheFile *handle_readers
Align pointer error in struct (size_64 8): CacheFile *handle_readers
Sizeerror 8 in struct: CacheFile (add 4 bytes)

Even after applying D10197 first, I don't see the effect of the default radius. I can change the value in the modifier's CacheFile settings, but it doesn't seem to change anything in the viewport.

Lastly, the Default Radius property doesn't seem to have any units. Shouldn't it be expressed in length-units (which ones depend on the scene unit settings)?

I can't accept this patch without any clear steps to actually test this functionality.

This revision now requires changes to proceed.Jun 10 2021, 11:54 AM
  • Fix CacheFile DNA padding
  • Add unit to default_radius
  • Update curve radius when reading data from the Archive from the modifier, this requires passing the default_radius and radius_scale to ABC_read_mesh and dependant function calls.

I only made this from the Cycles procedural, so updating the data when changing properties
was not implemented for Blender.

To test this from Blender, I think you have to only use a freshly imported Alembic file, as
Curves are converted to meshes and that is what is written to and read from a .blend file.
Also bevel should be added to the curves for anything to be rendered by Blender.

Sybren A. Stüvel (sybren) requested changes to this revision.Jun 11 2021, 4:00 PM

I only made this from the Cycles procedural, so updating the data when changing properties
was not implemented for Blender.

That's not what's described in the patch description. That describes a generic feature, that I expected to work regardless of the render engine.

To test this from Blender, I think you have to only use a freshly imported Alembic file, as
Curves are converted to meshes and that is what is written to and read from a .blend file.
Also bevel should be added to the curves for anything to be rendered by Blender.

The option can be changed after the import, so that means that it should have effect after the import. This should be solved one way or the other.

This revision now requires changes to proceed.Jun 11 2021, 4:00 PM

It's only tangentially related here, but I'll just note that if the we had an alembic import node it could output all geometry component types in a single modifier, and with D11577 you could have easy access to the separate types.
So this problem won't be here forever.

I guess this option could only show for the procedural for now, and then later gets used for the modifier once that is supported.

If we wanted to add proper Alembic hair support in the near term, it could even be done by making the current modifier output a geometry set, without any nodes.

Two things may happen:

  • either it is only enabled if using the render engine procedural, or
  • this feature is delayed until curves/hair are better supported by the modifier (which I'll be happy to work on as well)

In any case, this patch will be modified to depend on D10197 (which currently depends on this one) so that D10197 can be committed either without this feature, or this patch can use flags from D10197 to disable the feature if the render engine does not have any Alembic procedural of its own.

  • Remove logic to update radiuses in the Blender importer for the time being, as this might require bigger changes
  • Make patch depend on D10197, and only enable radius controls in the UI if a render procedural is used

The default radius is still set in the Blender importer, _upon import_, but is only
editable/enabled with a render engine procedural.

I guess this option could only show for the procedural for now, and then later gets used for the modifier once that is supported.

If we wanted to add proper Alembic hair support in the near term, it could even be done by making the current modifier output a geometry set, without any nodes.

I'm fine either way, as long as things are clear, both in the patch description (please update it) and in the user interface.

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

Please update the patch with some test files and some instructions on how to test this feature. For one, the patch doesn't apply to master without errors.

Please update the patch with some test files and some instructions on how to test this feature. For one, the patch doesn't apply to master without errors.

The patch depends on D10197: Cycles: experimental integration of Alembic procedural in viewport rendering now, apparently this was not updated here, so I guess arcanist was not aware of that, I will still update the patch so it is based on the latest revision of D10197. There are some test files in D10196#268999. I'll move them to the patch description so it is easier to find.

  • Rebase on master and last revision of D10197.

Accidentally included code from D10197.

The patch doesn't apply to master any more. I tried to fix it, but then I don't see any effect when edit the radius in export_guerilla_render_3.blend. I might have overlooked something.

Kévin Dietrich (kevindietrich) planned changes to this revision.EditedJan 6 2022, 5:24 PM

Thanks for the updated code. Marking as planned changes for the time being.

Not sure what you did. The curves in export_guerilla_render_3.blend actually have radius information so the default_radius will not work. And the radius_scale is per object, so it needs to be changed on a curve object (I keep forgetting that).

I am not sure if we should wait for T94193: Curve data structures relation design and D11592: Alembic/USD: use geometry sets to import data (which has to be reworked with changes discussed in T94193) before moving on with this patch? Then the feature will also work properly on the Blender side. Then we could also perhaps have a dedicated UI panel for the radius settings.