From e0893f87ac610b0fd4cb1d5450431e95cd1c86d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 9 Nov 2021 19:59:26 +0100 Subject: [PATCH] Trade: another case for the deprecated path not being 100% compatible. Honestly I don't care much, this is just that the original PrimitiveImporter tests started to fail because they expected object3DForName() to return -1 for a 2D name, but that's no longer the case. It would be possible to fix this, but I doubt anyone ever relied on such behavior for 2D scenes, so just add a test that acknowledges this new behavior. --- src/Magnum/Trade/AbstractImporter.cpp | 12 +++- .../Trade/Test/AbstractImporterTest.cpp | 56 +++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index cda75437e..ec15b7ef2 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -589,7 +589,11 @@ Int AbstractImporter::object2DForName(const std::string& name) { Int AbstractImporter::doObject2DForName(const std::string& name) { /* Alias to the new interface. If it returns an ID that's larger than reported 2D object count, then it's probably for a 3D object instead - -- ignore it in that case. */ + -- ignore it in that case. Ideally this would be solved by checking if + the ID is actually present in a 2D scene (and same in doObject2DName()) + but that's a lot of extra code for just a backwards compatibility + feature that almost nobody needs. The only pre-existing 2D importer is + PrimitiveImporter, which is rarely used. */ const Long id = doObjectForName(name); CORRADE_IGNORE_DEPRECATED_PUSH return id < doObject2DCount() ? id : -1; @@ -763,7 +767,11 @@ Int AbstractImporter::object3DForName(const std::string& name) { Int AbstractImporter::doObject3DForName(const std::string& name) { /* Alias to the new interface. If it returns an ID that's larger than reported 3D object count, then it's probably for a 2D object instead - -- ignore it in that case. */ + -- ignore it in that case. Ideally this would be solved by checking if + the ID is actually present in a 3D scene (and same in doObject3DName()) + but that's a lot of extra code for just a backwards compatibility + feature that almost nobody needs. The only pre-existing 2D importer is + PrimitiveImporter, which is rarely used. */ const Long id = doObjectForName(name); CORRADE_IGNORE_DEPRECATED_PUSH return id < doObject3DCount() ? id : -1; diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index 627ba15a9..d549cf7db 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -128,6 +128,7 @@ struct AbstractImporterTest: TestSuite::Tester { void sceneDeprecatedFallbackMultiFunctionObjects3D(); void sceneDeprecatedFallbackObjectCountNoScenes(); void sceneDeprecatedFallbackObjectCountAllSceneImportFailed(); + void sceneDeprecatedFallbackBoth2DAnd3DScene(); #endif void sceneNameNotImplemented(); void objectNameNotImplemented(); @@ -414,6 +415,7 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::sceneDeprecatedFallbackMultiFunctionObjects3D, &AbstractImporterTest::sceneDeprecatedFallbackObjectCountNoScenes, &AbstractImporterTest::sceneDeprecatedFallbackObjectCountAllSceneImportFailed, + &AbstractImporterTest::sceneDeprecatedFallbackBoth2DAnd3DScene, #endif &AbstractImporterTest::sceneForNameOutOfRange, &AbstractImporterTest::objectForNameOutOfRange, @@ -3262,6 +3264,60 @@ void AbstractImporterTest::sceneDeprecatedFallbackObjectCountAllSceneImportFaile CORRADE_COMPARE(importer.object3DCount(), 27); CORRADE_IGNORE_DEPRECATED_POP } + +void AbstractImporterTest::sceneDeprecatedFallbackBoth2DAnd3DScene() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doSceneCount() const override { return 2; } + UnsignedLong doObjectCount() const override { return 7; } + Long doObjectForName(const std::string& name) override { + if(name == "sixth") return 5; + return -1; + } + std::string doObjectName(UnsignedLong id) override { + if(id == 5) return "sixth"; + return {}; + } + Containers::Optional doScene(UnsignedInt id) override { + if(id == 0) return SceneData{SceneObjectType::UnsignedInt, 7, nullptr, { + SceneFieldData{SceneField::Parent, SceneObjectType::UnsignedInt, nullptr, SceneFieldType::Int, nullptr}, + SceneFieldData{SceneField::Translation, SceneObjectType::UnsignedInt, nullptr, SceneFieldType::Vector2, nullptr}, + }}; + if(id == 1) return SceneData{SceneObjectType::UnsignedInt, 7, nullptr, { + SceneFieldData{SceneField::Parent, SceneObjectType::UnsignedInt, nullptr, SceneFieldType::Int, nullptr}, + SceneFieldData{SceneField::Translation, SceneObjectType::UnsignedInt, nullptr, SceneFieldType::Vector3, nullptr}, + }}; + CORRADE_INTERNAL_ASSERT_UNREACHABLE(); + } + } importer; + + CORRADE_IGNORE_DEPRECATED_PUSH + CORRADE_COMPARE(importer.object2DCount(), 7); + CORRADE_COMPARE(importer.object3DCount(), 7); + CORRADE_IGNORE_DEPRECATED_POP + + { + CORRADE_EXPECT_FAIL("No check for whether given object is 2D or 3D is done, so the names are reported for both 2D and 3D objects."); + CORRADE_IGNORE_DEPRECATED_PUSH + CORRADE_COMPARE(importer.object2DForName("sixth"), -1); + CORRADE_COMPARE(importer.object2DName(5), ""); + CORRADE_COMPARE(importer.object3DForName("sixth"), -1); + CORRADE_COMPARE(importer.object3DName(5), ""); + CORRADE_IGNORE_DEPRECATED_POP + } { + /* Just to be sure, verify that the names get really reported for both + instead of some other weird shit happening */ + CORRADE_IGNORE_DEPRECATED_PUSH + CORRADE_COMPARE(importer.object2DForName("sixth"), 5); + CORRADE_COMPARE(importer.object2DName(5), "sixth"); + CORRADE_COMPARE(importer.object3DForName("sixth"), 5); + CORRADE_COMPARE(importer.object3DName(5), "sixth"); + CORRADE_IGNORE_DEPRECATED_POP + } +} #endif void AbstractImporterTest::sceneNameNotImplemented() {