Page MenuHome

Wavefront: Refactor as OOP style
Needs ReviewPublic

Authored by Ankit (ankitm) on Mon, Jun 22, 10:29 AM.

Details

Summary

Refactor: arrange the code in OOP style.

The OBJWriter class' one instance writes to one file.
All OBJWriter handles is writing to the file after calling functions
of OBJMesh which collect the required data.

OBJNurbs and OBJMaterial will be added soon too.

Diff Detail

Repository
rB Blender
Branch
soc-2020-io-curves (branched from master)
Build Status
Buildable 8690
Build 8690: arc lint + arc unit

Event Timeline

Ankit (ankitm) requested review of this revision.Mon, Jun 22, 10:29 AM
Ankit (ankitm) created this revision.
Ankit (ankitm) retitled this revision from Renaming: Specify mesh objects instead of object to Refactor as OOP style, add curves support..Mon, Jun 22, 10:35 AM
Ankit (ankitm) edited the summary of this revision. (Show Details)
Ankit (ankitm) retitled this revision from Refactor as OOP style, add curves support. to Wavefront: Refactor as OOP style, add curves support..

Probably It'll be best to read the code locally, and use this revision to discuss changes.
git checkout soc-2020-io-performance
arc patch D8089

  • Only keep refactor in this differential.
Ankit (ankitm) retitled this revision from Wavefront: Refactor as OOP style, add curves support. to Wavefront: Refactor as OOP style.Mon, Jun 22, 2:59 PM
Ankit (ankitm) edited the summary of this revision. (Show Details)

There are a few different patterns used within OBJMesh:

  • get_mesh_eval() reads from member properties, and writes to them as well.
  • triangulate_mesh() reads from a parameter and returns its data as return value.

I would either use one approach or the other, but not mix the two. triangulate_mesh() is called as me_eval = triangulate_mesh(me_eval). If you move triangulate_mesh() into the OBJMesh class, it can just access what it needs there. That would also allow that function to free the me_mesh that it triangulated if me_eval_needs_free was already set to true, and then set me_eval_needs_free = true for itself. That way the functionality of triangulating the mesh is a bit more self-contained.

Naming wise, I don't think ob_mesh should refer to an OBJMesh object. ob is used for Blender Objects, and should not be used for anything else. Same for mesh, that should only refer to Blender meshes.

The concept of "Curve-like Mesh" is also unknown to me. What does it mean?

source/blender/io/wavefront_obj/intern/wavefront_obj.hh
165

me_eval is a very generic name. Probably export_mesh_eval is better.

165

Make everything as private as possible. For example, just because an OBJMesh instance needs export_params, that doesn't mean that other code should be able to access & modify those.

Add a _ suffix to the private names.

177

coords is an output parameter, so prefix the name with r_. Same for other functions.

source/blender/io/wavefront_obj/intern/wavefront_obj_exporter.cc
173–174

Instead of looping over all layers, just use CustomData_get_layer(&me_eval->ldata, CD_MLOOPUV) to get the layer you need.

173–174

I think this can be cleaned up by using a few extra variables, and by using an implicit construction of the std::array<float, 2>:

const int loopstart = mpoly[uv_vert->poly_index].loopstart;
const float[2] vert_uv_coords = mloopuv[loopstart + uv_vert->loop_of_poly_index];
uv_coords.push_back({vert_uv_coords[0], vert_uv_coords[1]});
174–175

There is no need to declare these variables. It makes it harder to understand the rest of the code in this function.

205

Indices are 0-based or 1-based (indicating the index of the first element). The total number of vertices cannot be 0-based or 1-based.

I don't understand why there is a need to keep tot_uv_vertices around. It should be the same as uv_coords.size(), right?

222–224

Don't index multiple times, just store me_eval->mvert[(mloop + i)->v] in a temporary variable.

227–228

Don't use designated initialisers. It works in some cases, on some compilers, but it's not part of the C++ standard.

228

What are "curve-like meshes"?

234–238

I don't understand what is happening here. There is no curve, just a mesh. And it doesn't check for any "this is cyclic" flag.

249

Here object changes semantics, from "original object" to "evaluated object". This should not be done with the same name, as you can then never be sure what it points to (unless you really trace all the possible calls to get_mesh_eval() and compare that with all the uses of objmesh->object, which you don't want).

object is a very generic name to use. Something more explicit like export_object_eval is better.

I also doubt that this is the right place for setting that property. After all, the OBJMesh() cannot work without having an object to export, or without its export parameters. This means that these should be passed to the constructor instead.

265–267

Naming: Make a distinction between "mesh" and "mesh writer for the OBJ format". export_mesh reads like it's a mesh, but it's not.

268–269

Use LISTBASE_FOREACH or ListBaseWrapper.

273–276

See comment above: the OBJMesh can't do its work without these, so just pass them to the constructor.

296–321

This accesses many internals of OBJMesh, indicating that this functionality should actually be moved into a member function of OBJMesh.

source/blender/io/wavefront_obj/intern/wavefront_obj_file_handler.cc
93

Avoid comments that repeat the function declaration.

103

Avoid comments that repeat the function declaration.

104

This should either be done in the destructor, or at least the class should have a destructor that calls this function if necessary.

159

The code below can probably be simplified to something like this:

for (uint i = 0; i < ob_mesh.tot_poly_normals; i++) {
  ob_mesh.calc_poly_vertex_indices(vertex_indices, i);
  if (ob_mesh.export_params->export_normals) {
    ob_mesh.calc_poly_normal_indices(normal_indices, i);
  }
  const MPoly &poly_to_write = ob_mesh.me_eval->mpoly[i];

  ...
}

The code on the ... could also be simplified if you give the four write_vert_..._indices() functions the same signature. Once you've done that, you can use a pointer to the appropriate function (based on the export_normals and export_uv params) and call that within the loop.

This is not something pressing, but could be done if you have some spare time ;-)

source/blender/io/wavefront_obj/intern/wavefront_obj_file_handler.hh
33–35

This is C++, use an enum class instead.

38

Same as with OBJWriter, make everything as private as possible.

Ankit (ankitm) marked 21 inline comments as done.
Ankit (ankitm) edited the summary of this revision. (Show Details)
  • Use blender::Vector and Array instead of std
  • Optimisation: reserve memory early; use UNLIKELY
  • Review update: move more things to private access

Add constructors and destructors for both OBJWriter and OBJMesh classes

Move more variables to private and access the required via getters.

Remove unnecessary variables.

Rename confusing variables: ob_mesh to export_mesh_data etc.

Ankit (ankitm) added inline comments.Tue, Jun 23, 9:06 PM
source/blender/io/wavefront_obj/intern/wavefront_obj_exporter.cc
228

Clarified it: curves converted to meshes when export_curves_as_nurbs is false.

234–238

When a curve is converted to mesh, it doesn't have any cyclic flags anymore. So I thought me_eval->totvert == me_eval->totedge is a good test for cyclicality. When cyclic, last edge connects to the first edge, otherwise it's open ended.

source/blender/io/wavefront_obj/intern/wavefront_obj_file_handler.cc
93

Should I put the init_writer() in constructor and use exceptions to catch if the file opening failed ?

I would either use one approach or the other, but not mix the two. triangulate_mesh() is called as me_eval = triangulate_mesh(me_eval). If you move triangulate_mesh() into the OBJMesh class, it can just access what it needs there. That would also allow that function to free the me_mesh that it triangulated if me_eval_needs_free was already set to true, and then set me_eval_needs_free = true for itself. That way the functionality of triangulating the mesh is a bit more self-contained.

I set both of them void and moved inside the OBJMesh class too. The function arguments also have different names so that object = some_func(object) doesn't happen.
For freeing, I added a destruct function since a vector of exportable_meshes will not call destructors by itself. New mesh is also made for curves converted to mesh, so freeing is not unique to triangulation.

Naming wise, I don't think ob_mesh should refer to an OBJMesh object. ob is used for Blender Objects, and should not be used for anything else. Same for mesh, that should only refer to Blender meshes.

obj_mesh_data, export_object, _export_mesh_eval, _export_object_eval are used now.

The concept of "Curve-like Mesh" is also unknown to me. What does it mean?

Curves (bezier, circle), when export_as_nurbs is false, are converted to mesh so that edges can be exported. But someone reading a comment might not know that it happens elsewhere. So added descriptive comments wherever required.