blender
/
blender
137
416
Fork 599
Code Issues 6.3k Pull Requests 773 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 +30 -27
10 changed files with 30 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f8088f1801 - Show all commits

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

@ -468,7 +468,7 @@ 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 USDMeshReadParams *params,
const USDMeshReadParams params,
const char **err_str)
{
USDGeomReader *usd_reader = dynamic_cast<USDGeomReader *>(get_usd_reader(reader, ob, err_str));

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

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

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

@ -36,7 +36,7 @@ class USDCurvesReader : public USDGeomReader {
void read_curve_sample(Curve *cu, double motionSampleTime);
Mesh *read_mesh(struct Mesh *existing_mesh,
const USDMeshReadParams *params,
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;
};

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

@ -20,7 +20,7 @@ class USDGeomReader : public USDXformReader {
}
virtual Mesh *read_mesh(struct Mesh *existing_mesh,
const USDMeshReadParams *params,
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

@ -245,10 +245,10 @@ void USDMeshReader::read_object_data(Main *bmain, const double motionSampleTime)
is_initial_load_ = true;
USDMeshReadParams params = {};
params.time = motionSampleTime;
params.motion_sample_time = motionSampleTime;
params.read_flags = import_params_.mesh_read_flag;
Mesh *read_mesh = this->read_mesh(mesh, &params, nullptr);
Mesh *read_mesh = this->read_mesh(mesh, params, nullptr);
is_initial_load_ = false;
if (read_mesh != mesh) {
@ -275,7 +275,7 @@ void USDMeshReader::read_object_data(Main *bmain, const double motionSampleTime)
}
USDXformReader::read_object_data(bmain, motionSampleTime);
}
} // namespace blender::io::usd
bool USDMeshReader::valid() const
{
@ -820,7 +820,7 @@ void USDMeshReader::readFaceSetsSample(Main *bmain, Mesh *mesh, const double mot
}
Mesh *USDMeshReader::read_mesh(Mesh *existing_mesh,
const USDMeshReadParams *params,
const USDMeshReadParams params,
const char ** /* err_str */)
{
if (!mesh_prim_) {
@ -837,7 +837,7 @@ Mesh *USDMeshReader::read_mesh(Mesh *existing_mesh,
std::vector<pxr::TfToken> uv_tokens;
/* Currently we only handle UV primvars. */
if (params->read_flags & MOD_MESHSEQ_READ_UV) {
if (params.read_flags & MOD_MESHSEQ_READ_UV) {
std::vector<pxr::UsdGeomPrimvar> primvars = primvarsAPI.GetPrimvars();
@ -890,9 +890,9 @@ Mesh *USDMeshReader::read_mesh(Mesh *existing_mesh,
* the topology is consistent, as in the Alembic importer. */
ImportSettings settings;
settings.read_flag |= params->read_flags;
settings.read_flag |= params.read_flags;
if (topology_changed(existing_mesh, params->time)) {
if (topology_changed(existing_mesh, params.motion_sample_time)) {
new_mesh = true;
active_mesh = BKE_mesh_new_nomain_from_template(
existing_mesh, positions_.size(), 0, 0, face_indices_.size(), face_counts_.size());
@ -902,7 +902,7 @@ Mesh *USDMeshReader::read_mesh(Mesh *existing_mesh,
}
}
read_mesh_sample(&settings, active_mesh, params->time, new_mesh || is_initial_load_);
read_mesh_sample(&settings, active_mesh, params.motion_sample_time, new_mesh || is_initial_load_);
if (new_mesh) {
/* Here we assume that the number of materials doesn't change, i.e. that
@ -914,7 +914,7 @@ Mesh *USDMeshReader::read_mesh(Mesh *existing_mesh,
bke::MutableAttributeAccessor attributes = active_mesh->attributes_for_write();
bke::SpanAttributeWriter<int> material_indices =
attributes.lookup_or_add_for_write_span<int>("material_index", ATTR_DOMAIN_FACE);
assign_facesets_to_material_indices(motionSampleTime, material_indices.span, &mat_map);
assign_facesets_to_material_indices(params.motion_sample_time, material_indices.span, &mat_map);
material_indices.finish();
}
}

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

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

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

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

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

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

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

@ -66,9 +66,12 @@ struct USDImportParams {
eUSDMtlNameCollisionMode mtl_name_collision_mode;
};
/* This struct is in place to store the mesh sequence parameters needed when reading a data from a
* usd file for the mesh sequence cache.
*/
typedef struct USDMeshReadParams {
double time;
int read_flags;
double motion_sample_time; /* Read USD TimeCode in frames. */
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
int read_flags; /* MOD_MESHSEQ_xxx value that is set from MeshSeqCacheModifierData.read_flag. */
} USDMeshReadParams;
/* The USD_export takes a as_background_job parameter, and returns a boolean.
@ -106,7 +109,7 @@ 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,
const USDMeshReadParams *params,
const USDMeshReadParams params,
const char **err_str);
bool USD_mesh_topology_changed(struct CacheReader *reader,

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

@ -248,10 +248,10 @@ static Mesh *modifyMesh(ModifierData *md, const ModifierEvalContext *ctx, Mesh *
case CACHEFILE_TYPE_USD: {
# ifdef WITH_USD
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.motion_sample_time = time * FPS;
params.read_flags = mcmd->read_flag;
result = USD_read_mesh(mcmd->reader, ctx->object, mesh, &params, &err_str);
result = USD_read_mesh(mcmd->reader, ctx->object, mesh, params, &err_str);
# endif
break;
}