blender
/
blender
134
399
Fork 581
Code Issues 6.3k Pull Requests 754 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 +56 -45
12 changed files with 56 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -475,12 +475,19 @@ static USDPrimReader *get_usd_reader(CacheReader *reader,
return usd_reader;
}
USDMeshReadParams create_mesh_read_params(const double motion_sample_time, const int read_flags)
{
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`
USDMeshReadParams params = {};
params.motion_sample_time = motion_sample_time;
params.read_flags = read_flags;
return params;
}
struct Mesh *USD_read_mesh(struct CacheReader *reader,
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));
@ -488,7 +495,7 @@ struct Mesh *USD_read_mesh(struct CacheReader *reader,
return nullptr;
}
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.motion_sample_time);
int num_subcurves = usdCounts.size();
pxr::VtVec3fArray usdPoints;
pointsAttr.Get(&usdPoints, motionSampleTime);
pointsAttr.Get(&usdPoints, params.motion_sample_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.motion_sample_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,
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,
USDMeshReadParams params,
const char **err_str) = 0;
virtual bool topology_changed(const Mesh * /* existing_mesh */, double /* motionSampleTime */)

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

@ -194,8 +194,9 @@ 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);
const USDMeshReadParams params = create_mesh_read_params(motionSampleTime, import_params_.mesh_read_flag);
Mesh *read_mesh = this->read_mesh(mesh, params, nullptr);
is_initial_load_ = false;
if (read_mesh != mesh) {
@ -222,7 +223,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
{
@ -767,8 +768,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_) {
@ -785,7 +785,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();
@ -838,9 +838,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.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());
@ -850,7 +850,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.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
@ -862,7 +862,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();
}
}

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,
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.motion_sample_time);
int num_subcurves = usdCounts.size();
pxr::VtVec3fArray usdPoints;
pointsAttr.Get(&usdPoints, motionSampleTime);
pointsAttr.Get(&usdPoints, params.motion_sample_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.motion_sample_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,
USDMeshReadParams params,
const char **err_str) override;
};

11
source/blender/io/usd/intern/usd_reader_shape.cc
View File

@ -45,9 +45,10 @@ void USDShapeReader::create_object(Main *bmain, double /*motionSampleTime*/)
void USDShapeReader::read_object_data(Main *bmain, double motionSampleTime)
{
const USDMeshReadParams params = create_mesh_read_params(motionSampleTime,
import_params_.mesh_read_flag);
Mesh *mesh = (Mesh *)object_->data;
Mesh *read_mesh = this->read_mesh(
mesh, motionSampleTime, import_params_.mesh_read_flag, nullptr);
Mesh *read_mesh = this->read_mesh(mesh, params, nullptr);
if (read_mesh != mesh) {
BKE_mesh_nomain_to_mesh(read_mesh, mesh, object_);
@ -124,8 +125,7 @@ bool USDShapeReader::read_mesh_values(double motionSampleTime,
}
Mesh *USDShapeReader::read_mesh(struct Mesh *existing_mesh,
double motionSampleTime,
int /*read_flag*/,
const USDMeshReadParams params,
const char ** /*err_str*/)
{
pxr::VtIntArray face_indices;
@ -136,7 +136,8 @@ Mesh *USDShapeReader::read_mesh(struct Mesh *existing_mesh,
}
/* Should have a good set of data by this point-- copy over. */
Mesh *active_mesh = mesh_from_prim(existing_mesh, motionSampleTime, face_indices, face_counts);
Mesh *active_mesh = mesh_from_prim(
existing_mesh, params.motion_sample_time, face_indices, face_counts);
if (active_mesh == existing_mesh) {
return existing_mesh;
}

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

@ -48,8 +48,7 @@ class USDShapeReader : public USDGeomReader {
void create_object(Main *bmain, double /*motionSampleTime*/) override;
void read_object_data(Main *bmain, double motionSampleTime) override;
Mesh *read_mesh(Mesh *existing_mesh,
double motionSampleTime,
int /*read_flag*/,
USDMeshReadParams params,
const char ** /*err_str*/) override;
bool is_time_varying();

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

@ -86,6 +86,16 @@ struct USDImportParams {
bool import_all_materials;
};
/* 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 motion_sample_time; /* USD TimeCode in frames. */
int read_flags; /* MOD_MESHSEQ_xxx value that is set from MeshSeqCacheModifierData.read_flag. */
} USDMeshReadParams;
USDMeshReadParams create_mesh_read_params(double motion_sample_time, int read_flags);
/* The USD_export takes a as_background_job parameter, and returns a boolean.
*
* When as_background_job=true, returns false immediately after scheduling
@ -121,9 +131,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);
USDMeshReadParams params,
const char **err_str);
bool USD_mesh_topology_changed(struct CacheReader *reader,
const struct Object *ob,

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

@ -245,12 +245,13 @@ 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);
const USDMeshReadParams params = create_mesh_read_params(time * FPS, mcmd->read_flag);
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
result = USD_read_mesh(mcmd->reader, ctx->object, mesh, params, &err_str);
# endif
break;
}
case CACHE_FILE_TYPE_INVALID:
break;
}