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

support external jsmn implementations #121

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dangmoody
Copy link

Hi,

This is my first time making a PR/contribution to someone else's project - so please forgive me for any schoolboy errors.

I'm using CGLTF in a personal project where I am also using JSMN on its own to parse some JSON files. My project builds via Unity Builds, so I was getting an error that JSMN code was already being defined in another source file - it was the embedded JSMN implementation inside cgltf.h.

I put in a change for myself where passing a define in before including cgltf.h doesn't use the embedded JSMN implementation but then thought these changes might be useful in the library for others?

I added a quick test to make sure that it still compiles with the define turned on (CGLTF_EXTERNAL_JSMN) and with the external jsmn file included. Everything passes on Travis.

One thing I did notice was that the JSMN implementation seems to be a bit behind the latest version, which required a typecast at cgltf.h line 5072. Other than the #define I've added and the bump in version number (I don't know what your convention for version numbers is with this project but I can always revert it if you like), that's the only other change I've made.

The only thing I've not done is update the documentation. I figured it was only worth me doing that if you OK'd the change. =)

I'd be very interested to hear your feedback on this. I appreciate this is probably an edge-case problem - but hopefully this will be useful?

Thanks,

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

Successfully merging this pull request may close these issues.

1 participant