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

Add openState feature to AssimpImporter #37

Merged
merged 2 commits into from Jan 18, 2018

Conversation

3 participants
@costashatz
Contributor

costashatz commented Jan 18, 2018

This PR adds the openState feature to AssimpImporter in order to be able to import an aiScene.

@mosra mosra added this to TODO in Asset management via automation Jan 18, 2018

@mosra

Reviewed :)

@@ -196,6 +196,7 @@ class MAGNUM_TRADE_ASSIMPIMPORTER_EXPORT AssimpImporter: public AbstractImporter
MAGNUM_TRADE_ASSIMPIMPORTER_LOCAL bool doIsOpened() const override;
MAGNUM_TRADE_ASSIMPIMPORTER_LOCAL void doOpenData(Containers::ArrayView<const char> data) override;
MAGNUM_TRADE_ASSIMPIMPORTER_LOCAL void doOpenState(const void* state, const std::string& filePath = {}) override;

This comment has been minimized.

@mosra

mosra Jan 18, 2018

Owner

No need to have the = {} here, it's a private function that's never called from outside.

This comment has been minimized.

@mosra

mosra Jan 18, 2018

Owner

One more thing: can you document what type should be passed to the openState() function in the "Access to internal importer state" section of the class docs?

Besides that, I'm happy :) 👍

@coveralls

This comment has been minimized.

coveralls commented Jan 18, 2018

Coverage Status

Coverage increased (+0.03%) to 85.411% when pulling f2d4b21 on costashatz:assimp_open_state into 5beea61 on mosra:master.

@mosra

mosra approved these changes Jan 18, 2018

costashatz added some commits Jan 17, 2018

Implement openState to open already loaded aiScene
Added tests for new openState

Apply to new AbstractImporter/fix to tests

Better docs in AssimpImporter

@costashatz costashatz force-pushed the costashatz:assimp_open_state branch from b00a457 to 6f1f682 Jan 18, 2018

@mosra mosra merged commit 6f1f682 into mosra:master Jan 18, 2018

0 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
coverage/coveralls Coverage pending from Coveralls.io
Details

Asset management automation moved this from TODO to Done Jan 18, 2018

@mosra

This comment has been minimized.

Owner

mosra commented Jan 18, 2018

Merged, thank you!

@coveralls

This comment has been minimized.

coveralls commented Jan 18, 2018

Coverage Status

Coverage increased (+0.03%) to 85.411% when pulling b00a457 on costashatz:assimp_open_state into 5beea61 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jan 18, 2018

Coverage Status

Coverage increased (+0.03%) to 85.411% when pulling 7199ca6 on costashatz:assimp_open_state into 5beea61 on mosra:master.

@coveralls

This comment has been minimized.

coveralls commented Jan 20, 2018

Coverage Status

Coverage increased (+0.03%) to 85.411% when pulling 6f1f682 on costashatz:assimp_open_state into 5beea61 on mosra:master.

@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