Alembic export & import improvements
ClosedPublic

Authored by Sybren A. Stüvel (sybren) on Feb 24 2017, 5:14 PM.

Details

Summary

This patch contains the relatively safe part of my Alembic-related work of the past weeks. There are a lot of cleanups, clarifications and fixes.

Here is a test file

, together with scripts that show how I exported & imported it. Please also take some creativity in creating your own test files, to see what I missed ;-)

T50403 also contains a test file that can be used here. Even though these fixes don't close that ticket yet, the file already imports with much less issues than in master. I'll have to dive deeper into that file to see what's still going wrong, though.

Diff Detail

Repository
rB Blender
Branch
temp-sybren-alembic
Build Status
Buildable 444
Build 444: arc lint + arc unit
Sybren A. Stüvel (sybren) retitled this revision from to Alembic export & import improvements.Feb 24 2017, 5:14 PM
Sybren A. Stüvel (sybren) updated this object.
Sybren A. Stüvel (sybren) set the repository for this revision to rB Blender.
Sybren A. Stüvel (sybren) updated this object.
  • Revert "Alembic import: don't crash on instances" for now, will be included in another review.

Some initial notes:

  1. Should we remove WITH_ALEMBIC_HDF5 from CMake?
  1. T50227 (UV) not fixed by this
  1. I have alembics generated with 2.78 that load differently with your patch.

.blend:

.abc (generated with 2.78):

Look at the direction the monkeys are facing. If I create the alembic with your branch it loads fine in your branch (but has a similar rotation issue if I open it in 2.78).

Some small remarks inline. And a bonus crash report:

I tried with a corrupted file:

