New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assimp importer #31

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Squareys
Contributor

Squareys commented May 11, 2017

Hello everybody!

This is some code I started working on during the magnum hackathon in January: A scene importer plugin which makes use of assimp!
I didn't have much time until recently to start cleaning this up, but now that it finally compiles, here's a pullrequest with the early work in progress.

Since this is based off of the OpenGexImporter code, there may be copy&paste errors, if you find any, please do leave a comment.

Cheers, Jonathan.

Current State

  • Compiles
  • After coming across a bunch of issues, I decided to manually create files that assimp loads and test the mapping between assimp structures to magnum Trade::* structures rather than testing assimp functionality.

Todo List

  • Tests
  • Fix the 3 TODO comments
  • Implement texture property import
  • More documentation required in "Behaviour and Limitations" section
  • Enable on CI (requires getting assimp on there somehow)
    • Travis
    • AppVeyor
@coveralls

This comment has been minimized.

coveralls commented May 11, 2017

Coverage Status

Coverage remained the same at 87.238% when pulling 1b0b240 on Squareys:assimp-importer into 5c5a641 on mosra:master.

@mosra

This comment has been minimized.

Owner

mosra commented May 14, 2017

set(ASSIMP_LIBRARY_DIR "lib64")
elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
set(ASSIMP_LIBRARY_DIR "lib32")
endif(CMAKE_SIZEOF_VOID_P EQUAL 8)

This comment has been minimized.

@mosra

mosra May 14, 2017

Owner

endif() without the inside, please (I think new CMake versions even warn about that) ;)

This comment has been minimized.

@Squareys

Squareys May 14, 2017

Contributor
  • Done
find_library(ASSIMP_LIBRARY_RELEASE assimp-${ASSIMP_MSVC_VERSION}-mt.lib PATHS ${ASSIMP_LIBRARY_DIR})
find_library(ASSIMP_LIBRARY_DEBUG assimp-${ASSIMP_MSVC_VERSION}-mtd.lib PATHS ${ASSIMP_LIBRARY_DIR})
set(ASSIMP_LIBRARY optimized ${ASSIMP_LIBRARY_RELEASE} debug ${ASSIMP_LIBRARY_DEBUG})

This comment has been minimized.

@mosra

mosra May 14, 2017

Owner

I would just not define ASSIMP_LIBRARY at all -- it's not meant to be used anyway. The same for ASSIMP_LIBRARIES.

This comment has been minimized.

@Squareys

Squareys May 14, 2017

Contributor

So... use a temp variable instead, for adding that to the imported target later?

This comment has been minimized.

@mosra

mosra May 14, 2017

Owner

In the imported target you are using ASSIMP_LIBRARY only if ASSIMP_LIBRARY_DEBUG / _RELEASE is not defined, so I think not defining it in this case shouldn't be a problem.

This comment has been minimized.

@Squareys

Squareys May 16, 2017

Contributor

🤔
I use ASSIMP_LIBRARY if ASSIMP_LIBRARY_DEBUG / _RELEASE are not bth defined. Also I use it with find_package_handle_standard_args. I don't want to require both debug and release libraries to be present. I will try specifying both, maybe it knows how to handle that intelligently.

This comment has been minimized.

@Squareys

Squareys May 16, 2017

Contributor

I think I got it now. But didn't test it with missing Release xor Debug yet.

# This file is part of Magnum.
#
# Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016
# Vladimír Vondruš <mosra@centrum.cz>

This comment has been minimized.

@mosra

mosra May 14, 2017

Owner

Could you put 2017 in all the copyright headers so it's in sync with all the other code? Thank you :)

This comment has been minimized.

@Squareys

Squareys May 14, 2017

Contributor

