USD Export: New Curves/Hair Support #105375

Merged
Michael Kowalski merged 19 commits from SonnyCampbell_Unity/blender:unity/T102376-USD-Curves-Export into main 2023-05-16 20:04:26 +02:00
Member

Previous patch location: https://archive.blender.org/developer/D16545

This patch addresses #102376

A new writer is added, the usd_writer_curves.cc to handle transforming the new curves system into USD.

The goal was to enable export of the new curves type, but @HooglyBoogly mentioned there is a curve_legacy_to_curves utility function that could also handle converting legacy curves to the new Curves type. This very trivially enables the legacy curves for export too so I have included that change in this patch.

Result in usdview:
image

Previous patch location: https://archive.blender.org/developer/D16545 This patch addresses https://projects.blender.org/blender/blender/issues/102376 A new writer is added, the usd_writer_curves.cc to handle transforming the new curves system into USD. The goal was to enable export of the new curves type, but @HooglyBoogly mentioned there is a `curve_legacy_to_curves` utility function that could also handle converting legacy curves to the new Curves type. This very trivially enables the legacy curves for export too so I have included that change in this patch. Result in usdview: ![image](/attachments/80263d9f-a31b-4d09-9a4e-211ad5dd9b76)
Sonny Campbell requested review from Sybren A. Stüvel 2023-03-07 13:41:27 +01:00
Sonny Campbell requested review from Hans Goudey 2023-03-07 13:41:28 +01:00
Sonny Campbell requested review from Charles Wardlaw 2023-03-07 13:42:32 +01:00
Sonny Campbell requested review from Michael Kowalski 2023-03-07 13:42:33 +01:00
Sonny Campbell requested review from Matt McLin 2023-03-07 13:42:33 +01:00
Sybren A. Stüvel reviewed 2023-03-07 17:49:58 +01:00
@ -0,0 +1,492 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2022 Blender Foundation. All rights reserved. */

Should be this year.

Should be this year.
SonnyCampbell_Unity marked this conversation as resolved
Sonny Campbell changed title from WIP: USD Export - New Curves/Hair Support to USD Export - New Curves/Hair Support 2023-03-08 15:17:45 +01:00
Hans Goudey changed title from USD Export - New Curves/Hair Support to USD Export: New Curves/Hair Support 2023-03-08 21:51:18 +01:00
Hans Goudey requested changes 2023-03-09 13:44:22 +01:00
Hans Goudey left a comment
Member

Looking good! Will be really nice to have this supported so IO with the new curve system is more possible.

Did you think about support for generic attributes here? That's an important part of the new workflow so it might be nice to support it here. Probably adds a bunch more code though, so it could make sense to do it in a separate PR.

