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

Material texture layer support for TinyGltfImporter and AssimpImporter #83

Merged
merged 2 commits into from
May 11, 2020

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented May 2, 2020

Hi @mosra !

Here's the accompanying PR for mosra/magnum#438.

TODOs:

  • Compile check
  • Add configuration value for textures sets
  • Testing (A gltf+glb file with a different layer for each, reusing that for assimp, too)
  • Advertise support

@mosra
Copy link
Owner

mosra commented May 2, 2020

gltf+glb

Just FYI I don't think you need both, just gltf alone is simple (and you can cut that part out of the material-invalid.gltf where it checks all cases).

For Assimp I'm afraid a glTF won't work (at least not on the ass-old versions on the CI), but copying and modifying one of COLLADA files could be easy enough I hope.

@Squareys Squareys force-pushed the material-texture-layer branch from 45f3e2b to b0e4b15 Compare May 3, 2020 12:54
@mosra mosra marked this pull request as draft May 3, 2020 16:05
@mosra mosra changed the title [WIP] Material texture layer support for TinyGltfImporter and AssimpImporter Material texture layer support for TinyGltfImporter and AssimpImporter May 3, 2020
src/MagnumPlugins/AssimpImporter/AssimpImporter.conf Outdated Show resolved Hide resolved
src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.conf Outdated Show resolved Hide resolved
CORRADE_COMPARE(mat.diffuseCoordinateSet(), 2);
CORRADE_COMPARE(mat.ambientCoordinateSet(), 3);
CORRADE_COMPARE(mat.normalCoordinateSet(), 2);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same as in case of the TinyGltf test -- ideally two variants, testing that the default behavior correctly fails. This also means you need to have three four different materials in order to verify each of the diffuse/specular/ambient/normal failures is done correctly. Instanced tests might (or might not) be helpful.

(What about a specular texture here?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually solved this before reading your comment... the second ambient should have been specular (copy pasta), and the failure is now also tested, but not separate. Maybe check really quick if the new version satisfies your requirements :)

@Squareys Squareys force-pushed the material-texture-layer branch 3 times, most recently from a31f68c to 513abec Compare May 7, 2020 13:01
@Squareys Squareys marked this pull request as ready for review May 7, 2020 13:02
@Squareys Squareys force-pushed the material-texture-layer branch from 513abec to 70731c8 Compare May 8, 2020 14:00
@codecov-io
Copy link

codecov-io commented May 8, 2020

Codecov Report

Merging #83 into master will decrease coverage by 0.24%.
The diff coverage is 68.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
- Coverage   93.44%   93.20%   -0.25%     
==========================================
  Files          81       81              
  Lines        5496     5532      +36     
==========================================
+ Hits         5136     5156      +20     
- Misses        360      376      +16     
Impacted Files Coverage Δ
.../MagnumPlugins/TinyGltfImporter/TinyGltfImporter.h 100.00% <ø> (ø)
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp 87.00% <57.89%> (-2.56%) ⬇️
...agnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp 97.44% <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 8e66409...4afa57c. Read the comment docs.

@Squareys
Copy link
Contributor Author

@mosra FYI, this is ready and I'm currently waiting for feedback on this :)

Squareys added 2 commits May 11, 2020 14:47
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys Squareys force-pushed the material-texture-layer branch from 70731c8 to 4afa57c Compare May 11, 2020 12:47
@mosra mosra merged commit 4afa57c into mosra:master May 11, 2020
@mosra
Copy link
Owner

mosra commented May 11, 2020

Thx!

@mosra mosra added this to the 2020.0a milestone May 11, 2020
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