Sure 👍

  • Done
}
void AssimpImporter::createNodeIndices() {
if(_nodes.empty()) {

This comment has been minimized.

@mosra

mosra May 14, 2017

Owner

🤔

if(!_nodes.empty())? :)

This comment has been minimized.

@Squareys

Squareys May 14, 2017

Contributor

Indeed :D

  • Done
@mosra

This comment has been minimized.

Owner

mosra commented Jun 20, 2017

Just a note to not forget later: Travis and most of the systems out there would probably still have the crashy version of Assimp installed, so in the tests it's needed to skip such a case if OpenDDLParser::getVersion() is less than 0.4.0 (I hope this API is public, though).

@mosra

Reviewed just the buildsystem stuff for now, to account for things from Squareys#1

find_library(ASSIMP_LIBRARY_DEBUG assimp-${ASSIMP_MSVC_VERSION}-mtd.lib PATHS ${ASSIMP_LIBRARY_DIR})
else()
find_library(ASSIMP_LIBRARY_RELEASE libassimp PATHS lib)
find_library(ASSIMP_LIBRARY_DEBUG libassimpd PATHS lib)

This comment has been minimized.

@mosra

mosra Jun 29, 2017

Owner

This should be just assimp and assimpd and PATHS doesn't need to be there at all (this is the default).

This comment has been minimized.

@Squareys

Squareys Jul 11, 2017

Contributor

✔️

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(Assimp DEFAULT_MSG
ASSIMP_LIBRARY_DEBUG
ASSIMP_LIBRARY_RELEASE

This comment has been minimized.

@mosra

mosra Jun 29, 2017

Owner

Use just ASSIMP_LIBRARY here (it's coming from SelectLibraryConfigurations) -- this will make it work properly on systems that don't have both versions installed.

This comment has been minimized.

@Squareys

Squareys Jul 11, 2017

Contributor

✔️

IMPORTED_LOCATION_RELEASE ${ASSIMP_LIBRARY_RELEASE})
else()
set_target_properties(Assimp::Assimp PROPERTIES
IMPORTED_LOCATION ${ASSIMP_LIBRARY_DEBUG} ${ASSIMP_LIBRARY_RELEASE})

This comment has been minimized.

@mosra

mosra Jun 29, 2017

Owner

Should be just ${ASSIMP_LIBRARY}.

This comment has been minimized.

@Squareys

Squareys Jul 11, 2017

Contributor

✔️

INTERFACE_INCLUDE_DIRECTORIES ${ASSIMP_INCLUDE_DIR})
endif()
set(ASSIMP_LIBRARIES ${ASSIMP_LIBRARY})

This comment has been minimized.

@mosra

mosra Jun 29, 2017

Owner

I would not set this variable at all. The user should use the imported targets instead.

(Also remove it from the docs above.)

This comment has been minimized.

@Squareys

Squareys Jul 11, 2017

Contributor

✔️

@@ -14,6 +14,7 @@ addons:
- libpng12-dev
- libqt4-dev
- libdevil-dev
- libassimp-dev
matrix:

This comment has been minimized.

@mosra

mosra Jun 29, 2017

Owner

This is hopefully enough to make it working on macOS as well:

diff --git a/package/ci/travis.yml b/package/ci/travis.yml
index 8906dd8..0e43084 100644
--- a/package/ci/travis.yml
+++ b/package/ci/travis.yml
@@ -162,8 +162,8 @@ install:
 # works. Replace with just `brew install emscripten` when sane again
 - if [ "$TRAVIS_OS_NAME" == "osx" ] && [ "$TARGET" == "emscripten" ]; then brew install https://raw.githubusercontent.com/Homebrew/homebrew-core/53f53f6498ba0f507c443dd4f0c6217937f1ddf2/Formula/emscripten.rb && export LLVM=/usr/local/opt/emscripten/libexec/llvm/bin && emcc; fi
 
-# HarfBuzz
-- if [ "$TRAVIS_OS_NAME" == "osx" ] && [ "$TARGET" == "desktop" ]; then brew install harfbuzz; fi
+# HarfBuzz, Assimp
+- if [ "$TRAVIS_OS_NAME" == "osx" ] && [ "$TARGET" == "desktop" ]; then brew install harfbuzz assimp; fi
 
 script:
 - if [ "$TRAVIS_OS_NAME" == "linux" ] && ( [ "$TARGET" == "desktop" ] || [ "$TARGET" == "desktop-sanitizers" ] ); then ./package/ci/travis-desktop.sh; fi

This comment has been minimized.

@Squareys

Squareys Jul 11, 2017

Contributor

✔️

@@ -0,0 +1 @@
depends=AnyImageImporter

This comment has been minimized.

@mosra

mosra Jun 29, 2017

Owner

More of a reminder for myself (I can do that post-merge):

There should be a bunch of provides= lines here, based on what all formats this supports (at least it should alias all the existing OBJ, COLLADA, Stanford, .. importers), mentioning this also in the docs and then finally updating AnyImporter to detect and handle the new formats.

@mosra mosra referenced this pull request Jun 29, 2017

Closed

Fixed FindAssimp for MacOS #1

@Squareys Squareys force-pushed the Squareys:assimp-importer branch 2 times, most recently from b3084d3 to 896c649 Jul 11, 2017

@coveralls

This comment has been minimized.

coveralls commented Jul 11, 2017

Coverage Status

Coverage decreased (-4.1%) to 82.922% when pulling 896c649 on Squareys:assimp-importer into 6acfc21 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-3.5%) to 83.58% when pulling c4f2e3d on Squareys:assimp-importer into 6acfc21 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-3.07%) to 83.994% when pulling 15b1049 on Squareys:assimp-importer into 6acfc21 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-2.6%) to 84.465% when pulling 512edf8 on Squareys:assimp-importer into 6acfc21 on mosra:master.

