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

Updating tinygltfloader #16

Open
AurL opened this issue May 18, 2017 · 13 comments
Open

Updating tinygltfloader #16

AurL opened this issue May 18, 2017 · 13 comments

Comments

@AurL
Copy link

AurL commented May 18, 2017

Hi,

Just a few words here to let you know that we are working on this project and have add a few updates (skin support, animation, PBR materials, and writing function (maybe out of the scope)).

We are currently working on updating it to handle glTF 2.0 files including the breaking changes that are being pushed:

  • schema update (arrays instead of dicts)
  • PBR material definition + extension
  • skinning updates
    etc.

We will probably push a PR here if you are interested

@syoyo
Copy link
Owner

syoyo commented May 18, 2017

Super cool!

Yeah, supporting glTF 2.0, PBR material and skin are TODO list, but I couldn't spare enough time for it these days.
I appreciate your PR!

@AurL
Copy link
Author

AurL commented May 23, 2017

Hi,

It's almost done.
I am having some issues with a few details in the code.
These static members sample 1 and sample 2 are making our code leave with a Inconsistency detected by ld.so: dl-close.c: 762: _dl_close: Assertion 'map->l_init_called' failed!'. I'm not sure how to fix this better than removing the static keyword.
Do you have any hint ?

I'll open the PR soon, then don't hesitate to take a good look as it includes a lot of changes.
It could be cool to discuss it a bit, if you have any questions.

Side note: for our use, we also added serialization functions in tinygltfloader. It still needs to be updated with the latest glTF 2.0 updates, but I wonder if it would make sense to include it in this project ?
I could also open a PR for this once it's updated.

@syoyo
Copy link
Owner

syoyo commented May 23, 2017

Thanks!

These static members sample 1 and sample 2 are making our code leave with a Inconsistency detected by ld.so: dl-close.c: 762: _dl_close: Assertion 'map->l_init_called' failed!'

This initialization idiom is for suppressing a clang warning: https://stackoverflow.com/questions/15708411/how-to-deal-with-global-constructor-warning-in-clang

But if you encounter a runtime error with this technique, we can suppress clang warning by adding clang pragma explicitly.

I wonder if it would make sense to include it in this project ?

Serialization feature is cool and would be better to include into this project(I have writer example https://github.com/syoyo/tinygltfloader/tree/master/examples/writer as you may know).
We may be better to rename TinyGLTFLoader to TinyGLTFLib or similar.

@syoyo
Copy link
Owner

syoyo commented May 24, 2017

I have created tinygltf repo

https://github.com/syoyo/tinygltf

So new version will be moved to this repo. tinygltfloader repo will remain for a while for existing glTF 1.0 programs.

@syoyo
Copy link
Owner

syoyo commented May 24, 2017

Please send PR to https://github.com/syoyo/tinygltf , @AurL

@AurL
Copy link
Author

AurL commented May 24, 2017

Ok cool, thanks a lot for your answers!

syoyo added a commit that referenced this issue May 24, 2017
Revert to use `staic Value null_value` expression since `static Value null_value = *(new Value())` results in runtime inconsistency in ld.so(#16)
@syoyo
Copy link
Owner

syoyo commented May 24, 2017

FYI, I have reverted to use static Value null_value in this recent commit: bc83dbb

We cannot remove static keyword since Get function returns the reference of a variable.

@AurL
Copy link
Author

AurL commented May 24, 2017

@syoyo thanks! Could you report it on tinygltf ? or should I cherry-pick it and include it in my PR on tinygltf ?
I tested and the issue is still not fixed (since the variable is still declared as static ?)

@AurL
Copy link
Author

AurL commented May 24, 2017

The PR is opened here, you can take a look when you have time.
Please don't merge it too quickly, I guess it will need some discussions

@syoyo
Copy link
Owner

syoyo commented May 24, 2017

Awesome! I'll review it!

I tested and the issue is still not fixed (since the variable is still declared as static ?)

Hmm... which compiler/os/glibc do you use?

@syoyo
Copy link
Owner

syoyo commented May 26, 2017

I tested and the issue is still not fixed (since the variable is still declared as static ?)

I have changed to use private static variable approach.

d1fcf24

@AurL could you please try this?

@AurL
Copy link
Author

AurL commented May 29, 2017

I tested with this commit and in a first time, I had a link issue. The static field needed to be declared after the classe.
I added a definition but then it throws the same Inconsistency detected by ld.so: dl-close.c: 762: _dl_close: Assertion map->l_init_called' failed!`.

I'm surprised to be the only one having this issue.
OS: Ubuntu 14.04
compiler: g++ (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
glibc: EGLIBC 2.19-0ubuntu6.5

@syoyo
Copy link
Owner

syoyo commented May 29, 2017

Inconsitency detected error may be due to mixing precompiled another .so's or looking different libc/ld.so on your system:

Linuxbrew/brew#73

If you still encounter the problem, please post reproducable source code and make script
(It looks you are building .so library. .so build is not tested well on my side)

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

No branches or pull requests

2 participants