Page MenuHome

Wavefront: Header files breakup & review update.
Needs ReviewPublic

Authored by Ankit (ankitm) on Jun 3 2020, 3:23 PM.

Details

Summary

This is patch 1 of review update in rB485cc4330aff5f38c8af7874a45082.
D7918: Wavefront: Style guide, comments & review update. has changes
in the cc files.
Changes here:

Rearrangement of struct members in OBJ_data_to_export.

Better comments & documentation.

Linkage: internal wavefront_obj.hh & external IO_wavefront_obj.h.

Move functions to namespace IO::OBJ.

Diff Detail

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

Event Timeline

Ankit (ankitm) requested review of this revision.Jun 3 2020, 3:23 PM
Ankit (ankitm) created this revision.

I only had time to quickly look at your new diff. There are some good improvements, thanks.
One big question I have is: in get_transformed_mesh_vertices, you calloc space for an MVert vector and then put ob_eval->obmat times me_eval->mvert[i].co into that vector,
What I was trying to ask yesterday was: is it not possible to just store me_eval->mvert directly in data_to_export, and then also store ob_eval->obmat there, and then in your write_obj_data function, do the multiplication by obmat just before output? The advantage to that is that you avoid a making a lot of copies of coordinates.

There is a similar question about polygon vertex indices. If you passed me_eval->mloop and me_eval->m_poly through to your writer, you could avoid copying that data too. I'm less sure about this because your packaging into a Polygon certainly makes the code easier to read. But you could write member functions to your export data class that could hide the details of how you get from mpoly and mloop to the answer,

Broke obj.h into obj_extern.h and obj.hh. The former is the public API [...] The latter is for internal use

This is the opposite of how the rest of Blender's code is structured. If a header file is to be used externally, it should be named without "external" suffix, and have the module as prefix. Internal code should be in an intern directory. Since your code is the first new code that's added to the "io" module (before that all submodules were separate), you don't find that structure here yet.

I would suggest naming the external interface file IO_obj.h, and the internal-only file intern/obj.h. Other internal files should also be in intern.

One big question I have is: in get_transformed_mesh_vertices, you calloc space for an MVert vector and then put ob_eval->obmat times me_eval->mvert[i].co into that vector,
What I was trying to ask yesterday was: is it not possible to just store me_eval->mvert directly in data_to_export, and then also store ob_eval->obmat there, and then in your write_obj_data function, do the multiplication by obmat just before output? The advantage to that is that you avoid a making a lot of copies of coordinates.

An added advantage is that the obmat can be multiplied with the axis transformation matrix, and then the coordinates of each vertex only need to be multiplied once.

Ankit (ankitm) abandoned this revision.Jun 4 2020, 1:20 PM

Thank you for reviewing it!

I would suggest naming the external interface file IO_obj.h, and the internal-only file intern/obj.h. Other internal files should also be in intern.

Okay I'll make the changes, commit them directly locally & close this revision.

is it not possible to just store me_eval->mvert directly in data_to_export, and then also store ob_eval->obmat there, and then in your write_obj_data function, do the multiplication by obmat just before output? The advantage to that is that you avoid a making a lot of copies of coordinates.

An added advantage is that the obmat can be multiplied with the axis transformation matrix, and then the coordinates of each vertex only need to be multiplied once.

Those are nice reasons to change it.
I'll make that change to move multiplication to the writers, to do it on the fly.

source/blender/io/obj/obj.hh
30–34 ↗(On Diff #25453)

This is not using the correct format. Set your editor to auto-format on save or run "make format" before you create a patch.

35 ↗(On Diff #25453)

The comment just describes the fields of the struct. I'm missing an indication that this polygon represents the face of a mesh. The comment is also incorrect, because typically a face has multiple vertices and multiple normals, whereas the comment is now singular.

What is more important to include is the motivation as to why this struct exists at all.

38 ↗(On Diff #25453)

Remove the word "particular". It's unnecessary and thus can cause confusion.

39 ↗(On Diff #25453)

Understanding the meaning of "the vertex list already exported" requires understanding the rest of the structure of the exporter. Make sure your comments can be understood without too much other knowledge.

49 ↗(On Diff #25453)

What is this struct used for? It seems to represent mesh data, but then it would not be usable for the case when you want to export multiple meshes.

53 ↗(On Diff #25453)

An object doesn't have vertices, but a mesh (and some other data types) does.

I would reorder things, and put MVert *mvert first, then you can just write

/** Vertices to export. */
MVert *mvert;
/** Number of vertices in mvert. */
int tot_vertices;

The fact that MVert *mvert is a pointer doesn't have to be repeated in a comment, as that's obvious from the code itself.

65–66 ↗(On Diff #25453)

Why are these declared here? They're written as forward declarations, but as they are at the end of the file they aren't needed as forward declarations at all. Just remove them.

source/blender/io/obj/obj_exporter.hh
29 ↗(On Diff #25453)

Documentation! What is this function for?

source/blender/io/obj/obj_extern.h
33 ↗(On Diff #25453)

This doesn't have to be a Doxygen comment.

37 ↗(On Diff #25453)

This doesn't need to be a Doxygen comment.

source/blender/io/obj/obj_file_handler.hh
28 ↗(On Diff #25453)

Documentation! How is this different from exporter_main()? Both will write OBJ data.

Ankit (ankitm) marked 11 inline comments as done.

Review: Added documentation & comments.

Review update. Changes here:

Rearrangement of struct members in OBJ_data_to_export.

Better comments & documentation.

Linkage: internal wavefront_obj.hh & external IO_wavefront_obj.h.

Move functions to namespace IO::OBJ.

This is not using the correct format. Set your editor to auto-format on save or run "make format" before you create a patch.

Yes, I had seen it. make format doesn't pick up new untracked files.
And editor being Xcode has no extensions functionality. I'll use VScode to format it before committing now on.

Ankit (ankitm) edited the summary of this revision. (Show Details)Jun 4 2020, 5:50 PM