Exporter part of soc 2020 fast Obj IO project
ClosedPublic
Actions

Authored by Howard Trickey (howardt) on Oct 30 2021, 9:23 PM.

Details

Reviewers

Sybren A. Stüvel (sybren)
Ankit Meel (ankitm)

Commits

rB4e44cfa3d996: Add a new C++ version of an exporter for the Wavefront .obj format.

Summary

This is a replacement for the python .obj exporter, written in C++ by Ankit Meel, with some fixes and modifications and added tests by Howard Trickey.

There are some known differences between the output of this exporter and the python one, namely:

The py exporter appends the file name on the first comment (header) line of the output .obj file.
The py exporter has a second comment header line with 'Material Count: <the count>' on it.

I searched the internet to see if these were common or expected in other software's .obj output, and couldn't find that, so I don't think these differences matter. They could be fixed to be exactly like the .py exporter but it would be fiddly to do so.

Other differences:

the py exporter uses 's off' while the new one uses 's 0' to turn off smoothing groups. Both are acceptable according to the standard.
the py exporter uses 'usemtl (null)' to say there is no material; the new one uses 'usemtl'.
the py exporter sorts faces to try to minimize the number of 's' commands needed; the new one does not
the py exporter puts in order: v's, vt's, vn's; the new one puts in order v's. vn's, vt's.
the py exporter uses a dictionary of normals with coordinates rounded to 4 places, to try to minimize the number of separate normals needed; the new one doesn't do this
the py exporter has a 'l' (line) command for every edge, even if they are part of a face, when 'export edges' is enabled; the new on doesn't have that option and always uses 'l' commands but only for loose (wire) edges

Diff Detail

Event Timeline

Howard Trickey (howardt) requested review of this revision.Oct 30 2021, 9:23 PM

Howard Trickey (howardt) created this revision.

Gavriel Lavi (Gavriel5578) awarded a token.Oct 30 2021, 9:59 PM

Benjamin Sauder (kioku) awarded a token.Nov 3 2021, 8:12 PM

Pieter Schiettecatte (Schiette) awarded a token.Nov 4 2021, 1:31 PM

Pieter Schiettecatte (Schiette) added a subscriber: Pieter Schiettecatte (Schiette).

Tomáš Matýs (Asterix) awarded a token.Dec 21 2021, 8:58 PM

Is somewhere built to test this or do we have to compile it with the master? It would be also great if developers find their time to review this, I understand that some are on holiday but later would be great.

Most of the code looks pretty good, thanks for taking over the work Howard!

It doesn't build on recent master (rBdd01ce2cd079e4a77):

/home/sybren/workspace/blender-git/blender/source/blender/editors/io/io_obj.c: In function ‘wm_obj_export_exec’:
/home/sybren/workspace/blender-git/blender/source/blender/editors/io/io_obj.c:108:49: error: ‘struct Main’ has no member named ‘name’
  108 |   export_params.blen_filepath = CTX_data_main(C)->name;
      |                                                 ^~

This changed to ->filepath recently.

source/blender/editors/io/io_obj.h
17–18	I don't think we put dates in there any more. This one is out of date, anyway.
source/blender/io/wavefront_obj/exporter/obj_export_file_writer.cc
40–45	What do the quotes mean? This could do with some rewrapping too.
source/blender/io/wavefront_obj/exporter/obj_exporter.cc
108–112	Curly braces are not necessary in any of these `case` statements. Removing them can remove some visual clutter.
source/blender/io/wavefront_obj/exporter/obj_exporter.hh
35–37	Stealing is bad. This needs more explanation as to what it does & how it behaves.
70–80	These need documentation.
source/blender/io/wavefront_obj/tests/obj_exporter_tests.cc
238	This should get a bit of a comment to explain what it's for. Might also need to be `const`.
239–242	`const`

This revision now requires changes to proceed.Dec 24 2021, 3:32 PM

Gilberto Rodrigues (gilberto_rodrigues) awarded a token.Dec 25 2021, 2:37 PM

Addressed the comments from Sybren's review of Dec 24, 2021.

Thanks for the review. I made the requested changes and updated the Diff. Please look again.

