T99807: Add support for exporting to USDZ #104556

Closed
Sonny Campbell wants to merge 8 commits from SonnyCampbell_Unity/blender:unity/T99807-usdz-refactor into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
7 changed files with 268 additions and 24 deletions

View File

@ -491,7 +491,7 @@ class TOPBAR_MT_file_export(Menu):
self.layout.operator("wm.alembic_export", text="Alembic (.abc)")
if bpy.app.build_options.usd:
self.layout.operator(
"wm.usd_export", text="Universal Scene Description (.usd, .usdc, .usda)")
"wm.usd_export", text="Universal Scene Description (.usd*)")
if bpy.app.build_options.io_gpencil:
# Pugixml lib dependency

View File

@ -57,8 +57,8 @@ int BLI_delete(const char *file, bool dir, bool recursive) ATTR_NONNULL();
* \return zero on success (matching 'remove' behavior).
*/
int BLI_delete_soft(const char *file, const char **error_message) ATTR_NONNULL();
int BLI_path_move(const char *path, const char *to) ATTR_NONNULL();
#if 0 /* Unused */
int BLI_move(const char *path, const char *to) ATTR_NONNULL();
int BLI_create_symlink(const char *path, const char *to) ATTR_NONNULL();
#endif

View File

@ -458,9 +458,7 @@ int BLI_delete_soft(const char *file, const char **error_message)
return err;
}
/* Not used anywhere! */
# if 0
int BLI_move(const char *file, const char *to)
int BLI_path_move(const char *file, const char *to)
{
char str[MAXPATHLEN + 12];
int err;
@ -490,7 +488,6 @@ int BLI_move(const char *file, const char *to)
return err;
}
# endif
int BLI_copy(const char *file, const char *to)
{
@ -822,8 +819,8 @@ static int delete_soft(const char *file, const char **error_message)
Class NSStringClass = objc_getClass("NSString");
SEL stringWithUTF8StringSel = sel_registerName("stringWithUTF8String:");
id pathString = ((
id(*)(Class, SEL, const char *))objc_msgSend)(NSStringClass, stringWithUTF8StringSel, file);
id pathString = ((id(*)(Class, SEL, const char *))objc_msgSend)(
NSStringClass, stringWithUTF8StringSel, file);
Class NSFileManagerClass = objc_getClass("NSFileManager");
SEL defaultManagerSel = sel_registerName("defaultManager");
@ -834,8 +831,8 @@ static int delete_soft(const char *file, const char **error_message)
id nsurl = ((id(*)(Class, SEL, id))objc_msgSend)(NSURLClass, fileURLWithPathSel, pathString);
SEL trashItemAtURLSel = sel_registerName("trashItemAtURL:resultingItemURL:error:");
BOOL deleteSuccessful = ((
BOOL(*)(id, SEL, id, id, id))objc_msgSend)(fileManager, trashItemAtURLSel, nsurl, nil, nil);
BOOL deleteSuccessful = ((BOOL(*)(id, SEL, id, id, id))objc_msgSend)(
fileManager, trashItemAtURLSel, nsurl, nil, nil);
if (deleteSuccessful) {
ret = 0;
@ -1123,8 +1120,6 @@ static int copy_single_file(const char *from, const char *to)
return RecursiveOp_Callback_OK;
}
/* Not used anywhere! */
# if 0
static int move_callback_pre(const char *from, const char *to)
{
int ret = rename(from, to);
@ -1149,7 +1144,7 @@ static int move_single_file(const char *from, const char *to)
/* if *file represents a directory, moves all its contents into *to, else renames
* file itself to *to. */
int BLI_move(const char *file, const char *to)
int BLI_path_move(const char *file, const char *to)
{
int ret = recursive_operation(file, to, move_callback_pre, move_single_file, NULL);
@ -1159,7 +1154,6 @@ int BLI_move(const char *file, const char *to)
return ret;
}
# endif
static const char *check_destination(const char *file, const char *to)
{

View File

@ -226,7 +226,7 @@ static bool wm_usd_export_check(bContext *UNUSED(C), wmOperator *op)
char filepath[FILE_MAX];
RNA_string_get(op->ptr, "filepath", filepath);
if (!BLI_path_extension_check_n(filepath, ".usd", ".usda", ".usdc", NULL)) {
if (!BLI_path_extension_check_n(filepath, ".usd", ".usda", ".usdc", ".usdz", NULL)) {
BLI_path_extension_ensure(filepath, FILE_MAX, ".usdc");
RNA_string_set(op->ptr, "filepath", filepath);
return true;

View File

@ -151,7 +151,7 @@ if(WITH_GTESTS)
tests/usd_stage_creation_test.cc
tests/usd_tests_common.cc
tests/usd_tests_common.h
tests/usd_usdz_export_test.cc
intern/usd_writer_material.h
)
if(USD_IMAGING_HEADERS)

View File

@ -11,6 +11,7 @@
#include <pxr/usd/usd/primRange.h>
#include <pxr/usd/usd/stage.h>
#include <pxr/usd/usdGeom/tokens.h>
#include <pxr/usd/usdUtils/dependencies.h>
#include "MEM_guardedalloc.h"
@ -41,21 +42,87 @@ struct ExportJobData {
Depsgraph *depsgraph;
wmWindowManager *wm;
char filepath[FILE_MAX];
char unarchived_filepath[FILE_MAX]; /* unarchived_filepath is used for usda/usdc/usd export. */
char usdz_filepath[FILE_MAX];
USDExportParams params;
bool export_ok;
timeit::TimePoint start_time;
const bool targets_usdz() const
{
if (usdz_filepath) {

No need to count all the characters when all you want to know is "empty or not". Use either usdz_filepath[0] or BLI_strnlen(usdz_filepath, 1) > 0.

No need to count all the characters when all you want to know is "empty or not". Use either `usdz_filepath[0]` or `BLI_strnlen(usdz_filepath, 1) > 0`.
Review

This is utterly incorrect, usdz_filepath is always true by definition... you want to check usdz_filepath[0] != '\0' here...

But that whole function can simply be return usdz_filepath[0] != '\0';

This is utterly incorrect, `usdz_filepath` is always `true` by definition... you want to check `usdz_filepath[0] != '\0'` here... But that whole function can simply be `return usdz_filepath[0] != '\0';`
Review

I think the null terminator always evaluates to false.

So if you have an empty string with usdz_filepath[0] = '\0' then if(usdz_filepath[0]) will evaluate to false.

The check for if(usdz_filepath) is to make sure that the usdz_filepath has been initialised.

I think the null terminator always evaluates to false. So if you have an empty string with `usdz_filepath[0] = '\0'` then `if(usdz_filepath[0])` will evaluate to false. The check for `if(usdz_filepath)` is to make sure that the `usdz_filepath` has been initialised.
Review

Though maybe your way is more explicit and there's less possibility for confusion?

Though maybe your way is more explicit and there's less possibility for confusion?
Review

Though yeah I just did a check and it looks like the if(usdz_filepath) is unnecessary so I'll remove it.

Though yeah I just did a check and it looks like the `if(usdz_filepath)` is unnecessary so I'll remove it.
return usdz_filepath[0];
}
return false;
}
const char *export_filepath() const
{
if (targets_usdz()) {
return usdz_filepath;
}
return unarchived_filepath;
}
};
static void report_job_duration(const ExportJobData *data)
{
timeit::Nanoseconds duration = timeit::Clock::now() - data->start_time;
std::cout << "USD export of '" << data->filepath << "' took ";
const char *export_filepath = data->export_filepath();
std::cout << "USD export of '" << export_filepath << "' took ";
timeit::print_duration(duration);
std::cout << '\n';
}
/**
* For usdz export, we must first create a usd/a/c file and then covert it to usdz. In Blender's
* case, we first create a usdc file in Blender's temporary working directory, and store the path
* to the usdc file in `unarchived_filepath`. This function then does the conversion of that usdc
* file into usdz.
*
* \return true when the conversion from usdc to usdz is successful.
*/
static bool perform_usdz_conversion(const ExportJobData *data)
{
char usdc_temp_dir[FILE_MAX], usdc_file[FILE_MAX];
BLI_split_dirfile(data->unarchived_filepath, usdc_temp_dir, usdc_file, FILE_MAX, FILE_MAX);
char usdz_file[FILE_MAX];
BLI_split_file_part(data->usdz_filepath, usdz_file, FILE_MAX);
SonnyCampbell_Unity marked this conversation as resolved

What is a dirfile? I think a term like "full path" would be better than "dirfile".

What is a dirfile? I think a term like "full path" would be better than "dirfile".
char original_working_dir[FILE_MAX];
BLI_current_working_dir(original_working_dir, FILE_MAX);
Review

This line and the one above are not properly using the API, which expect you to use the returned char pointer.

char original_working_dir_buff[FILE_MAX];
char *original_working_dir = BLI_current_working_dir(original_working_dir_buff, sizeof(original_working_dir_buff));
This line and the one above are not properly using the API, which expect you to use the returned char pointer. ``` char original_working_dir_buff[FILE_MAX]; char *original_working_dir = BLI_current_working_dir(original_working_dir_buff, sizeof(original_working_dir_buff)); ```
BLI_change_working_dir(usdc_temp_dir);
pxr::UsdUtilsCreateNewUsdzPackage(pxr::SdfAssetPath(usdc_file), usdz_file);
BLI_change_working_dir(original_working_dir);
char usdz_temp_full_path[FILE_MAX];
BLI_path_join(usdz_temp_full_path, FILE_MAX, usdc_temp_dir, usdz_file);
int result = 0;
SonnyCampbell_Unity marked this conversation as resolved

Is it guaranteed that the temp "dirfile" is on the same filesystem as usdz_filepath? Have you tested with them being on different filesystems?

Is it guaranteed that the temp "dirfile" is on the same filesystem as `usdz_filepath`? Have you tested with them being on different filesystems?
if (BLI_exists(data->usdz_filepath)) {
result = BLI_delete(data->usdz_filepath, false, false);
if (result != 0) {
WM_reportf(
RPT_ERROR, "USD Export: Unable to delete existing usdz file %s", data->usdz_filepath);
return false;
}
}
result = BLI_path_move(usdz_temp_full_path, data->usdz_filepath);
if (result != 0) {
WM_reportf(RPT_ERROR,
"USD Export: Couldn't move new usdz file from temporary location %s to %s",
usdz_temp_full_path,
data->usdz_filepath);
return false;
}
return true;
}
static void export_startjob(void *customdata,
/* Cannot be const, this function implements wm_jobs_start_callback.
* NOLINTNEXTLINE: readability-non-const-parameter. */
@ -89,13 +156,14 @@ static void export_startjob(void *customdata,
/* For restoring the current frame after exporting animation is done. */
const int orig_frame = scene->r.cfra;
pxr::UsdStageRefPtr usd_stage = pxr::UsdStage::CreateNew(data->filepath);
pxr::UsdStageRefPtr usd_stage = pxr::UsdStage::CreateNew(data->unarchived_filepath);
if (!usd_stage) {
/* This happens when the USD JSON files cannot be found. When that happens,
* the USD library doesn't know it has the functionality to write USDA and
* USDC files, and creating a new UsdStage fails. */
WM_reportf(
RPT_ERROR, "USD Export: unable to find suitable USD plugin to write %s", data->filepath);
WM_reportf(RPT_ERROR,
"USD Export: unable to find suitable USD plugin to write %s",
data->unarchived_filepath);
return;
}
@ -159,19 +227,51 @@ static void export_startjob(void *customdata,
BKE_scene_graph_update_for_newframe(data->depsgraph);
}
if (data->targets_usdz()) {
bool usd_conversion_success = perform_usdz_conversion(data);
if (!usd_conversion_success) {
data->export_ok = false;
*progress = 1.0f;
*do_update = true;
return;
}

Why should this not clean up anything if unarchived_filepath is missing? Wouldn't it be prudent to always erase usdc_temp_dir?

Why should this not clean up anything if `unarchived_filepath` is missing? Wouldn't it be prudent to always erase `usdc_temp_dir`?
Review

usdc_temp_dir is the directory that contains data->unarchived_filepath, so if there is no data->unarchived_filepath there shouldn't be anything to delete.

This is basically so that we don't try to delete anything when there shouldn't be anything there to delete in the first place.

`usdc_temp_dir` is the directory that contains `data->unarchived_filepath`, so if there is no `data->unarchived_filepath` there shouldn't be anything to delete. This is basically so that we don't try to delete anything when there shouldn't be anything there to delete in the first place.
}
data->export_ok = true;
*progress = 1.0f;
*do_update = true;
}
static void export_endjob_usdz_cleanup(const ExportJobData *data)
{
if (!BLI_exists(data->unarchived_filepath)) {
return;
}
char dir[FILE_MAX];
BLI_split_dir_part(data->unarchived_filepath, dir, FILE_MAX);
char usdc_temp_dir[FILE_MAX];
BLI_path_join(usdc_temp_dir, FILE_MAX, BKE_tempdir_session(), "USDZ", SEP_STR);
BLI_assert_msg(BLI_strcasecmp(dir, usdc_temp_dir) == 0,
"USD Export: Attempting to delete directory that doesn't match the expected "
"temporary directory for usdz export.");
BLI_delete(usdc_temp_dir, true, true);
}
static void export_endjob(void *customdata)
{
ExportJobData *data = static_cast<ExportJobData *>(customdata);
DEG_graph_free(data->depsgraph);
if (!data->export_ok && BLI_exists(data->filepath)) {
BLI_delete(data->filepath, false, false);
if (data->targets_usdz()) {
export_endjob_usdz_cleanup(data);
}
if (!data->export_ok && BLI_exists(data->unarchived_filepath)) {
BLI_delete(data->unarchived_filepath, false, false);
}
G.is_rendering = false;
@ -183,6 +283,36 @@ static void export_endjob(void *customdata)
} // namespace blender::io::usd
/* To create a usdz file, we must first create a .usd/a/c file and then covert it to .usdz. The
* temporary files will be created in Blender's temporary session storage. The .usdz file will then
* be moved to job->usdz_filepath. */
static void create_temp_path_for_usdz_export(const char *filepath,
blender::io::usd::ExportJobData *job)
{
char file[FILE_MAX];
BLI_split_file_part(filepath, file, FILE_MAX);
char *usdc_file = BLI_str_replaceN(file, ".usdz", ".usdc");
SonnyCampbell_Unity marked this conversation as resolved

Most setters have the parameter order to mimick thing = value, so are in the form setter(thing, value). Swap the two parameters here.

Most setters have the parameter order to mimick `thing = value`, so are in the form `setter(thing, value)`. Swap the two parameters here.
char usdc_temp_filepath[FILE_MAX];
BLI_path_join(usdc_temp_filepath, FILE_MAX, BKE_tempdir_session(), "USDZ", usdc_file);
BLI_strncpy(job->unarchived_filepath, usdc_temp_filepath, strlen(usdc_temp_filepath) + 1);
BLI_strncpy(job->usdz_filepath, filepath, strlen(filepath) + 1);
MEM_freeN(usdc_file);
}
static void set_job_filepath(blender::io::usd::ExportJobData *job, const char *filepath)
{
if (BLI_path_extension_check_n(filepath, ".usdz", NULL)) {
create_temp_path_for_usdz_export(filepath, job);
return;
}
BLI_strncpy(job->unarchived_filepath, filepath, sizeof(job->unarchived_filepath));
job->usdz_filepath[0] = '\0';
}
bool USD_export(bContext *C,
const char *filepath,
const USDExportParams *params,
@ -197,7 +327,7 @@ bool USD_export(bContext *C,
job->bmain = CTX_data_main(C);
job->wm = CTX_wm_manager(C);
job->export_ok = false;
BLI_strncpy(job->filepath, filepath, sizeof(job->filepath));
set_job_filepath(job, filepath);
job->depsgraph = DEG_graph_new(job->bmain, scene, view_layer, params->evaluation_mode);
job->params = *params;

View File

@ -0,0 +1,120 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
#include "testing/testing.h"
#include "tests/blendfile_loading_base_test.h"
#include <pxr/usd/usd/prim.h>
#include <pxr/usd/usd/stage.h>
#include "BKE_appdir.h"
#include "BKE_context.h"
#include "BKE_main.h"
#include "BLI_fileops.h"
#include "BLI_path_util.h"
#include "BLO_readfile.h"
#include "DEG_depsgraph.h"
#include "WM_api.h"
#include "usd.h"
#include "usd_tests_common.h"
namespace blender::io::usd {
const StringRefNull usdz_export_test_filename = "usd/usdz_export_test.blend";
char temp_dir[FILE_MAX];
char temp_output_dir[FILE_MAX];
char output_filename[FILE_MAX];
class UsdUsdzExportTest : public BlendfileLoadingBaseTest {
protected:
struct bContext *context = nullptr;
public:
bool load_file_and_depsgraph(const StringRefNull &filepath,
const eEvaluationMode eval_mode = DAG_EVAL_VIEWPORT)
{
if (!blendfile_load(filepath.c_str())) {
return false;
}
depsgraph_create(eval_mode);
context = CTX_create();
CTX_data_main_set(context, bfile->main);
CTX_data_scene_set(context, bfile->curscene);
return true;
}
virtual void SetUp() override
{
BlendfileLoadingBaseTest::SetUp();
std::string usd_plugin_path = register_usd_plugins_for_tests();
if (usd_plugin_path.empty()) {
FAIL();
}
char original_wd[FILE_MAX];
BLI_current_working_dir(original_wd, FILE_MAX);
Review

Same issue as noted above regarding not using return value from BLI_current_working_dir().

Same issue as noted above regarding not using return value from `BLI_current_working_dir()`.
BKE_tempdir_init(nullptr);
const char *temp_base_dir = BKE_tempdir_base();
BLI_path_join(temp_dir, FILE_MAX, temp_base_dir, "usdz_test_temp_dir");
BLI_dir_create_recursive(temp_dir);
SonnyCampbell_Unity marked this conversation as resolved

👍 for testing with unicode characters.

Using the assets dir is not that great, though -- same as in #104525. Since this code already uses BKE_tempdir_init(), you can use BKE_tempdir_base(); and std::tmpnam() to generate a name that's guaranteed to be unique.

:+1: for testing with unicode characters. Using the assets dir is not that great, though -- same as in #104525. Since this code already uses `BKE_tempdir_init()`, you can use `BKE_tempdir_base();` and `std::tmpnam()` to generate a name that's guaranteed to be unique.
BLI_path_join(temp_output_dir, FILE_MAX, temp_base_dir, "usdz_test_output_dir");
BLI_dir_create_recursive(temp_output_dir);
BLI_path_join(output_filename, FILE_MAX, temp_output_dir, "output_новый.usdz");
}
virtual void TearDown() override
{
BlendfileLoadingBaseTest::TearDown();
CTX_free(context);
SonnyCampbell_Unity marked this conversation as resolved

Why not always delete temp_dir, regardless of whether output_filenaem exists or not?

Why not always delete `temp_dir`, regardless of whether `output_filenaem` exists or not?
context = nullptr;
BLI_delete(temp_dir, true, true);
BLI_delete(temp_output_dir, true, true);
}
};
TEST_F(UsdUsdzExportTest, usdz_export)
{
if (!load_file_and_depsgraph(usdz_export_test_filename)) {
ADD_FAILURE();
return;
}
SonnyCampbell_Unity marked this conversation as resolved

Use ASSERT_EQ for such checks. That will stop the test from continuing when it fails.

Use `ASSERT_EQ` for such checks. That will stop the test from continuing when it fails.
/* File sanity check. */
ASSERT_EQ(BLI_listbase_count(&bfile->main->objects), 4)
<< "Blender scene should have 4 objects.";
SonnyCampbell_Unity marked this conversation as resolved

Add some logging to this and the following EXPECT calls. When the test fails, it should be clear why it failed. You can do something like:

EXPECT_TRUE(result) << "USD export to " << output_filename << " failed";

If this particular test fails, the rest of the test is useless (it will all fail), so this one can be replaced with ASSERT_TRUE.

Add some logging to this and the following EXPECT calls. When the test fails, it should be clear why it failed. You can do something like: ```cpp EXPECT_TRUE(result) << "USD export to " << output_filename << " failed"; ``` If this particular test fails, the rest of the test is useless (it will all fail), so this one can be replaced with `ASSERT_TRUE`.
USDExportParams params{};
bool result = USD_export(context, output_filename, &params, false);
ASSERT_TRUE(result) << "usd export to " << output_filename << " failed.";
pxr::UsdStageRefPtr stage = pxr::UsdStage::Open(output_filename);
ASSERT_TRUE(bool(stage)) << "unable to open stage for the exported usdz file.";
std::string prim_name = pxr::TfMakeValidIdentifier("Cube");
pxr::UsdPrim test_prim = stage->GetPrimAtPath(pxr::SdfPath("/Cube/" + prim_name));
EXPECT_TRUE(bool(test_prim)) << "Cube prim should exist in exported usdz file.";
prim_name = pxr::TfMakeValidIdentifier("Cylinder");
test_prim = stage->GetPrimAtPath(pxr::SdfPath("/Cylinder/" + prim_name));
EXPECT_TRUE(bool(test_prim)) << "Cylinder prim should exist in exported usdz file.";
prim_name = pxr::TfMakeValidIdentifier("Icosphere");
test_prim = stage->GetPrimAtPath(pxr::SdfPath("/Icosphere/" + prim_name));
EXPECT_TRUE(bool(test_prim)) << "Icosphere prim should exist in exported usdz file.";
prim_name = pxr::TfMakeValidIdentifier("Sphere");
test_prim = stage->GetPrimAtPath(pxr::SdfPath("/Sphere/" + prim_name));
EXPECT_TRUE(bool(test_prim)) << "Sphere prim should exist in exported usdz file.";
}
} // namespace blender::io::usd