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

Please update the version of stb_truetype.h and stb_rect_pack.h in nuklear.h #51

Closed
ronaaron opened this issue Jan 19, 2020 · 16 comments
Closed
Labels
help wanted Extra attention is needed

Comments

@ronaaron
Copy link
Contributor

The versions are very old, and in particular a number of bugs in TTF handling have been fixed in more recent versions.

@dumblob
Copy link
Member

dumblob commented Jan 19, 2020

Thanks for letting us know. There was a similar request and I think the goal was to automatically update them in each run of paq.sh/paq.bat (i.e. during the "amalgamation" process). PRs are welcome and because it sounds like an easy to review change, they'll be merged very quickly (couple of days probably).

@ronaaron
Copy link
Contributor Author

I was going to push to a new branch, but I don't seem to be able to do that. So I'm attaching the modified 'src/nuklear_font.h'.

NOTE: There are two functions which in the current version take an nk_allocator, but which have no such thing in the code I merge; I do not understand all the ramifications of not having the allocator, it seems to work as-is on my Linux box.

The functions in question are part of the baker:

  • nk_tt_PackFontRangesRenderIntoRects
  • nk_tt_PackEnd

nuklear_font.zip

@ronaaron
Copy link
Contributor Author

The code does crash on macOS, so apparently the allocator is needed. I was hoping it might be possible to allocate whatever's needed beforehand rather than piecemeal. But I don't know the code, and am relying on you guys.

@ronaaron
Copy link
Contributor Author

Still relying on you guys... any feedback on the need for the allocator?

@dumblob
Copy link
Member

dumblob commented Jan 22, 2020

I'm still out of time to do reviews. But what caught my eye is that you can't submit a PR - that's weird. Could you please try again (step by step as GitHub doc says)? It would encourage others to look over your changes 😉.

@ronaaron
Copy link
Contributor Author

Pinging again, since I put a PR in just after your last comment. Looking for assistance on that issue, since the ancient TTF support is hindering me.

@ronaaron ronaaron reopened this Jan 31, 2020
@dumblob
Copy link
Member

dumblob commented Jan 31, 2020

@Nielsbishere feel free to review the PR - I'd like to give you commit permission afterwards 😉

@ghost
Copy link

ghost commented Feb 5, 2020

@ronaaron Someone shares my pain <3
I have been bothered by the old stb_library versions myself. Many thanks for for the PR and the contribution!

@ronaaron
Copy link
Contributor Author

ronaaron commented Feb 6, 2020

Yeah; I hope it gets integrated (with appropriate changes) soon.

@dumblob
Copy link
Member

dumblob commented Feb 6, 2020

stb_truetype update request appeared several times also in the old Nuklear repo. I think we should move away from making changes to stb_truetype once and forever. Could you @ronaaron change your PR in a way, that there is separate stb_truetype.h taken 1:1 from the stb repo and then in src_nuklear_font.c there'll be just the Nuklear-specific stuff? It'll be also easier to review 😉 (and I think I'll review it despite my lack of time because of the high demand).

@SairesArt would you want to review the PR (or other PRs)? This would help the project a lot (I'd then give you commit permission 😉).

@ghost
Copy link

ghost commented Feb 6, 2020

that there is separate stb_truetype.h taken 1:1 from the stb repo

That sounds like a good quality of life quality of code change for future updates, which ideally should have been done from the get go. So @ronaaron is unknowingly kicking off some important code refactoring :P

@SairesArt would you want to review the PR (or other PRs)? This would help the project a lot (I'd then give you commit permission wink).

I was very pleasantly surprised to find nuklear changing repos and being revived again, after some slumber during the past two years. Contributing to nuklear has been on my todo list for quite some time (GLFW OpenGL ES2 demo and ideally an option for SDF text rendering), but never came around to it... so I'll gladly take you up on that offer. Whilst I'm at it, I'll make a fresh new github account (another todo thing), message you this evening for the permissions and review me some PR's this weekend :]

@ronaaron
Copy link
Contributor Author

ronaaron commented Feb 6, 2020

So I just uploaded a modified version of my ttf branch, which separates both stb files. I had to also play a bit with the packer so that I could get the STB*IMPLEMENTATION if NK_IMPLEMENTATION is defined.

I left two "TODO" comments in the code, where I am not sure what to do.

@FrostKiwi
Copy link
Contributor

@SairesArt Here with my new Account to tackle all my Open Source responsibilities :]
@dumblob Although a bit late, I'd love to get commit rights and go through the backlog of PRs now.

@ghost
Copy link

ghost commented Feb 17, 2020

@SairesArt Here with my new Account

@FrostKiwi Quick proof of ownership via quote reply
Welp, so much for "this weekend". Life decided to head a couple more stuff my way, but now I can take on some PRs^^

@dumblob
Copy link
Member

dumblob commented Feb 17, 2020

@FrostKiwi greetings and thanks for your interest. Could you please find the culprit for the last unsolved issue described in #53 (comment) ? Then I'll merge the PR and you'll get the commit rights 😉.

@dumblob dumblob added the help wanted Extra attention is needed label Feb 24, 2020
@dumblob
Copy link
Member

dumblob commented Mar 10, 2020

I think this can be closed as the PR #53 got merged. Feel free to reopen if you feel so.

@dumblob dumblob closed this as completed Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants