Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TinyGltfImporter: Update to current HEAD and KHR_lights_punctual #77

Merged
merged 6 commits into from
Dec 22, 2019

Conversation

Squareys
Copy link
Contributor

Hi @mosra !

This was surprisingly easy, I wonder what I missed... I had to update tiny_gltf to get support for KHR_lights_punctual, in the process I adapted the importer to API changes there. Since the new light extension is pretty similar to the old, this was pretty straight forward.

I made a minor adaption to the test, from the documentation Linear was the default all along and therefore I wonder why the test was checking for Nearest as the minFilter was not set in the file.

Cheers,
Jonathan.

@codecov-io
Copy link

codecov-io commented Dec 20, 2019

Codecov Report

Merging #77 into master will increase coverage by 0.62%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
+ Coverage    91.5%   92.12%   +0.62%     
==========================================
  Files          76       76              
  Lines        4600     4469     -131     
==========================================
- Hits         4209     4117      -92     
+ Misses        391      352      -39
Impacted Files Coverage Δ
.../MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h 100% <ø> (ø) ⬆️
...agnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp 94.46% <100%> (+1.56%) ⬆️
...agnumPlugins/StbImageImporter/StbImageImporter.cpp 88.88% <0%> (-0.25%) ⬇️
...ns/MiniExrImageConverter/MiniExrImageConverter.cpp 95.83% <0%> (-0.17%) ⬇️
src/MagnumPlugins/DdsImporter/DdsImporter.cpp 77.89% <0%> (-0.1%) ⬇️
src/Magnum/OpenDdl/Structure.h 96.49% <0%> (-0.07%) ⬇️
...agnumPlugins/StanfordImporter/StanfordImporter.cpp 99.57% <0%> (-0.01%) ⬇️
src/Magnum/OpenDdl/Implementation/Parsers.cpp 87.7% <0%> (+0.24%) ⬆️
src/MagnumPlugins/BasisImporter/BasisImporter.cpp 90.38% <0%> (+0.3%) ⬆️
.../MagnumPlugins/OpenGexImporter/OpenGexImporter.cpp 96.06% <0%> (+0.34%) ⬆️
... and 7 more

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 4018164...b57cdf4. Read the comment docs.

@mosra mosra added this to the 2019.1c milestone Dec 20, 2019
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was unexpectedly quick, thank you!

src/MagnumExternal/TinyGltf/tiny_gltf.h Show resolved Hide resolved
src/MagnumExternal/TinyGltf/tiny_gltf.h Show resolved Hide resolved
src/MagnumExternal/TinyGltf/json.hpp Outdated Show resolved Hide resolved
src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp Outdated Show resolved Hide resolved
@Squareys
Copy link
Contributor Author

Alright, updated. The defaults test was there already, I am really unsure how this passed before (looking at the diff, I didn't touch the tested file).

I was able to omit the json.hpp update (which was only needed for clang with c++17).

@mosra
Copy link
Owner

mosra commented Dec 21, 2019

Ah! I see now, there's two cases -- one is with sampler missing completely (which is tested properly), second is with sampler parameters missing (right now only wrapping seems to be tested, not min/mag filters at all).

Could you expand the texture.gltf to list all properties, and then add a texture-empty-sampler.gltf that has just an empty sampler, verifying the defaults are picked correctly? No need to add the glb/packed variants in this case I think. Thanks!

@Squareys
Copy link
Contributor Author

Crazy that you figured that out so quickly. I made the requested changes.

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@mosra mosra merged commit b57cdf4 into mosra:master Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants