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

Fix #42: Serious gltf bugs #43

Merged
merged 13 commits into from Jun 2, 2018
Merged

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented May 22, 2018

Hi @mosra !

Here's a WIP PR for #42!

Cheers, Jonathan.

TODOs

  • bufferView.stride (now failing on non-trivial as suggested)
  • sensible material defaults (and interpretation of pbr values)
  • fix flipped textures
  • solve python scripts/buffer generation
  • test load more of the khronos provided example models

@codecov-io
Copy link

codecov-io commented May 22, 2018

Codecov Report

Merging #43 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   87.26%   87.15%   -0.11%     
==========================================
  Files          41       39       -2     
  Lines        3903     3193     -710     
==========================================
- Hits         3406     2783     -623     
+ Misses        497      410      -87
Impacted Files Coverage Δ
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp 69.79% <0%> (-13.55%) ⬇️
...numPlugins/StbImageConverter/StbImageConverter.cpp 73.84% <0%> (-0.44%) ⬇️
src/Magnum/OpenDdl/Document.h 88.57% <0%> (-0.32%) ⬇️
src/Magnum/OpenDdl/Structure.h 96.55% <0%> (-0.06%) ⬇️
src/MagnumPlugins/OpenGexImporter/openGexSpec.hpp 100% <0%> (ø) ⬆️
src/MagnumPlugins/PngImporter/PngImporter.cpp 69.87% <0%> (+0.11%) ⬆️
...numPlugins/PngImageConverter/PngImageConverter.cpp 69.11% <0%> (+0.54%) ⬆️
...agnumPlugins/StanfordImporter/StanfordImporter.cpp 94.3% <0%> (+0.55%) ⬆️
src/Magnum/OpenDdl/Implementation/Parsers.cpp 88.03% <0%> (+0.56%) ⬆️
.../MagnumPlugins/OpenGexImporter/OpenGexImporter.cpp 94.45% <0%> (+0.79%) ⬆️
... and 9 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 99174e8...b44bd06. Read the comment docs.

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.

Initial review. Did not pull locally yet, as I assume you'd still be testing with the sample models (and maybe finding some more issues there).

Thanks a lot for all the work!

unsigned int: 2 1 0 3
# Triangle Fan
float: 5.1 5.2 5.3 6.1 6.2 6.3 7.1 7.2 7.3 8.1 8.2 8.3
unsigned int: 2 1 3 0
Copy link
Owner

Choose a reason for hiding this comment

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

Why not make use of the Python struct package as I suggested on Gitter? You wouldn't need to do any parsing or anything, just feed structure definition together with a single array to Python. Original suggestion, for the record:

/*
type = "<3Hxx9f"
input = [0, 1, 2, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0]
*/
"uri" : "data:application/octet-> stream;base64,AAABAAIAAAAAAAAAAAAAAAAAAAAAAIA/AAAAAAAAAAAAAAAAAACAPwAAAAA=",
"byteLength" : 44

to explain, first is type specification according to https://docs.python.org/3.6/library/struct.html, second is a list of values encoded based on that spec:

  • < to denote little endian (could be even omitted, I think, and just part of the script)
  • then three unsigned short values, encoding indices
  • after that two padding bytes
  • after that 9 floats (three three-component vectors), encoding positions
    6 bytes + 2 padding + 36 bytes = 44 bytes total

The whole script would be then just:

import struct
exec(the_type_and_input_lines_above) # you get type and input from it
with open(file, 'wb') as f: f.write(struct.pack(type, input))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Back when I read that message I thought you meant having that comment like in the code snippet which would then be parsed and then insert its result into the appropriate place in the gltf file :D That seemed a little too complicated.
Didn't realize you meant having a script that would just create the bin in a hardcoded way... kinda makes sense, though.

Copy link
Owner

Choose a reason for hiding this comment

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

I thought you meant

Wellll, yes, sorry, I did. My initial ideas are always over-the-board crazy 😆 Having that in a separate .bin.in file is fine as well now, no need to go that crazy ;)

