Handle merges where GLTF resources are in different directories #7

erikdahlstrom · 2018-01-11T10:52:01Z

No description provided.

msftclas · 2018-01-11T10:52:11Z

All committers have signed the CLA.

robertos · 2018-01-11T20:38:07Z

WindowsMRAssetConverter/FileSystem.cpp

+std::wstring FileSystem::GetRelativePathWithTrailingSeparator(std::wstring from, std::wstring to)
+{
+    wchar_t outPath[MAX_PATH] = L"";
+    if (PathRelativePathTo(outPath, from.c_str(), FILE_ATTRIBUTE_DIRECTORY, to.c_str(), FILE_ATTRIBUTE_DIRECTORY))


This API will only work on desktop. Can you please use std::filesystem instead?

Ok, np. I'll take that as a go-ahead to replace the other two shlwapi calls (PathFindFileName, PathRemoveExtension) that were already in the code.

A closer inspection reveals that msvc doesn't support std::filesystem::relative (c++17), which would be the natural replacement for PathRelativePathTo. I was really hoping to avoid re-implementing that.

robertos · 2018-01-11T20:38:52Z

glTF-Toolkit/glTF-Toolkit.vcxproj

@@ -148,8 +148,7 @@
      <GenerateDebugInformation>true</GenerateDebugInformation>
    </Link>
    <Lib>
-      <AdditionalDependencies>
-      </AdditionalDependencies>
+      <AdditionalDependencies>shlwapi.lib</AdditionalDependencies>


Please remove - this doesn't work with UWP

robertos · 2018-01-11T20:39:20Z

glTF-Toolkit/inc/GLTFLODUtils.h

@@ -30,7 +30,7 @@ namespace Microsoft::glTF::Toolkit
        /// </summary>
        /// <returns>The primary GLTF Document with the inserted LOD node.</returns>
        /// <param name="docs">A vector of glTF documents to merge as LODs. The first element of the vector is assumed to be the primary LOD.</param>
-        static GLTFDocument MergeDocumentsAsLODs(const std::vector<GLTFDocument>& docs);
+        static GLTFDocument MergeDocumentsAsLODs(const std::vector<GLTFDocument>& docs, const std::vector<std::wstring>& relativePaths);


This should have an option without this parameter that assumes everything is in the same directory

robertos · 2018-01-11T20:39:37Z

glTF-Toolkit/inc/GLTFLODUtils.h

@@ -40,7 +40,7 @@ namespace Microsoft::glTF::Toolkit
        /// <param name="docs">A vector of glTF documents to merge as LODs. The first element of the vector is assumed to be the primary LOD.</param>
        /// <param name="screenCoveragePercentages">A vector with the screen coverage percentages corresponding to each LOD. If the size of this 
        /// vector is larger than the size of <see name="docs" />, lower coverage values will cause the asset to be invisible.</param>
-        static GLTFDocument MergeDocumentsAsLODs(const std::vector<GLTFDocument>& docs, const std::vector<double>& screenCoveragePercentages);
+        static GLTFDocument MergeDocumentsAsLODs(const std::vector<GLTFDocument>& docs, const std::vector<std::wstring>& relativePaths, const std::vector<double>& screenCoveragePercentages);


This should have an option without this parameter that assumes everything is in the same directory

robertos · 2018-01-11T20:42:51Z

glTF-Toolkit/src/GLTFLODUtils.cpp

@@ -375,7 +386,7 @@ GLTFDocument GLTFLODUtils::MergeDocumentsAsLODs(const std::vector<GLTFDocument>&

    for (size_t i = 1; i < docs.size(); i++)
    {
-        gltfPrimary = AddGLTFNodeLOD(gltfPrimary, lods, docs[i]);
+        gltfPrimary = AddGLTFNodeLOD(gltfPrimary, lods, docs[i], relativePaths[i]);


the first element of relativePaths is always ignored. Maybe the input should be only the non-lod0 relative paths?

true, I'll make it so

This removes the shlwapi.lib dependency.

Add some documentation. Revert the changes to the tests since passing relativePaths is no longer required.

robertos

Thanks a lot for your work on this! :)

robertos · 2018-01-18T17:22:00Z

glTF-Toolkit/src/GLTFLODUtils.cpp

@@ -16,6 +16,8 @@
 #include <algorithm>
 #include <iostream>
 #include <set>
+#include <codecvt>


<codecvt> is deprecated: www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0618r0.html

In general I've been using the naïve conversion from wstring to string elsewhere. I don't have a great solution for this - it's unfortunate that the GLTF SDK works with std::string everywhere, while files can have non-ansi chars.
If this conversion is the best, we should put it in a helper function and use it everywhere (probably with a unit test to ensure it does what we want)

Given that the GLTF spec states that "JSON must use UTF-8 encoding without BOM." my patch assumes that the std::string entries are UTF-8 encoded. I think that using codecvt way is better than the naïve approach, and agree that unit-testing this would be good.

Not sure about the timeframe for getting a c++ standard replacement for codecvt, another option would be to include a third-party lib e.g libiconv.

robertos · 2018-01-18T17:28:59Z

glTF-Toolkit/inc/pch.h

@@ -24,6 +24,7 @@
 #include <windows.h>
 #include <wincodec.h>
 #include <pathcch.h>
+#include <shlwapi.h>


Not needed?

Consider it gone.

robertos · 2018-01-18T17:32:19Z

WindowsMRAssetConverter/WindowsMRAssetConverter.cpp

@@ -15,6 +15,7 @@

 #include "CommandLine.h"
 #include "FileSystem.h"
+#include <filesystem>


Already included in FileSystem.h? Otherwise this should be there?

Sure, will move it there.

Handle resources with the same name (prepend relative path).

2ad7da5

Fix linking on x86.

2a05d17

robertos suggested changes Jan 11, 2018

View reviewed changes

Erik DahlstrÃ¶m added 5 commits January 15, 2018 17:02

Replace PathIsRelative with std::filesystem::path::is_relative.

5edc90d

This removes the shlwapi.lib dependency.

Use default argument for relativePaths.

bf6ab58

Add some documentation. Revert the changes to the tests since passing relativePaths is no longer required.

Updated to follow changes in GLTFLODUtils::MergeDocumentsAsLODs.

5a6437f

Implement GetRelativePathWithTrailingSeparator using std::filesystem.

5f9d3bf

Remove shlwapi.lib from WindowsMRAssetConverter project.

75aadf9

robertos suggested changes Jan 18, 2018

View reviewed changes

Erik Dahlström added 2 commits January 22, 2018 08:46

Remove redundant includes.

0927c52

Move the <filesystem> include into "filesystem.h".

119d02a

robertos approved these changes Feb 6, 2018

View reviewed changes

robertos merged commit 9cc0960 into microsoft:master Feb 6, 2018

erikdahlstrom deleted the relative_path_hack branch February 22, 2018 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle merges where GLTF resources are in different directories #7

Handle merges where GLTF resources are in different directories #7

erikdahlstrom commented Jan 11, 2018

msftclas commented Jan 11, 2018 •

edited

robertos Jan 11, 2018

erikdahlstrom Jan 15, 2018

erikdahlstrom Jan 15, 2018

robertos Jan 11, 2018

erikdahlstrom Jan 15, 2018

robertos Jan 11, 2018

robertos Jan 11, 2018

robertos Jan 11, 2018

erikdahlstrom Jan 15, 2018

robertos left a comment

robertos Jan 18, 2018

robertos Jan 18, 2018

erikdahlstrom Jan 22, 2018

robertos Jan 18, 2018

erikdahlstrom Jan 22, 2018

robertos Jan 18, 2018

erikdahlstrom Jan 22, 2018

Handle merges where GLTF resources are in different directories #7

Handle merges where GLTF resources are in different directories #7

Conversation

erikdahlstrom commented Jan 11, 2018

msftclas commented Jan 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msftclas commented Jan 11, 2018 •

edited