source/blender/editors/io/io_obj.h
17–18	OK looking at some other files in Blender I see newer ones omit this copyright block altogether, so I've done that for all the files in this patch.
source/blender/io/wavefront_obj/exporter/obj_export_file_writer.cc
40–45	The quotes meant that the quoted text came from the document with the hyperlink specified after the quoted text. I moved that hyperlink up front, with text to explain that that was the reference. Also removed the quotes as unnecessary.
source/blender/io/wavefront_obj/exporter/obj_exporter.cc
108–112	The braces are needed int he OB_CURVE case to hold the variable declaration (else get like 'note: crosses initialization of ‘Nurb* nurb’'), so I left that pair but removed the rest.
source/blender/io/wavefront_obj/exporter/obj_exporter.hh
35–37	This was being used to let something like "for (x : y)..." compile where y is an array of unique_ptr's. Since that construct makes copies of the unique_ptr to assign to x, it doesn't work without stealing. But writing it as for (auto & x : y)... works, so I changed the three loops that were doing this to that latter form and deleted this StealUniquePtr struct.

baoyu (baoyu) awarded a token.Dec 29 2021, 3:50 AM

Was hoping to change the code myself so comments might be a bit concise.. but better is just posting them.

source/blender/io/wavefront_obj/exporter/obj_export_file_writer.cc
483	write to backup file first and then rename it ? Just a safety feature that is used for blend file too. Not too urgent.
source/blender/io/wavefront_obj/exporter/obj_export_io.hh
285	Needs to be reported to the user in the UI
source/blender/io/wavefront_obj/exporter/obj_export_mesh.cc
364	assert < totpoly
source/blender/io/wavefront_obj/exporter/obj_export_mtl.cc
96	Rename `linked` to `source`/ `src` socket to match destination-source pair.
360	nodetree could leak.. suggest unique_ptr
source/blender/io/wavefront_obj/exporter/obj_exporter.cc
179	Rename `add_materials` to set/ store etc ?
234	remove this assert `BLI_assert(!"File should be writable by now.");`? Maybe i was being too defensive.
300	send operator cancelled/ failed message to io_obj.c file
source/blender/io/wavefront_obj/tests/obj_exporter_tests.cc
188	Suggest making it ctest -j2 friendly. Same file multiple tests can create race conditions.
219	would `/` path work on windows ?
244	ctest handles debug level itself with `-V`/ `-VV`.
395–396	Can change the default `up_axis`/ `forward_axis` themselves if this is too common

I'm getting this warning:

In file included from /usr/include/string.h:519,
                 from /blender/extern/gtest/include/gtest/internal/gtest-port.h:250,
                 from /blender/extern/gtest/include/gtest/internal/gtest-internal.h:40,
                 from /blender/extern/gtest/include/gtest/gtest.h:62,
                 from /blender/source/blender/io/wavefront_obj/tests/obj_exporter_tests.cc:4:
In function ‘char* strncpy(char*, const char*, size_t)’,
    inlined from ‘void blender::io::obj::obj_exporter_regression_test::compare_obj_export_to_golden(const string&, const string&, const string&, OBJExportParams&)’ at /home/sybren/workspace/blender-git/blender/source/blender/io/wavefront_obj/tests/obj_exporter_tests.cc:289:12:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:95:34: warning: ‘char* __builtin_strncpy(char*, const char*, long unsigned int)’ specified bound 1024 equals destination size [-Wstringop-truncation]
   95 |   return __builtin___strncpy_chk (__dest, __src, __len,
      |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
   96 |       __glibc_objsize (__dest));
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~

Apart from that it looks good to me.

This revision is now accepted and ready to land.Jan 3 2022, 2:29 PM

Don't see anything to block it from bcon1

Paul Larson (GeorgiaPacific) added a subscriber: Paul Larson (GeorgiaPacific).Jan 3 2022, 7:56 PM

Howard Trickey (howardt) mentioned this in rB4e44cfa3d996: Add a new C++ version of an exporter for the Wavefront .obj format..Jan 3 2022, 8:52 PM

The patch as approved by Sybren was commited with rB4e44cfa3d996

Raimund Klink (Raimund58) closed this revision.Jan 26 2022, 3:38 PM

Raimund Klink (Raimund58) added a commit: rB4e44cfa3d996: Add a new C++ version of an exporter for the Wavefront .obj format..

Keep Vertex Order Option is missing from the export settings, is it completely scrapped or will it be added later?

It wasn't clear that "keep vertex order" was useful. If it is, open a bug for that and we will try to add it.