@@ -222,43 +224,50 @@ std::unique_ptr<ObjectData3D> TinyGltfImporter::doObject3D(UnsignedInt id) {
CORRADE_INTERNAL_ASSERT(node.rotation.size() == 0 || node.rotation.size() == 4);
CORRADE_INTERNAL_ASSERT(node.translation.size() == 0 || node.translation.size() == 3);
CORRADE_INTERNAL_ASSERT(node.scale.size() == 0 || node.scale.size() == 3);
/* Ensure we have either a matrix or L-R-S */
Copy link
Owner

Choose a reason for hiding this comment

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

What does L stand for? (The glTF tutorial names it as TRS.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Location. (Like LocRotScale in blender)

Will change it to match the spec

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh, Loc in Blender, of course. Sorry for dumb questions 🙊

@@ -4,7 +4,7 @@
//
// The MIT License (MIT)
//
// Copyright (c) 2015 - 2017 Syoyo Fujita, Aurélien Chatelain and many
// Copyright (c) 2015 - 2018 Syoyo Fujita, Aurélien Chatelain and many
Copy link
Owner

Choose a reason for hiding this comment

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

Can you amend the commit message that updates this file and say which upstream SHA-1 is this? Helps a lot when updating later.

Thanks.

@@ -288,6 +297,9 @@ Containers::Optional<MeshData3D> TinyGltfImporter::doMesh3D(const UnsignedInt id
meshPrimitive = MeshPrimitive::Lines;
} else if(primitive.mode == TINYGLTF_MODE_LINE_LOOP) {
meshPrimitive = MeshPrimitive::LineLoop;
} else if(primitive.mode == 3) {
/* For some reason tiny_gltf doesn't have a define for this */
Copy link
Owner

Choose a reason for hiding this comment

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

🙈 😆

@@ -317,19 +329,19 @@ Containers::Optional<MeshData3D> TinyGltfImporter::doMesh3D(const UnsignedInt id
return Containers::NullOpt;
}

const size_t numPositions = bufferView.byteLength/sizeof(Vector3);
const size_t numPositions = accessor.count;
Copy link
Owner

Choose a reason for hiding this comment

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

I think this variable is not really needed anymore. (Yes, sorry, this originates from my patch, which was just hastily put together to make things working).

if(dt != material.values.end()) {
diffuseTexture = dt->second.TextureIndex();
flags |= PhongMaterialData::Flag::DiffuseTexture;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Similarly to baseColorTexture, there's the baseColorFactor. As I mentioned in #42, the glTF sample model repository uses it on even the most basic models. Assimp imports it as diffuse, so we could do the same here (unless diffuseFactor is set).

} else if(idxAccessor.componentType == TINYGLTF_COMPONENT_TYPE_UNSIGNED_INT) {
std::copy_n(reinterpret_cast<const UnsignedInt*>(start), idxBufferView.byteLength/sizeof(UnsignedInt), std::back_inserter(indices));
std::copy_n(reinterpret_cast<const UnsignedInt*>(start), idxAccessor.count, std::back_inserter(indices));
} else CORRADE_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */
Copy link
Owner

Choose a reason for hiding this comment

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

Unless I missed something, this is still not correctly handling interleaved data. Failing with a clear error in that case would be the best, but can you mention that in the limitations in the docs, at least? I have no idea if tinygltf supports that, even.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, there is accessor.stride...

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, great. I am planning on supporting interleaved attributes directly in MeshData later, so at that point I can implement support for them here as well.

Until then, could you fail in case of a non-trivial stride?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pardon me, it's bufferView.stride (just in case you need it sometime)

/* The specification instructs to use "auto sampling", i.e. it is left to the implementor to decide on the default values... */
return TextureData{TextureData::Type::Texture2D, SamplerFilter::Linear, SamplerFilter::Linear,
Sampler::Mipmap::Linear, {Sampler::Wrapping::Repeat, Sampler::Wrapping::Repeat, Sampler::Wrapping::Repeat}, UnsignedInt(tex.source), &tex};
}
Copy link
Owner

Choose a reason for hiding this comment

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

Would be great if you could mention the defaults in case no sampler definition is present in the docs.

@@ -414,53 +426,62 @@ std::unique_ptr<AbstractMaterialData> TinyGltfImporter::doMaterial(const Unsigne
/* Textures */
PhongMaterialData::Flags flags;
UnsignedInt diffuseTexture{}, specularTexture{};
Vector4 diffuseColor{1.0f};
Vector3 specularColor{0.0f};
Float shininess{1.0f};
Copy link
Owner

Choose a reason for hiding this comment

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

Mentioning these defaults in the docs would be great :)

@mosra
Copy link
Owner

mosra commented May 26, 2018

Oh, one more thing: are you planning to handle the texture coordinate flipping here as well? I am not exactly sure what's happening, though.

@Squareys Squareys force-pushed the serious-gltf-issues branch 2 times, most recently from 5b360ea to 5b21a82 Compare June 1, 2018 17:49
@mosra mosra mentioned this pull request Jun 1, 2018
5 tasks
@@ -320,6 +320,9 @@ Containers::Optional<MeshData3D> TinyGltfImporter::doMesh3D(const UnsignedInt id
const tinygltf::BufferView& bufferView = _d->model.bufferViews[accessor.bufferView];
const tinygltf::Buffer& buffer = _d->model.buffers[bufferView.buffer];

const int stride = bufferView.byteStride;
CORRADE_ASSERT(stride == 0, "Trade::TinyGltfImporter::mesh3D(): non-zero strides for buffer views are unsupported.", {});
Copy link
Owner

Choose a reason for hiding this comment

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

... can this be a non-fatal failure? Like, returning NullOpt instead of blowing up :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, already did that.

@Squareys
Copy link
Contributor Author

Squareys commented Jun 1, 2018

@mosra Alright, did everything (I tripple checked this time, but you may want to do so, too 😜 ). Ready for review 👍

@Squareys Squareys changed the title [WIP] Fix #42: Serious gltf bugs Fix #42: Serious gltf bugs Jun 1, 2018
Signed-off-by: Squareys <squareys@googlemail.com>
Matches 7c56f8eb9eba408fc7db8ec15ad621c25b414b7b in
https://github.com/syoyo/tinygltf.

Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@mosra mosra merged commit b44bd06 into mosra:master Jun 2, 2018
@mosra
Copy link
Owner

mosra commented Jun 2, 2018

Finally merged, thank you a lot :)

@mosra mosra added this to the 2018.0c milestone Jun 2, 2018
@mosra mosra added this to TODO in Asset management via automation Jun 2, 2018
@mosra mosra moved this from TODO to Done in Asset management Jun 2, 2018
@steeve
Copy link

steeve commented Jun 2, 2018

congrats guys ! and thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants