Skip to content

Commit

Permalink
Trade: reverse AbstractSceneConverter::convertToFile() argument order.
Browse files Browse the repository at this point in the history
It should be input first, output second, like with all other APIs. I
remember I was trying something else here, but that didn't really make
sense in the end. Also took that opportunity to get rid of one
std::string.

The original signature is a deprecated alias to the new one and will be
removed in a future release.
  • Loading branch information
mosra committed Apr 8, 2021
1 parent 8237626 commit 06e039d
Show file tree
Hide file tree
Showing 8 changed files with 52 additions and 24 deletions.
4 changes: 4 additions & 0 deletions doc/changelog.dox
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,10 @@ See also:
@ref MAGNUM_BUILD_DEPRECATED is enabled, the returned type acts as a
@ref Corrade::Containers::Optional but has (deprecated) implicit conversion
to a @ref Corrade::Containers::Pointer to avoid breaking user code.
- The signature of @ref Trade::AbstractSceneConverter::convertToFile() was
changed to have input first and output second, for consistency with other
interfaces, together with a switch to @ref Containers::StringView. The
original signature is marked as deprecated.
- @cpp Vk::hasVkFormat(Magnum::VertexFormat) @ce,
@cpp Vk::hasVkFormat(Magnum::PixelFormat) @ce,
@cpp Vk::hasVkFormat(Magnum::CompressedPixelFormat) @ce,
Expand Down
2 changes: 1 addition & 1 deletion src/Magnum/MeshTools/sceneconverter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,7 @@ used.)")
Debug{} << "Saving output with" << converterName << Debug::nospace << "...";

Duration d{conversionTime};
if(!converter->convertToFile(args.value("output"), *mesh)) {
if(!converter->convertToFile(*mesh, args.value("output"))) {
Error{} << "Cannot save file" << args.value("output");
return 5;
}
Expand Down
16 changes: 12 additions & 4 deletions src/Magnum/Trade/AbstractSceneConverter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
#include <Corrade/Containers/Array.h>
#include <Corrade/Containers/EnumSet.hpp>
#include <Corrade/Containers/Optional.h>
#include <Corrade/Containers/StringStl.h> /* Needed for Directory */
#include <Corrade/Containers/StringView.h>
#include <Corrade/Utility/DebugStl.h>
#include <Corrade/Utility/Directory.h>

Expand All @@ -43,7 +45,7 @@ namespace Magnum { namespace Trade {
std::string AbstractSceneConverter::pluginInterface() {
return
/* [interface] */
"cz.mosra.magnum.Trade.AbstractSceneConverter/0.1"
"cz.mosra.magnum.Trade.AbstractSceneConverter/0.1.1"
/* [interface] */
;
}
Expand Down Expand Up @@ -139,14 +141,20 @@ Containers::Array<char> AbstractSceneConverter::doConvertToData(const MeshData&)
CORRADE_ASSERT_UNREACHABLE("Trade::AbstractSceneConverter::convertToData(): mesh conversion advertised but not implemented", {});
}

bool AbstractSceneConverter::convertToFile(const std::string& filename, const MeshData& mesh) {
bool AbstractSceneConverter::convertToFile(const MeshData& mesh, const Containers::StringView filename) {
CORRADE_ASSERT(features() >= SceneConverterFeature::ConvertMeshToFile,
"Trade::AbstractSceneConverter::convertToFile(): mesh conversion not supported", {});

return doConvertToFile(filename, mesh);
return doConvertToFile(mesh, filename);
}

bool AbstractSceneConverter::doConvertToFile(const std::string& filename, const MeshData& mesh) {
#ifdef MAGNUM_BUILD_DEPRECATED
bool AbstractSceneConverter::convertToFile(const std::string& filename, const MeshData& mesh) {
return convertToFile(mesh, filename);
}
#endif

bool AbstractSceneConverter::doConvertToFile(const MeshData& mesh, const Containers::StringView filename) {
CORRADE_ASSERT(features() >= SceneConverterFeature::ConvertMeshToData, "Trade::AbstractSceneConverter::convertToFile(): mesh conversion advertised but not implemented", false);

const auto data = doConvertToData(mesh);
Expand Down
23 changes: 17 additions & 6 deletions src/Magnum/Trade/AbstractSceneConverter.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ enum class SceneConverterFeature: UnsignedByte {

/**
* Converting a mesh to a file with
* @ref AbstractSceneConverter::convertToFile(const std::string&, const MeshData&).
* @ref AbstractSceneConverter::convertToFile(const MeshData&, Containers::StringView).
*/
ConvertMeshToFile = 1 << 2,

Expand Down Expand Up @@ -171,8 +171,9 @@ checked by the implementation:
@ref SceneConverterFeature::ConvertMeshInPlace is supported.
- The function @ref doConvertToData(const MeshData&) is called only if
@ref SceneConverterFeature::ConvertMeshToData is supported.
- The function @ref doConvertToFile(const std::string&, const MeshData&) is
called only if @ref SceneConverterFeature::ConvertMeshToFile is supported.
- The function @ref doConvertToFile(const MeshData&, Containers::StringView)
is called only if @ref SceneConverterFeature::ConvertMeshToFile is
supported.
@m_class{m-block m-warning}
Expand Down Expand Up @@ -298,14 +299,24 @@ class MAGNUM_TRADE_EXPORT AbstractSceneConverter: public PluginManager::Abstract

/**
* @brief Convert a mesh to a file
* @m_since_latest
*
* Available only if @ref SceneConverterFeature::ConvertMeshToFile or
* @ref SceneConverterFeature::ConvertMeshToData is supported. Returns
* @cpp true @ce on success, prints an error message and returns
* @cpp false @ce otherwise.
* @see @ref features(), @ref convertToData()
*/
bool convertToFile(const std::string& filename, const MeshData& mesh);
bool convertToFile(const MeshData& mesh, Containers::StringView filename);

#ifdef MAGNUM_BUILD_DEPRECATED
/**
* @brief @copybrief convertToFile(const MeshData&, Containers::StringView)
* @m_deprecated_since_latest Use @ref convertToFile(const MeshData&, Containers::StringView)
* instead.
*/
CORRADE_DEPRECATED("use convertToFile(const MeshData&, Containers::StringView) instead") bool convertToFile(const std::string& filename, const MeshData& mesh);
#endif

private:
/**
Expand Down Expand Up @@ -340,13 +351,13 @@ class MAGNUM_TRADE_EXPORT AbstractSceneConverter: public PluginManager::Abstract
virtual Containers::Array<char> doConvertToData(const MeshData& mesh);

/**
* @brief Implementation for @ref convertToFile(const std::string&, const MeshData&)
* @brief Implementation for @ref convertToFile(const MeshData&, Containers::StringView)
*
* If @ref SceneConverterFeature::ConvertMeshToData is supported,
* default implementation calls @ref doConvertToData(const MeshData&)
* and saves the result to given file.
*/
virtual bool doConvertToFile(const std::string& filename, const MeshData& mesh);
virtual bool doConvertToFile(const MeshData& mesh, Containers::StringView filename);

SceneConverterFlags _flags;
};
Expand Down
16 changes: 9 additions & 7 deletions src/Magnum/Trade/Test/AbstractSceneConverterTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
*/

#include <sstream>
#include <Corrade/Containers/StringView.h>
#include <Corrade/Containers/StringStl.h>
#include <Corrade/Containers/Optional.h>
#include <Corrade/TestSuite/Tester.h>
#include <Corrade/TestSuite/Compare/Container.h>
Expand Down Expand Up @@ -192,7 +194,7 @@ void AbstractSceneConverterTest::thingNotSupported() {
converter.convert(mesh);
converter.convertInPlace(mesh);
converter.convertToData(mesh);
converter.convertToFile(Utility::Directory::join(TRADE_TEST_OUTPUT_DIR, "mesh.out"), mesh);
converter.convertToFile(mesh, Utility::Directory::join(TRADE_TEST_OUTPUT_DIR, "mesh.out"));
CORRADE_COMPARE(out.str(),
"Trade::AbstractSceneConverter::convert(): mesh conversion not supported\n"
"Trade::AbstractSceneConverter::convertInPlace(): mesh conversion not supported\n"
Expand Down Expand Up @@ -440,7 +442,7 @@ void AbstractSceneConverterTest::convertMeshToFile() {
struct: AbstractSceneConverter {
SceneConverterFeatures doFeatures() const override { return SceneConverterFeature::ConvertMeshToFile; }

bool doConvertToFile(const std::string& filename, const MeshData& mesh) override {
bool doConvertToFile(const MeshData& mesh, Containers::StringView filename) override {
return Utility::Directory::write(filename, Containers::arrayView( {char(mesh.vertexCount())}));
}
} converter;
Expand All @@ -451,7 +453,7 @@ void AbstractSceneConverterTest::convertMeshToFile() {
Utility::Directory::rm(filename);
CORRADE_VERIFY(!Utility::Directory::exists(filename));

CORRADE_VERIFY(converter.convertToFile(filename, MeshData{MeshPrimitive::Triangles, 0xef}));
CORRADE_VERIFY(converter.convertToFile(MeshData{MeshPrimitive::Triangles, 0xef}, filename));
CORRADE_COMPARE_AS(filename,
"\xef", TestSuite::Compare::FileToString);
}
Expand All @@ -472,7 +474,7 @@ void AbstractSceneConverterTest::convertMeshToFileThroughData() {
CORRADE_VERIFY(!Utility::Directory::exists(filename));

/* doConvertToFile() should call doConvertToData() */
CORRADE_VERIFY(converter.convertToFile(filename, MeshData{MeshPrimitive::Triangles, 0xef}));
CORRADE_VERIFY(converter.convertToFile(MeshData{MeshPrimitive::Triangles, 0xef}, filename));
CORRADE_COMPARE_AS(filename,
"\xef", TestSuite::Compare::FileToString);
}
Expand All @@ -496,7 +498,7 @@ void AbstractSceneConverterTest::convertMeshToFileThroughDataFailed() {
should be printed (the base implementation assumes the plugin does it) */
std::ostringstream out;
Error redirectError{&out};
CORRADE_VERIFY(!converter.convertToFile(filename, MeshData{MeshPrimitive::Triangles, 0xef}));
CORRADE_VERIFY(!converter.convertToFile(MeshData{MeshPrimitive::Triangles, 0xef}, filename));
CORRADE_VERIFY(!Utility::Directory::exists(filename));
CORRADE_COMPARE(out.str(), "");
}
Expand All @@ -512,7 +514,7 @@ void AbstractSceneConverterTest::convertMeshToFileThroughDataNotWritable() {

std::ostringstream out;
Error redirectError{&out};
CORRADE_VERIFY(!converter.convertToFile("/some/path/that/does/not/exist", MeshData{MeshPrimitive::Triangles, 0xef}));
CORRADE_VERIFY(!converter.convertToFile(MeshData{MeshPrimitive::Triangles, 0xef}, "/some/path/that/does/not/exist"));
CORRADE_COMPARE(out.str(),
"Utility::Directory::write(): can't open /some/path/that/does/not/exist\n"
"Trade::AbstractSceneConverter::convertToFile(): cannot write to file /some/path/that/does/not/exist\n");
Expand All @@ -529,7 +531,7 @@ void AbstractSceneConverterTest::convertMeshToFileNotImplemented() {

std::ostringstream out;
Error redirectError{&out};
converter.convertToFile(Utility::Directory::join(TRADE_TEST_OUTPUT_DIR, "mesh.out"), MeshData{MeshPrimitive::Triangles, 6});
converter.convertToFile(MeshData{MeshPrimitive::Triangles, 6}, Utility::Directory::join(TRADE_TEST_OUTPUT_DIR, "mesh.out"));
CORRADE_COMPARE(out.str(), "Trade::AbstractSceneConverter::convertToFile(): mesh conversion advertised but not implemented\n");
}

Expand Down
8 changes: 5 additions & 3 deletions src/MagnumPlugins/AnySceneConverter/AnySceneConverter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

#include "AnySceneConverter.h"

#include <Corrade/Containers/StringView.h>
#include <Corrade/Containers/StringStl.h>
#include <Corrade/PluginManager/Manager.h>
#include <Corrade/PluginManager/PluginMetadata.h>
#include <Corrade/Utility/Assert.h>
Expand All @@ -45,7 +47,7 @@ SceneConverterFeatures AnySceneConverter::doFeatures() const {
return SceneConverterFeature::ConvertMeshToFile;
}

bool AnySceneConverter::doConvertToFile(const std::string& filename, const MeshData& mesh) {
bool AnySceneConverter::doConvertToFile(const MeshData& mesh, const Containers::StringView filename) {
CORRADE_INTERNAL_ASSERT(manager());

/** @todo lowercase only the extension, once Directory::split() is done */
Expand Down Expand Up @@ -80,10 +82,10 @@ bool AnySceneConverter::doConvertToFile(const std::string& filename, const MeshD

/* Try to convert the file (error output should be printed by the plugin
itself) */
return converter->convertToFile(filename, mesh);
return converter->convertToFile(mesh, filename);
}

}}

CORRADE_PLUGIN_REGISTER(AnySceneConverter, Magnum::Trade::AnySceneConverter,
"cz.mosra.magnum.Trade.AbstractSceneConverter/0.1")
"cz.mosra.magnum.Trade.AbstractSceneConverter/0.1.1")
2 changes: 1 addition & 1 deletion src/MagnumPlugins/AnySceneConverter/AnySceneConverter.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class MAGNUM_ANYSCENECONVERTER_EXPORT AnySceneConverter: public AbstractSceneCon

private:
MAGNUM_ANYSCENECONVERTER_LOCAL SceneConverterFeatures doFeatures() const override;
MAGNUM_ANYSCENECONVERTER_LOCAL bool doConvertToFile(const std::string& filename, const MeshData& mesh) override;
MAGNUM_ANYSCENECONVERTER_LOCAL bool doConvertToFile(const MeshData& mesh, Containers::StringView filename) override;
};

}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
*/

#include <sstream>
#include <Corrade/Containers/StringView.h>
#include <Corrade/PluginManager/Manager.h>
#include <Corrade/TestSuite/Tester.h>
#include <Corrade/Utility/Directory.h>
Expand Down Expand Up @@ -92,7 +93,7 @@ void AnySceneConverterTest::detect() {

std::ostringstream out;
Error redirectError{&out};
CORRADE_VERIFY(!converter->convertToFile(data.filename, MeshData{MeshPrimitive::Triangles, 0}));
CORRADE_VERIFY(!converter->convertToFile(MeshData{MeshPrimitive::Triangles, 0}, data.filename));
/* Can't use raw string literals in macros on GCC 4.8 */
#ifndef CORRADE_PLUGINMANAGER_NO_DYNAMIC_PLUGIN_SUPPORT
CORRADE_COMPARE(out.str(), Utility::formatString(
Expand All @@ -108,7 +109,7 @@ void AnySceneConverterTest::unknown() {
Error redirectError{&output};

Containers::Pointer<AbstractSceneConverter> converter = _manager.instantiate("AnySceneConverter");
CORRADE_VERIFY(!converter->convertToFile("mesh.obj", MeshData{MeshPrimitive::Triangles, 0}));
CORRADE_VERIFY(!converter->convertToFile(MeshData{MeshPrimitive::Triangles, 0}, "mesh.obj"));

CORRADE_COMPARE(output.str(), "Trade::AnySceneConverter::convertToFile(): cannot determine the format of mesh.obj\n");
}
Expand Down

0 comments on commit 06e039d

Please sign in to comment.