@Squareys Squareys force-pushed the Squareys:assimp-importer branch from 21f3a63 to 1998da9 Jul 12, 2017

@coveralls

This comment has been minimized.

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-1.7%) to 85.391% when pulling 1998da9 on Squareys:assimp-importer into 6acfc21 on mosra:master.

set_target_properties(AssimpImporter PROPERTIES POSITION_INDEPENDENT_CODE ON)
endif()
target_include_directories(AssimpImporter PUBLIC ${PROJECT_SOURCE_DIR}/src)
target_link_libraries(AssimpImporter Magnum::Magnum Assimp::Assimp AnyImageImporter)

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

This will fix the Travis failure:

target_link_libraries(AssimpImporter Magnum::Magnum Assimp::Assimp)
if(CORRADE_TARGET_WINDOWS)
    target_link_libraries(AssimpImporter AnyImageImporter)
endif()

To explain: the plugins are built as MODULE libraries on almost all platforms except Windows (where I had some issues way back and didn't have time to look into them yet). That means they can have unresolved symbols, because the plugin manager will handle that on runtime. So it's not needed (not even possible) to link AnyImageImporter to this.

@@ -0,0 +1,257 @@
/*

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

What about the src/MagnumPlugins/AssimpImporter/Test/.Test.cpp.swp file here? ;)

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

Uh oh, seeems like I committed the vim swap file accidentally (on purpose, of course, so that everyone thinkgs I'm a vim-magician, hah :P )

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

✔️

@mosra

It's a lot of comments, but minor ones .. sorry :)

static aiColor3D to(const Vector<3, Float>& other) {
return {other[0], other[1], other[2]};
}

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

Is the to() ever used? I guess not -- I would just remove it.

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

✔️

std::unordered_map<const aiNode*, UnsignedInt> _nodeIndices;
std::unordered_map<const aiNode*, std::pair<Trade::ObjectInstanceType3D, UnsignedInt>> _nodeInstances;
std::unordered_map<std::string, UnsignedInt> _materialIndicesForName;
std::unordered_map<const aiMaterial*, UnsignedInt> _textureIndices;

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

Similarly to the change I proposed in the ObjImporter -- could this all be just one std::unique_ptr<File> that gets populated on opening a file and reset on close? Would remove the need for including AssImp headers and other stuff here.

createNodeIndices();
createMaterialIndices();
}
}

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

Why not call the AbstractImporter::doOpenFile() (which in turn calls doOpenData()) from within doOpenFile() and avoid the code duplication? Then you could also inline the create*Indices() functions.

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

Because I load the file differently in doOpenFile() (As assimp can guess file format based on file ending). Or is this only for the create*Indices() part?

_textureIndices.clear();
}
Int AssimpImporter::doDefaultScene() { return 0; }

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

This should return -1 if there is no default scene.

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

I would guess there always is a root node an therefore always a default scene. As I am not entirely sure, though, I will make this _scene->m_rootNode ? 0 : -1;

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

Did that for doSceneCount, too, so this also makes it consistent

/** Gather child indices */
std::vector<UnsignedInt> children;
children.reserve(node->mNumChildren);
for(auto child : Containers::ArrayView<aiNode*>(node->mChildren, node->mNumChildren)) {

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

Code golf ;)

for(auto child: Containers::arrayView(node->mChildren, node->mNumChildren)) {
if(aiGetVersionMajor() <= 3 && aiGetVersionMinor() < 1) {
/* Cannot load lights with undefined light type yet */
return;

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

I would use CORRADE_SKIP() here so it's visible in the output. Also, like I mentioned on Gitter, use some combined version integer for comparison (this would fail for theoretical version 2.9 for example).

std::unique_ptr<Trade::ObjectData3D> rootObject = importer.object3D(0);
CORRADE_COMPARE(rootObject->children(), {1});
//CORRADE_COMPARE(rootObject->instanceType(), ObjectInstanceType3D::Mesh); // Should be Empty, but Assimp creates mesh for root node for some reason

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

CORRADE_EXPECT_FAIL() then?

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

I think the rootObject has a mesh, but I don't know why (aiScene->mRoot has one already)

Matrix4({0.813798f, 0.469846f, -0.34202f, 0.0f},
{-0.44097f, 0.882564f, 0.163176f, 0.0f},
{0.378522f, 0.0180283f, 0.925417f, 0.0f},
{1.0f, 2.0f, 3.0f, 1.0f}).transposed());

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

Huh. Why transposed? This is how it should look (bottom row being zeros).

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

✔️ Done

void AssimpImporterTest::texture() {
AssimpImporter importer;
CORRADE_VERIFY(importer.openFile(Utility::Directory::join(ASSIMPIMPORTER_TEST_DIR, "embedded_texture.blend")));

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

Naming scheme nitpick (sorry) -- embedded-texture.blend? I'm not using underscores anywhere.

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

✔️ done

CORRADE_COMPARE(importer.textureCount(), 1);
CORRADE_COMPARE(importer.image2DCount(), 1);
CORRADE_SKIP("This test requires using AssimpImporter via a plugin manager which has access to PngImporter for blender embedded image data.");

This comment has been minimized.

@mosra

mosra Jul 13, 2017

Owner

Shouldn't be a problem to manage -- look in the OGEX importer tests how it's done.

This comment has been minimized.

@Squareys

Squareys Jul 13, 2017

Contributor

✔️ Done

@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-2.9%) to 84.195% when pulling 9f4ed47 on Squareys:assimp-importer into 6acfc21 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-2.2%) to 84.829% when pulling 4000326 on Squareys:assimp-importer into 6acfc21 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-1.9%) to 85.201% when pulling 04e4784 on Squareys:assimp-importer into 6acfc21 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-1.6%) to 85.473% when pulling c5c21d6 on Squareys:assimp-importer into 6acfc21 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-1.6%) to 85.49% when pulling 521d85a on Squareys:assimp-importer into 6acfc21 on mosra:master.

Squareys added some commits Jul 13, 2017

modules: Add FindAssimp.cmake
Signed-off-by: Squareys <squareys@googlemail.com>
Add WITH_ASSIMPIMPORTER option
Signed-off-by: Squareys <squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:assimp-importer branch from 521d85a to d6a8327 Jul 13, 2017

@Squareys Squareys changed the title from [WIP] Assimp importer to Assimp importer Jul 13, 2017

@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-1.6%) to 85.49% when pulling d6a8327 on Squareys:assimp-importer into 1f00ec4 on mosra:master.

@Squareys Squareys force-pushed the Squareys:assimp-importer branch from d6a8327 to 1f5eef7 Jul 15, 2017

@coveralls

This comment has been minimized.

coveralls commented Jul 15, 2017

Coverage Status

Coverage decreased (-1.4%) to 85.671% when pulling 1f5eef7 on Squareys:assimp-importer into 1f00ec4 on mosra:master.

@Squareys Squareys force-pushed the Squareys:assimp-importer branch from 1f5eef7 to 0285d76 Jul 15, 2017

Squareys added some commits Jul 13, 2017

ci: Enable WITH_ASSIMPIMPORTER on travis
Signed-off-by: Squareys <squareys@googlemail.com>
AssimpImporter: Add initial implementation
Signed-off-by: Squareys <squareys@googlemail.com>
AssimpImporter: Add tests
Signed-off-by: Squareys <squareys@googlemail.com>
AssimpImporter: Unsupport raw embedded image data
I was not able to find a way to test this.

Signed-off-by: Squareys <squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:assimp-importer branch from 0285d76 to eafeaf8 Jul 15, 2017

@coveralls

This comment has been minimized.

coveralls commented Jul 15, 2017

Coverage Status

Coverage decreased (-1.4%) to 85.671% when pulling 0285d76 on Squareys:assimp-importer into 1f00ec4 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jul 15, 2017

Coverage Status

Coverage decreased (-1.4%) to 85.671% when pulling eafeaf8 on Squareys:assimp-importer into 1f00ec4 on mosra:master.

@mosra

This comment has been minimized.

Owner

mosra commented Jul 23, 2017

Merged with a few changes in 63732bc..245db08.

I reduced the test COLLADA files a bit, integrated the new formats into AnySceneImporter and ignored the memory leak that seems to be present only in the Assimp that's on Travis.

Thank you a lot for this!

@mosra mosra closed this Jul 23, 2017

@Squareys

This comment has been minimized.

Contributor

Squareys commented Jul 24, 2017

Thanks for merging! 🎉

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment