AssimpImporter: normalize image paths

[Squareys]

Hi @mosra !

As described in #101, I have an FBX file "from the wild", which has a relative windows path reference. This currently fails to load on Unix, but works on Windows.

The first commit runs the tests fine. My compiler complained that the symbol in StbImageImporter.cpp#99 does not exist, and since that made no sense whatsoever, I didn't want to waste time on that and I added support for PngImporter (it's might be faster, even).

The second commit adds the test first with EXPECT_FAIL on non-Windows. This is to ensure that the CI would have indeed failed to import the image file.

The final commit fixes the test and removes the EXPECT_FAIL.

Best,
Jonathan

[codecov]

Codecov Report

Merging #102 (96e23fe) into master (d0398ed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 96e23fe differs from pull request most recent head 6f8b377. Consider uploading reports for the commit 6f8b377 to get more accurate results

@@           Coverage Diff           @@
##           master     #102   +/-   ##
=======================================
  Coverage   94.68%   94.68%           
=======================================
  Files         101      101           
  Lines        7644     7645    +1     
=======================================
+ Hits         7238     7239    +1     
  Misses        406      406           

Impacted Files	Coverage Δ
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp	92.00% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0398ed...6f8b377. Read the comment docs.

[mosra]

Thanks!

👍 for putting the test into an earlier separate commit. Good practice, appreciate that. It didn't need to have the EXPECT_FAIL tho, since it's fixed right after :)

My compiler complained that the symbol in StbImageImporter.cpp#99 does not exist

Huh, can you elaborate? I have a fork of stb_image with nothings/stb#950 applied, so chances are it used some other file in your case. The test CMakeLists are deliberately inflexible to keep them small and easy to maintain (in this case I'd need to add the PngImporter variant to all the other importer tests as well) so if it's okay with you I'd skip that one commit.

[Squareys]

It didn't need to have the EXPECT_FAIL tho, since it's fixed right after :)

Well, this makes bisecting easier...

it used some other file in your case

Oh! That could be 🤔

it used some other file in your case

Sure, no problem.

Changes applied!

[mosra]

Well, this makes bisecting easier...

Depends what's the intent, I guess :)

In my case the reason to commit the test first is to have a commit that clearly fails to either compile or to pass the test, to have a verification that there's indeed a problem. Then the immediately following commit has to fix the issue and make things compile / pass tests again. If both commit would pass, then it wouldn't be so obvious (this test has 100 XFAILs already, hard to spot there's one more) and could potentially hide issues like

• the first commit not actually testing the right problem (and the second commit then having no effect)
• the second commit not actually fixing the problem (and the XFAIL accidentally kept there)

---

I dropped the PngImporter commit, finished the rename and merged as d0398ed...d6234a6. Thanks, and I hope that's everything for Assimp bugs, this week at least :D