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

Update tiny_gltf.h to upstream #45

Closed
wants to merge 3 commits into
base: master
from

Conversation

3 participants
@Squareys
Contributor

Squareys commented Jul 23, 2018

Hi @mosra !

As discussed per gitter.

(Summary for others: the patches I applied downstream back then seem to be obsolete.)

Cheers, Jonathan.

@mosra

This comment has been minimized.

Owner

mosra commented Jul 23, 2018

One important thing: the commit message should contain upstream commit ID (or a link to it, ideally) ;)

@mosra

This comment has been minimized.

Owner

mosra commented Jul 23, 2018

Looking at https://codecov.io/gh/mosra/magnum-plugins/src/b16dac21cbc2cd9c7b3cb3a94742dfc7b3e83bb1/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp#L655, the URI image import doesn't seem to be tested at all, so can't be sure if the update breaks this code path or not.

EDIT: dang sorry wrong button, didn't want to close. For the record, the CIs were green before I closed by accident.

@mosra mosra closed this Jul 23, 2018

@mosra mosra reopened this Jul 23, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jul 23, 2018

Codecov Report

Merging #45 into master will decrease coverage by 0.16%.
The diff coverage is 35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
- Coverage   67.18%   67.01%   -0.17%     
==========================================
  Files          43       43              
  Lines        7341     7434      +93     
==========================================
+ Hits         4932     4982      +50     
- Misses       2409     2452      +43
Impacted Files Coverage Δ
src/MagnumExternal/TinyGltf/tiny_gltf.h 45.17% <35%> (-0.01%) ⬇️
...agnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp 87.5% <0%> (+1.94%) ⬆️
...agnumPlugins/StbImageImporter/StbImageImporter.cpp 79.54% <0%> (+2.27%) ⬆️

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 cbcb706...667a4ae. Read the comment docs.

@mosra mosra added this to TODO in Asset management via automation Jul 23, 2018

@mosra mosra added this to the 2018.0c milestone Jul 23, 2018

Squareys added some commits Jul 23, 2018

MagnumExternal: Update tiny_gltf.h
Now at syoyo/tinygltf@39abfb5

Signed-off-by: Squareys <squareys@googlemail.com>
TinyGltfImporter: Add test for images embedded as buffer data
Signed-off-by: Squareys <squareys@googlemail.com>
TinyGltfImporter: Remove unrequired asset.generator value from test f…
…iles

Signed-off-by: Squareys <squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:update-tinygltf branch from b0f8912 to 667a4ae Jul 23, 2018

@mosra

This comment has been minimized.

Owner

mosra commented Jul 26, 2018

Thank you! Merged into master now :) For the record, with those modifications:

  • tinygltf update as 29661ad, but had to merge a few more things upstream, so the commit is different
  • embedded images test as 61afb91, but the code was crashing on images with embedded data URIs, fixed in 34f3adb
  • test file cleanup in 5ee0a5f, but the original commit forgot to update the corresponding glb files and missed a few cases, i did an overall cleanup in the following commits

@mosra mosra closed this Jul 26, 2018

Asset management automation moved this from TODO to Done Jul 26, 2018

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