blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 778 Projects 19 Wiki Activity

Refactor: in USD importer extract read_mesh params into a struct #104459

Closed
Sonny Campbell wants to merge 11 commits from SonnyCampbell_Unity/blender:unity/T91369-parameter-refactor into main
pull from: SonnyCampbell_Unity/blender:unity/T91369-parameter-refactor
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 6 Commits 11 Files Changed 12 +39 -36
10 changed files with 39 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 3de9df6b6d - Show all commits

7
source/blender/io/usd/intern/usd_capi_import.cc
View File

@ -468,9 +468,8 @@ static USDPrimReader *get_usd_reader(CacheReader *reader,
struct Mesh *USD_read_mesh(struct CacheReader *reader,
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-13 11:52:53 +01:00
Review
Copy Link

const for both parameters.

`const` for both parameters.
struct Object *ob,
struct Mesh *existing_mesh,
const double time,
const char **err_str,
const int read_flag)
const USDMeshReadParams *params,
const char **err_str)
{
USDGeomReader *usd_reader = dynamic_cast<USDGeomReader *>(get_usd_reader(reader, ob, err_str));
@ -478,7 +477,7 @@ struct Mesh *USD_read_mesh(struct CacheReader *reader,
return nullptr;
}
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-13 11:53:14 +01:00
Review
Copy Link

const

`const`
return usd_reader->read_mesh(existing_mesh, time, read_flag, err_str);
return usd_reader->read_mesh(existing_mesh, params, err_str);
}
bool USD_mesh_topology_changed(CacheReader *reader,

9
source/blender/io/usd/intern/usd_reader_curve.cc
View File

@ -162,8 +162,7 @@ void USDCurvesReader::read_curve_sample(Curve *cu, const double motionSampleTime
}
Mesh *USDCurvesReader::read_mesh(struct Mesh *existing_mesh,
const double motionSampleTime,
const int /* read_flag */,
const USDMeshReadParams *params,
const char ** /* err_str */)
{
if (!curve_prim_) {
@ -176,11 +175,11 @@ Mesh *USDCurvesReader::read_mesh(struct Mesh *existing_mesh,
pxr::VtIntArray usdCounts;
vertexAttr.Get(&usdCounts, motionSampleTime);
vertexAttr.Get(&usdCounts, params->time);
int num_subcurves = usdCounts.size();
pxr::VtVec3fArray usdPoints;
pointsAttr.Get(&usdPoints, motionSampleTime);
pointsAttr.Get(&usdPoints, params->time);
int vertex_idx = 0;
int curve_idx;
@ -204,7 +203,7 @@ Mesh *USDCurvesReader::read_mesh(struct Mesh *existing_mesh,
if (!same_topology) {
BKE_nurbList_free(&curve->nurb);
read_curve_sample(curve, motionSampleTime);
read_curve_sample(curve, params->time);
}
else {
Nurb *nurbs = static_cast<Nurb *>(curve->nurb.first);

3
source/blender/io/usd/intern/usd_reader_curve.h
View File

@ -36,8 +36,7 @@ class USDCurvesReader : public USDGeomReader {
void read_curve_sample(Curve *cu, double motionSampleTime);
Mesh *read_mesh(struct Mesh *existing_mesh,
double motionSampleTime,
int read_flag,
const USDMeshReadParams *params,
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 12:27:29 +01:00
Review
Copy Link

Remove const, it has no meaning in function declarations for pass-by-value types.
Same in other declarations with a const USDMeshReadParams params parameter.

Remove `const`, it has no meaning in function declarations for pass-by-value types. Same in other declarations with a `const USDMeshReadParams params` parameter.
Sonny Campbell commented 2023-02-10 13:44:49 +01:00
Review
Copy Link

Done

Done
const char **err_str) override;
};

3
source/blender/io/usd/intern/usd_reader_geom.h
View File

@ -20,8 +20,7 @@ class USDGeomReader : public USDXformReader {
}
virtual Mesh *read_mesh(struct Mesh *existing_mesh,
double motionSampleTime,
int read_flag,
const USDMeshReadParams *params,
const char **err_str) = 0;
virtual bool topology_changed(const Mesh * /* existing_mesh */, double /* motionSampleTime */)

18
source/blender/io/usd/intern/usd_reader_mesh.cc
View File

@ -244,8 +244,11 @@ void USDMeshReader::read_object_data(Main *bmain, const double motionSampleTime)
Mesh *mesh = (Mesh *)object_->data;
is_initial_load_ = true;
Mesh *read_mesh = this->read_mesh(
mesh, motionSampleTime, import_params_.mesh_read_flag, nullptr);
USDMeshReadParams params = {};
params.time = motionSampleTime;
params.read_flags = import_params_.mesh_read_flag;
Mesh *read_mesh = this->read_mesh(mesh, &params, nullptr);
is_initial_load_ = false;
if (read_mesh != mesh) {
@ -817,8 +820,7 @@ void USDMeshReader::readFaceSetsSample(Main *bmain, Mesh *mesh, const double mot
}
Mesh *USDMeshReader::read_mesh(Mesh *existing_mesh,
const double motionSampleTime,
const int read_flag,
const USDMeshReadParams *params,
const char ** /* err_str */)
{
if (!mesh_prim_) {
@ -835,7 +837,7 @@ Mesh *USDMeshReader::read_mesh(Mesh *existing_mesh,
std::vector<pxr::TfToken> uv_tokens;
/* Currently we only handle UV primvars. */
if (read_flag & MOD_MESHSEQ_READ_UV) {
if (params->read_flags & MOD_MESHSEQ_READ_UV) {
std::vector<pxr::UsdGeomPrimvar> primvars = primvarsAPI.GetPrimvars();
@ -888,9 +890,9 @@ Mesh *USDMeshReader::read_mesh(Mesh *existing_mesh,
* the topology is consistent, as in the Alembic importer. */
ImportSettings settings;
settings.read_flag |= read_flag;
settings.read_flag |= params->read_flags;
if (topology_changed(existing_mesh, motionSampleTime)) {
if (topology_changed(existing_mesh, params->time)) {
new_mesh = true;
active_mesh = BKE_mesh_new_nomain_from_template(
existing_mesh, positions_.size(), 0, 0, face_indices_.size(), face_counts_.size());
@ -900,7 +902,7 @@ Mesh *USDMeshReader::read_mesh(Mesh *existing_mesh,
}
}
read_mesh_sample(&settings, active_mesh, motionSampleTime, new_mesh || is_initial_load_);
read_mesh_sample(&settings, active_mesh, params->time, new_mesh || is_initial_load_);
if (new_mesh) {
/* Here we assume that the number of materials doesn't change, i.e. that

3
source/blender/io/usd/intern/usd_reader_mesh.h
View File

@ -49,8 +49,7 @@ class USDMeshReader : public USDGeomReader {
void read_object_data(Main *bmain, double motionSampleTime) override;
struct Mesh *read_mesh(struct Mesh *existing_mesh,
double motionSampleTime,
int read_flag,
const USDMeshReadParams *params,
const char **err_str) override;
bool topology_changed(const Mesh *existing_mesh, double motionSampleTime) override;

9
source/blender/io/usd/intern/usd_reader_nurbs.cc
View File

@ -165,8 +165,7 @@ void USDNurbsReader::read_curve_sample(Curve *cu, const double motionSampleTime)
}
Mesh *USDNurbsReader::read_mesh(struct Mesh * /* existing_mesh */,
const double motionSampleTime,
const int /* read_flag */,
const USDMeshReadParams *params,
const char ** /* err_str */)
{
pxr::UsdGeomCurves curve_prim_(prim_);
@ -177,11 +176,11 @@ Mesh *USDNurbsReader::read_mesh(struct Mesh * /* existing_mesh */,
pxr::VtIntArray usdCounts;
vertexAttr.Get(&usdCounts, motionSampleTime);
vertexAttr.Get(&usdCounts, params->time);
int num_subcurves = usdCounts.size();
pxr::VtVec3fArray usdPoints;
pointsAttr.Get(&usdPoints, motionSampleTime);
pointsAttr.Get(&usdPoints, params->time);
int vertex_idx = 0;
int curve_idx;
@ -205,7 +204,7 @@ Mesh *USDNurbsReader::read_mesh(struct Mesh * /* existing_mesh */,
if (!same_topology) {
BKE_nurbList_free(&curve->nurb);
read_curve_sample(curve, motionSampleTime);
read_curve_sample(curve, params->time);
}
else {
Nurb *nurbs = static_cast<Nurb *>(curve->nurb.first);

3
source/blender/io/usd/intern/usd_reader_nurbs.h
View File

@ -36,8 +36,7 @@ class USDNurbsReader : public USDGeomReader {
void read_curve_sample(Curve *cu, double motionSampleTime);
Mesh *read_mesh(struct Mesh *existing_mesh,
double motionSampleTime,
int read_flag,
const USDMeshReadParams *params,
const char **err_str) override;
};

10
source/blender/io/usd/usd.h
View File

@ -66,6 +66,11 @@ struct USDImportParams {
eUSDMtlNameCollisionMode mtl_name_collision_mode;
};
typedef struct USDMeshReadParams {
double time;
int read_flags;
} USDMeshReadParams;
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 12:31:06 +01:00
Review
Copy Link

"Read" is ambiguous here. Is it past or future tense?

"Read" is ambiguous here. Is it past or future tense?
Sonny Campbell commented 2023-02-10 12:57:36 +01:00
Review
Copy Link

It's neither. It's read as opposed to write. It's usage is everywhere as a parameter in a read_mesh function, so it's very clear what it means from the context.

It's neither. It's read as opposed to write. It's usage is everywhere as a parameter in a `read_mesh` function, so it's very clear what it means from the context.
Sonny Campbell commented 2023-02-10 12:59:07 +01:00
Review
Copy Link

It's also consistent with ABCReadParams for ABC_read_mesh in the alembic import code

It's also consistent with `ABCReadParams` for `ABC_read_mesh` in the alembic import code
Sybren A. Stüvel commented 2023-02-13 11:56:52 +01:00
Review
Copy Link

In that case I would just change the comment to "USD TimeCode in frames", as it then limits itself to just describing the units of measurement, and not the operation that it's involved with. The fact that it's used for reading is clear from the name of the struct.

In that case I would just change the comment to "USD TimeCode in frames", as it then limits itself to just describing the units of measurement, and not the operation that it's involved with. The fact that it's used for reading is clear from the name of the struct.
Sonny Campbell commented 2023-02-13 12:46:14 +01:00
Review
Copy Link

Done

Done
/* The USD_export takes a as_background_job parameter, and returns a boolean.
*
* When as_background_job=true, returns false immediately after scheduling
@ -101,9 +106,8 @@ void USD_get_transform(struct CacheReader *reader, float r_mat[4][4], float time
struct Mesh *USD_read_mesh(struct CacheReader *reader,
struct Object *ob,
struct Mesh *existing_mesh,
double time,
const char **err_str,
int read_flag);
const USDMeshReadParams *params,
const char **err_str);
bool USD_mesh_topology_changed(struct CacheReader *reader,
const struct Object *ob,

10
source/blender/modifiers/intern/MOD_meshsequencecache.cc
View File

@ -245,12 +245,16 @@ static Mesh *modifyMesh(ModifierData *md, const ModifierEvalContext *ctx, Mesh *
# endif
break;
}
case CACHEFILE_TYPE_USD:
case CACHEFILE_TYPE_USD: {
# ifdef WITH_USD
result = USD_read_mesh(
mcmd->reader, ctx->object, mesh, time * FPS, &err_str, mcmd->read_flag);
USDMeshReadParams params = {};
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 12:32:49 +01:00
Review
Copy Link

Since there are multiple places where a USDMeshReadParams is created, it'll be better to have a function that takes the fields as arguments, and returns a USDMeshReadParams struct.

That will cause compiler errors if ever a field was added but not handled in that particular bit of code. It also aids in constness as it allows for const USDMeshReadParams params = newUSDMeshReadParams(bla, bla); here.

Since there are multiple places where a `USDMeshReadParams` is created, it'll be better to have a function that takes the fields as arguments, and returns a `USDMeshReadParams` struct. That will cause compiler errors if ever a field was added but not handled in that particular bit of code. It also aids in `const`ness as it allows for `const USDMeshReadParams params = newUSDMeshReadParams(bla, bla);` here.
Sonny Campbell commented 2023-02-10 13:45:39 +01:00
Review
Copy Link

Done

Done
params.time = time * FPS;
params.read_flags = mcmd->read_flag;
result = USD_read_mesh(mcmd->reader, ctx->object, mesh, &params, &err_str);
# endif
break;
}
case CACHE_FILE_TYPE_INVALID:
break;
}