Improve handling of shared materials between LODs #14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ware device is found. (microsoft#11) * Use the WARP software fallback in release builds too. Fixes issue microsoft#10. * Revert "Use the WARP software fallback in release builds too. Fixes issue microsoft#10." This reverts commit 5cc40ab. * Handle lack of GPU device in GLTFTextureCompressionUtils. Fixes issue microsoft#10.
…ft#13) * Update all NuGet packages to latest and fix breaking changes * Update README.md Removes version from Microsoft.glTF.CPP link
- Pack textures only once. - Pass the flag to merger (to avoid adding the same materials again).
Lower LODs may use fewer textures due to features being removed, even if they use the same images/textures for all that remains. Because of this the material indices need to be adjusted for the lower LODs to be correct.
robertos
reviewed
Feb 22, 2018
| @@ -62,7 +65,8 @@ void CommandLine::PrintHelp() | |||
| void CommandLine::ParseCommandLineArguments( | |||
| int argc, wchar_t *argv[], | |||
| std::wstring& inputFilePath, AssetType& inputAssetType, std::wstring& outFilePath, std::wstring& tempDirectory, | |||
| std::vector<std::wstring>& lodFilePaths, std::vector<double>& screenCoveragePercentages, size_t& maxTextureSize) | |||
| std::vector<std::wstring>& lodFilePaths, std::vector<double>& screenCoveragePercentages, size_t& maxTextureSize, | |||
| bool& share_materials) | |||
There was a problem hiding this comment.
We've been using camelCase for this, change to shareMaterials
robertos
reviewed
Feb 22, 2018
| @@ -76,6 +80,7 @@ void CommandLine::ParseCommandLineArguments( | |||
| lodFilePaths.clear(); | |||
| screenCoveragePercentages.clear(); | |||
| maxTextureSize = MAXTEXTURESIZE_DEFAULT; | |||
| share_materials = false; | |||
robertos
reviewed
Feb 22, 2018
| @@ -73,7 +73,8 @@ GLTFDocument LoadAndConvertDocumentForWindowsMR( | |||
| std::wstring& inputFilePath, | |||
| AssetType inputAssetType, | |||
| const std::wstring& tempDirectory, | |||
| size_t maxTextureSize) | |||
| size_t maxTextureSize, | |||
| bool packTextures) | |||
There was a problem hiding this comment.
default to true? Also, call it packAndCompressTextures or processTextures?
robertos
reviewed
Feb 22, 2018
glTF-Toolkit/src/GLTFLODUtils.cpp
Outdated
| @@ -110,7 +110,7 @@ namespace | |||
| return stringBuffer.GetString(); | |||
| } | |||
|
|
|||
| GLTFDocument AddGLTFNodeLOD(const GLTFDocument& primary, LODMap& primaryLods, const GLTFDocument& lod, const std::wstring& relativePath = L"") | |||
| GLTFDocument AddGLTFNodeLOD(const GLTFDocument& primary, LODMap& primaryLods, const GLTFDocument& lod, const std::wstring& relativePath = L"", bool shared_materials = false) | |||
There was a problem hiding this comment.
camelCase: sharedMaterials
robertos
suggested changes
Feb 22, 2018
There was a problem hiding this comment.
Looks good; I think the LOD code is starting to get ripe for some refactoring (e.g. break up the bigger methods into smaller, then only call the smaller ones if not sharing materials). I might be able to try some refactoring later, but if you want to get started this could be a good opportunity.
robertos
suggested changes
Feb 26, 2018
| @@ -24,7 +25,8 @@ enum class CommandLineParsingState | |||
| ReadTmpDir, | |||
| ReadLods, | |||
| ReadScreenCoverage, | |||
| ReadMaxTextureSize | |||
| ReadMaxTextureSize, | |||
| ShareMaterials | |||
There was a problem hiding this comment.
You don't need this state. Reading the flag does not change the state of the parser
robertos
approved these changes
Feb 27, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes part of issue #9 by adding a commandline flag that bypasses packing of texture resources for the lower LOD levels, and that makes the lower LODs use the materials from LOD0.
The assumption is that LOD0 has all the materials used for all the LODs, and the lower quality LODs may use a subset of the materials from LOD0. It also assumes that the same texture packing scheme is used for all LODs.