and got this segfault:
1​ASAN:DEADLYSIGNAL
2​=================================================================
3​==5782==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000010 (pc 0x7f993715fb10 bp 0x7f98f9551130 sp 0x7f98f9551118 T20)
4​ #0 0x7f993715fb0f (/lib/x86_64-linux-gnu/libpthread.so.0+0x9b0f)
5​ #1 0x7f993608449d in Alembic::Util::v8::mutex::lock() (/opt/lib/alembic/lib/libAlembic.so+0x10e49d)
6​ #2 0x7f99360844f3 in Alembic::Util::v8::scoped_lock::scoped_lock(Alembic::Util::v8::mutex&) (/opt/lib/alembic/lib/libAlembic.so+0x10e4f3)
7​ #3 0x7f993616b64c in Alembic::Ogawa::v8::IStreams::read(unsigned long, unsigned long, unsigned long, void*) (/opt/lib/alembic/lib/libAlembic.so+0x1f564c)
8​ #4 0x7f9936168efe in Alembic::Ogawa::v8::IArchive::init() (/opt/lib/alembic/lib/libAlembic.so+0x1f2efe)
9​ #5 0x7f9936168e55 in Alembic::Ogawa::v8::IArchive::IArchive(std::vector<std::istream*, std::allocator<std::istream*> > const&) (/opt/lib/alembic/lib/libAlembic.so+0x1f2e55)
10​ #6 0x7f9936082f9c in Alembic::AbcCoreOgawa::v8::ArImpl::ArImpl(std::vector<std::istream*, std::allocator<std::istream*> > const&) (/opt/lib/alembic/lib/libAlembic.so+0x10cf9c)
11​ #7 0x7f99360b5951 in Alembic::AbcCoreOgawa::v8::ReadArchive::operator()(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const (/opt/lib/alembic/lib/libAlembic.so+0x13f951)
12​ #8 0x55dfb796952b in open_archive /home/guest/src/blender/blender/source/blender/alembic/intern/abc_archive.cc:45
13​ #9 0x55dfb7969852 in ArchiveReader::ArchiveReader(char const*) /home/guest/src/blender/blender/source/blender/alembic/intern/abc_archive.cc:87
14​ #10 0x55dfb795ee1d in import_startjob /home/guest/src/blender/blender/source/blender/alembic/intern/alembic_capi.cc:595
15​ #11 0x55dfb4dd9a6c in do_job_thread /home/guest/src/blender/blender/source/blender/windowmanager/intern/wm_jobs.c:337
16​ #12 0x55dfb7891b22 in tslot_thread_start /home/guest/src/blender/blender/source/blender/blenlib/intern/threads.c:254
17​ #13 0x7f993715d423 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x7423)
18​ #14 0x7f9930b179be in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xe89be)
19
20​AddressSanitizer can not provide additional info.
21​SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libpthread.so.0+0x9b0f)
22​Thread T20 created by T0 here:
23​ #0 0x7f993a2f8f59 in __interceptor_pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.3+0x30f59)
24​ #1 0x55dfb7891c26 in BLI_insert_thread /home/guest/src/blender/blender/source/blender/blenlib/intern/threads.c:270
25​ #2 0x55dfb4dda1c1 in WM_jobs_start /home/guest/src/blender/blender/source/blender/windowmanager/intern/wm_jobs.c:422
26​ #3 0x55dfb7961239 in ABC_import /home/guest/src/blender/blender/source/blender/alembic/intern/alembic_capi.cc:835
27​ #4 0x55dfb5d0631d in wm_alembic_import_exec /home/guest/src/blender/blender/source/blender/editors/io/io_alembic.c:493
28​ #5 0x55dfb4dc12a2 in wm_handler_fileselect_do /home/guest/src/blender/blender/source/blender/windowmanager/intern/wm_event_system.c:1853
29​ #6 0x55dfb4dc1ce6 in wm_handler_fileselect_call /home/guest/src/blender/blender/source/blender/windowmanager/intern/wm_event_system.c:1940
30​ #7 0x55dfb4dc27bf in wm_handlers_do_intern /home/guest/src/blender/blender/source/blender/windowmanager/intern/wm_event_system.c:2064
31​ #8 0x55dfb4dc2d02 in wm_handlers_do /home/guest/src/blender/blender/source/blender/windowmanager/intern/wm_event_system.c:2142
32​ #9 0x55dfb4dc4540 in wm_event_do_handlers /home/guest/src/blender/blender/source/blender/windowmanager/intern/wm_event_system.c:2415
33​ #10 0x55dfb4da7b10 in WM_main /home/guest/src/blender/blender/source/blender/windowmanager/intern/wm.c:489
34​ #11 0x55dfb4d9e2b5 in main /home/guest/src/blender/blender/source/creator/creator.c:527
35​ #12 0x7f9930a4f2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
36
37​==5782==ABORTING
(to be fair I get a crash with 2.78 as well). I was trying to test one of the std:cerr but I may have over-corrupted my file ;)

source/blender/alembic/intern/abc_object.cc
265

nitpicking, single line between functions

275

If the user should now that, shouldn't we have a way to tell her that there is a message in console?

std:cerr is fine and all, but the average user will never even be aware of what happened

source/blender/alembic/intern/abc_util.cc
217

Bad code style, should indent "case", see https://wiki.blender.org/index.php/Dev:Doc/Code_Style

I didn't have time to test this patch yet. @Sybren A. Stüvel (sybren), did you test it with some of the files I sent you? Technically all of the files should import fine.

Some initial notes:

  1. Should we remove WITH_ALEMBIC_HDF5 from CMake?

I'm partial here. I left it because I had Alembic with HDF5 support for development purposes and, since some other apps do not have Ogawa support, to give people who need HDF5 and can build Blender the option to build with it and expect files to open somewhat properly. Since then, I had to reinstall my OS and I don't have HDF5 anymore and I might not bother to install it again, at least not anytime soon.

  1. T50227 (UV) not fixed by this

I tried to look into this a couple of days ago. From what I gathered, T50227 happens because when converting the DerivedMesh to a mesh upon import the custom data flags are not properly transferred from the DM to the mesh, some update/flag might be missing somewhere. The custom data code is still a bit confusing to me, I can overcome it but if there is an expert around who happens to know that part of the code base very well, I wouldn't mind being schooled ;) This issue seems to also cause some of the meshes in the RealFlow files I sent to @Sybren A. Stüvel (sybren) to not be drawn.

source/blender/alembic/intern/abc_object.cc
275

I've been thinking of some proper error reporting. In these cases, since the export will pretty much always be part of a job (except when launched through Python), I think we could set some boolean value and in export_endjob(...) in alembic_capi.cc use WM_report(...) to tell the user to check the console or so. Something similar is done for the importer already. Also, we should avoid using std::cerr (or std::cout), especially in a separate thread, since it is a global variable; better accumulate all the errors into a single std::ostream and pass everything in one go to std::cerr at the end. I can look into it if you guys want.

Another batch of comments, focusing on the exporter first.

source/blender/alembic/intern/abc_exporter.cc
165

In this case there is no need to change the loop logic.

451–452

I would suggest bring this inside the '''if (parent)''' body.

source/blender/alembic/intern/abc_util.cc
182

please don't promote our floats to double ;)

  • more lenient check on absence of sheer & homogeneous scaling
  • replaced GHash * with std::map<>
LazyDodo (LazyDodo) added inline comments.
source/blender/alembic/intern/abc_util.h
127

const short?

145

const short?

Why the change from GHash to std::map? Gotta ask sergey but IIRC GHash is faster (profiling done for the dependency graph). Also I'd rather use BLI than stdlib whenever possible (less generated code).

Edit, didn't see the commit, that somewhat explains it, but is manageable IMO.

source/blender/alembic/intern/alembic_capi.cc
453

This overwrites the create_empty condition set on line 431

Sybren A. Stüvel (sybren) marked 8 inline comments as done.Mar 1 2017, 1:54 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/alembic/intern/abc_object.cc
275

I think doing error reporting in a better way is a good idea. However, I think it's also out of scope of these changes, which are intended to fix bugs in Alembic (not necessarily improve usability yet). @Kévin Dietrich (kevindietrich) if you can look into this, great.

I've committed some fixups based on all the feedback so far. Before
merging with master I'll squash them into the fixed-up commits.

  • FIXUP no longer using typedef for m_shapes_type
  • FIXUP cleaned up if-parent structure in AbcExporter::createTransformWriter
  • FIXUP code style
  • FIXUP: superfluous create_empty=true
  • FIXUP: float -> short in copy_yup_from_zup

final set of comments (done in peer review with @Sybren A. Stüvel (sybren))

source/blender/alembic/intern/abc_object.cc
266

return parameters can/should have a r_ prefix

source/blender/alembic/intern/abc_transform.cc
154–155

use m_object_name instead

source/blender/alembic/intern/abc_util.cc
240–241

possibly move to caller?

246

consider using r_ prefix for returning matrix

source/blender/alembic/intern/alembic_capi.cc
380

make it bool!

423
- children_claiming_this_object += child_claims_this_object;
+ children_claiming_this_object += child_claims_this_object ? 1 : 0;
440

there are cases when this may happen without being an error. e.g.,: Rinky with hair and a child object.

534–540

add a comment please, and re-consider ghash, we use it everywhere in Blender

Sybren A. Stüvel (sybren) marked 8 inline comments as done.Mar 2 2017, 10:13 AM
Sybren A. Stüvel (sybren) updated this object.
  • FIXUP reinstated GHash *
  • Alembic: added some r_ prefixes for return parameters
  • Alembic: simplified AbcEmptyReader::readObjectData
  • Alembic: removed unnecessary matrix copy
  • Alembic import: moved import-time scaling to different function
  • Alembic import: prevented unnecessary vector scaling
  • FIXUP less sloppy with bool vs. int
  • FIXUP added some comments to visit_object
  • FIXUP removed unnecessary warning
This revision is now accepted and ready to land.Mar 2 2017, 12:13 PM

Committed as rB3748ca432d2 through rB43a910abce5, with extra fixes & improvements.