Looking good! Will be really nice to have this supported so IO with the new curve system is more possible. Did you think about support for generic attributes here? That's an important part of the new workflow so it might be nice to support it here. Probably adds a bunch more code though, so it could make sense to do it in a separate PR.
@ -0,0 +126,4 @@
control_point_counts[i_curve] = tot_points;
/* For periodic linear curve, segment count = curveVertexCount.
For periodic cubic curve, segment count = curveVertexCount / vstep.
Member
Comment formatting: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments
SonnyCampbell_Unity marked this conversation as resolved
@ -0,0 +175,4 @@
const int bezier_vstep = 3;
const OffsetIndices points_by_curve = geometry.points_by_curve();
for (int i_curve = 0; i_curve < geometry.curve_num; i_curve++) {
Member

for (const int i_curve : geometry.curves_range()) is a slightly nicer way to write this, and it allows the iterator variable to be const

`for (const int i_curve : geometry.curves_range())` is a slightly nicer way to write this, and it allows the iterator variable to be const
SonnyCampbell_Unity marked this conversation as resolved
@ -0,0 +177,4 @@
for (int i_curve = 0; i_curve < geometry.curve_num; i_curve++) {
const IndexRange curve_points = points_by_curve[i_curve];
Member

The "canonical" variable name used elsewhere for this is points instead of curve_points

The "canonical" variable name used elsewhere for this is `points` instead of `curve_points`
SonnyCampbell_Unity marked this conversation as resolved
@ -0,0 +183,4 @@
const int start_verts_count = verts.size();
for (int i_point = start_point_index; i_point < last_point_index; i_point++) {
Member

IndexRange can be iterated directly here: for (const int i : points) {

If you want to skip the last point, you can use for (const int i : points.drop_back(1)) {

`IndexRange` can be iterated directly here: `for (const int i : points) {` If you want to skip the last point, you can use `for (const int i : points.drop_back(1)) {`
Member

Is this a necessary change? Personally I find "i_point < last_point_index" more readable than points.drop_back(1).

Is this a necessary change? Personally I find "i_point < last_point_index" more readable than points.drop_back(1).
SonnyCampbell_Unity marked this conversation as resolved
@ -0,0 +294,4 @@
knots.resize(knots_num);
for (int i_knot = 0; i_knot < knots_num; i_knot++) {
knots[i_knot] = double(temp_knots[i_knot]);
Member

Maybe I'm missing something, but doesn't this replace the knots completely with every new curve?

Maybe I'm missing something, but doesn't this replace the knots completely with every new curve?
Author
Member

Yeah you are right, thanks for catching this! I need to rethink what I was trying to do here.

Yeah you are right, thanks for catching this! I need to rethink what I was trying to do here.
Author
Member

I have resolved this issue with the knots now. According to the USD spec knots should be the concatentation of all batched curves so I've updated the logic there and added a test with a curve containing two NURBS curves.

https://openusd.org/dev/api/class_usd_geom_nurbs_curves.html#details

I have resolved this issue with the knots now. According to the USD spec `knots should be the concatentation of all batched curves` so I've updated the logic there and added a test with a curve containing two NURBS curves. https://openusd.org/dev/api/class_usd_geom_nurbs_curves.html#details
SonnyCampbell_Unity marked this conversation as resolved
@ -0,0 +382,4 @@
});
if (number_of_curve_types > 1) {
WM_report(RPT_WARNING, "Cannot export mixed curve types in the same Curve object.");
Member

Small thing, but Curve should be Curves here, same with below.

Small thing, but `Curve` should be `Curves` here, same with below.
SonnyCampbell_Unity marked this conversation as resolved
@ -0,0 +419,4 @@
}
else if (first_frame_curve_type != curve_type) {
WM_reportf(RPT_WARNING,
"USD does not support animating curve types. The curve type changes from %i to "
Member

If you want this to be extra pretty, you could use rna_enum_curves_types and IFACE_(RNA_enum_name_from_value(..) to print the UI name instead of the integer value.

If you want this to be extra pretty, you could use `rna_enum_curves_types` and `IFACE_(RNA_enum_name_from_value(..)` to print the UI name instead of the integer value.
SonnyCampbell_Unity marked this conversation as resolved
Bastien Montagne added this to the USD project 2023-03-09 17:37:27 +01:00
Matt McLin reviewed 2023-03-11 00:22:14 +01:00
@ -0,0 +52,4 @@
void check_bezier_curve(const pxr::UsdPrim bezier_prim,
const bool is_periodic,
const int vertex_count);
void check_nurbs_curve(const pxr::UsdPrim nurbs_prim,
Member

The above three functions (check_catmullRom_curve, check_bezier_curve, check_nurbs_curve) need to be marked as static to match their definition below, otherwise this code does not compile with clang on macOS.

The above three functions (`check_catmullRom_curve`, `check_bezier_curve`, `check_nurbs_curve`) need to be marked as static to match their definition below, otherwise this code does not compile with clang on macOS.
SonnyCampbell_Unity marked this conversation as resolved
Matt McLin requested changes 2023-03-11 02:01:57 +01:00
Matt McLin left a comment
Member

Looks good to me, with just small fix needed for compiling on Mac.

Looks good to me, with just small fix needed for compiling on Mac.
Hans Goudey requested changes 2023-05-03 14:39:41 +02:00
Hans Goudey left a comment
Member

I forget exactly where this left off, but the change looks nice now, and makes sense for a first step.Just a few small inline comments this time.

I forget exactly where this left off, but the change looks nice now, and makes sense for a first step.Just a few small inline comments this time.
@ -0,0 +86,4 @@
return pxr::TfToken();
}
const size_t accumulatedControlPointCount = std::accumulate(
Member

Use snake_case for variable names (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Naming)

Use `snake_case` for variable names (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Naming)
SonnyCampbell_Unity marked this conversation as resolved
@ -0,0 +387,4 @@
});
if (number_of_curve_types > 1) {
WM_report(RPT_WARNING, "Cannot export mixed curve types in the same Curves object.");
Member

It's weird, but the period is added automatically to these report strings AFAIK, so no need to add it here. Same below.

It's weird, but the period is added automatically to these report strings AFAIK, so no need to add it here. Same below.
SonnyCampbell_Unity marked this conversation as resolved
Author
Member

Thanks @HooglyBoogly - yeah sorry it's been awhile since I got a chance to come back to this.

For some reason gitea won't let me resolve conversations, but from the previous comments I just need to take a look at the knots issue, and will address those two you have just added.

Thanks @HooglyBoogly - yeah sorry it's been awhile since I got a chance to come back to this. For some reason gitea won't let me resolve conversations, but from the previous comments I just need to take a look at the knots issue, and will address those two you have just added.
Sybren A. Stüvel refused to review 2023-05-05 12:58:15 +02:00

I'm standing down as reviewer, to not be a delaying factor here.

I'm standing down as reviewer, to not be a delaying factor here.
Matt McLin approved these changes 2023-05-05 20:22:21 +02:00
Matt McLin left a comment
Member

LGTM, and confirmed builds & appears to work ok on Mac (after merging latest from main into this branch).

LGTM, and confirmed builds & appears to work ok on Mac (after merging latest from main into this branch).
Sonny Campbell added 1 commit 2023-05-16 11:49:46 +02:00
Sonny Campbell requested review from Hans Goudey 2023-05-16 11:50:26 +02:00
Sonny Campbell added 1 commit 2023-05-16 11:52:50 +02:00
Hans Goudey approved these changes 2023-05-16 14:38:05 +02:00
@ -0,0 +427,4 @@
else if (first_frame_curve_type != curve_type) {
const char *first_frame_curve_type_name = nullptr;
RNA_enum_name_from_value(
rna_enum_curves_types, (int)first_frame_curve_type, &first_frame_curve_type_name);
Member

(int)first_frame_curve_type -> int(first_frame_curve_type)

Functional style casts here (and below) (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast)

`(int)first_frame_curve_type` -> `int(first_frame_curve_type)` Functional style casts here (and below) (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast)
SonnyCampbell_Unity marked this conversation as resolved
Sonny Campbell added 1 commit 2023-05-16 15:14:13 +02:00

@blender-bot build

@blender-bot build
Michael Kowalski approved these changes 2023-05-16 19:33:24 +02:00
Michael Kowalski left a comment
Member

I re-tested with the latest changes, and confirmed curves export is working well! I'll commit the PR shortly, as soon as conflict-checking is complete.

I also just committed the test file to svn.

I re-tested with the latest changes, and confirmed curves export is working well! I'll commit the PR shortly, as soon as conflict-checking is complete. I also just committed the test file to svn.

@blender-bot build

@blender-bot build
Michael Kowalski merged commit 1164976dd8 into main 2023-05-16 20:04:26 +02:00
Sonny Campbell deleted branch unity/T102376-USD-Curves-Export 2023-05-17 09:57:02 +02:00
Howard Trickey referenced this issue from a commit 2023-05-29 02:51:39 +02:00
Bastien Montagne removed this from the USD project 2023-08-25 23:52:02 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#105375
No description provided.