Page MenuHome

Add faster fprintf writer, remove unused headers
Needs RevisionPublic

Authored by Ankit (ankitm) on Mon, Jun 8, 8:53 PM.

Details

Summary

Changes here:

Reverted temporarily to std::vector to keep working, instead of
waiting for D7931 to get committed in master.

Adds an forced inline function for calculation of face/ polygon normals
by averaging vertex normals. I will look for an existing method.
This way, the allocated memory in data_to_export->mvert[i].no is
actually used.
Also, BLI::Vector<uint> face_normal_index is not needed anymore since
we loop over the same polygon list while writing normals, so
their indices will be the same.

Adds a writer method option, fprintf, which is faster than fstream.
With fstream, 478 MB of an ico-sphere with 8 subsurf takes 22 seconds.
with fprintf, the same takes 13 seconds.
With fstream, a 44 MB of cube with 8 subsurf take 2.3 seconds.
with fprintf, the same takes 1.4 seconds.

Adds timing info of the full export directly in console.

Removed unused and repeated headers from wavefront_obj.cc.

Diff Detail

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

Event Timeline

Ankit (ankitm) requested review of this revision.Mon, Jun 8, 8:53 PM
Ankit (ankitm) created this revision.

Rename vertex to vertex_list

correct mistake: arc diff added everything since master

source/blender/io/wavefront_obj/intern/wavefront_obj_exporter.cc
68–69

This function won't work unless data_to_export->mvert has already been allocated.
It is best to document such preconditions. You could say just that; or you could say
"get_transformed_mesh_vertices must have already been called", or you could put
an assert like BLI_assert(data_to_export->mvert != nullptr);
(Though that latter isn't even right, since you don't actually initialize data_to_export->mvert
before calling get_transformed_mesh_vertices.)

Comment about mvert normals memory allocation.

Readability: Replace integers to indicate axes with AXIS_{X,Y,Z}.

Consistency: Use uint with indices, like the vectors of the indices
themselves.

Ankit (ankitm) marked an inline comment as done.Wed, Jun 10, 8:56 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Thu, Jun 18, 4:11 PM

I've added a bunch of comments, but I'm more concerned with the overall design of the code. As I see it now, this is basically what is happening:

  • Iterate over the geometry to export, copying all the mesh data into data_to_export.
  • Pass data_to_export to the format-specific write function, which then iterates over it to do the export.

This approach effectively doubles the memory usage, at least as far as the geometry goes. I thought we discussed creating a more object-oriented approach, where data is just passed to writer functions while the iteration happens, instead of copying it?

source/blender/io/wavefront_obj/IO_wavefront_obj.h
25

Use FILE_MAX from BLI_path_util.h

source/blender/io/wavefront_obj/intern/wavefront_obj.cc
36

For future code, use the functionality in PIL_time.h and PIL_time_utildefines.h.

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

You can only refer to "the" pre-defined vertex list if the reader already knows about this list. This comment also doesn't mean anything, because any list of vertex indices corresponds to some list of vertices.

Are these the vertices of the evaluated mesh?

source/blender/io/wavefront_obj/intern/wavefront_obj_exporter.cc
69

Try to write full English sentences; this sentence is missing a verb. I would write something like this:

Assumes memory for data_to_export->mvert is already allocated. See get_transformed_mesh_vertices().

The assumption that memory has been allocated has little to do with where that allocation happens exactly. It's better to describe this in two sentences instead of trying to put as much info as possible into a single one.

Then again, I think it's not that nice design that the get_transformed_...() functions have different semantics. One allocates the required memory, the other one doesn't, and now there is an inter-dependency between them. I would probably write a third function that allocates the memory, then calls the other two.

71–72

You can make ob_eval const.

85–86

Everything that exists is pre-defined, as it happened before this function is called. Also "get_...." functions should get/return something, and "store_..." functions should store something. Now the comment says one thing but the function name says something else.

This function seems to convert from mesh loops to poligon vertices. This is I think more important than the fact that the vertices are stored by index. This may give some ideas for a better function name.

96

Don't use single-letter variables. poly_index is much more readable than i, and loop_index more readable than j'.

98

Don't re-index the same list three times in this loop. Use a variable like

Polygon & polygon = data_to_export->polygon_list[i];
133–135

This is probably to be able to benchmark either one or the other. Add that to a comment so that this is explicit. Same with the functions below.

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

What is an "axis component"? And isn't there already functionality in Blender that computes face normals?

41

The MALWAYS_INLINE macro is defined in BLI_math_inline.h, so you have to #include that file too. Don't rely on other header files to include this file for you, always include what you need.

55

Everything is "data"; what would be the difference between "write_obj" and "write_obj_data"?

55

Declare data_to_export as const.

59

Mention the file path in error messages, or things will be hell to debug later when you try to export 10000 OBJ files and one of them fails.

63

Don't hard-code Blender's version number. Use BKE_blender_version.h.

63

Don't append trailing whitespace (i.e. the space before the \n).

65

There is no need to document here that the first two characters are the ID code; that's global for Blender.

70

What is the reason to use a pointer here, instead of a reference? You're already doing that for polygons. below.

70

Declare vertex as const.

79–81

face_normal_axis_component() iterates over all the vertices of the face, and computes the normal. This should not be done three times. Just compute the normal once, store it as a float[3], then write it.

This revision now requires changes to proceed.Thu, Jun 18, 4:11 PM
Ankit (ankitm) marked 19 inline comments as done.Fri, Jun 19, 8:03 AM
Ankit (ankitm) added inline comments.
source/blender/io/wavefront_obj/intern/wavefront_obj_exporter.cc
133–135

ftream has been removed since it didn't offer any benefits, and was slower too.

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

I couldn't find anything for the averaging method being used for face normals.
I'll try to remove the need of storing the vertex normals and calculate face normals by cross product of two edges of a polygon.

55

renamed it to write_mesh_objects. Will break it into three functions for opening the file, writing meshes, & writing curves.

Ankit (ankitm) marked 2 inline comments as done.Fri, Jun 19, 8:03 AM