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

Fixes #1671 and Fixes #1139. #3207

Merged
merged 1 commit into from
Nov 25, 2023
Merged

Fixes #1671 and Fixes #1139. #3207

merged 1 commit into from
Nov 25, 2023

Conversation

mcourteaux
Copy link
Contributor

@mcourteaux mcourteaux commented Nov 25, 2023

Flush the nanovg draw commands whenever the next draw would overflow the uint16_t index type.
Additionally added a bunch of BX_ASSERT (which don't get compiled into release builds) to prevent more bugs along these lines.

I think that, at least given the way things work, this should be a nanovg-bgfx-specific patch, and not a nanovg patch. Nanovg takes a different approach, without index buffers:
https://github.com/memononen/nanovg/blob/f93799c078fa11ed61c078c65a53914c8782c00b/src/nanovg_gl.h#L1029-L1030:

	for (i = 0; i < npaths; i++)
		glDrawArrays(GL_TRIANGLE_FAN, paths[i].fillOffset, paths[i].fillCount);

Therefore, nanovg never suffered this issue, as there were no indices to overflow.


Demo with a lot of content (example code not modified in this PR):

image

Whoop whoop! It took 6 years since opening #1139 for someone to actually investigate and fix this. 😂
(Fixes #1671, which is a duplicate)

…commands whenever the next draw would overflow the uint16_t index type.
@mcourteaux
Copy link
Contributor Author

Just confirmed this in SilverNode, that all the glitches are gone.

@bkaradzic
Copy link
Owner

Whoop whoop! It took 6 years since opening #1139 for someone to actually investigate and fix this. 😂
(Fixes #1671, which is a duplicate)

You need to understand that NanoVG in scope of bgfx is just to demonstrate porting from GL to bgfx, and whatever 20-nanovg needs. It never meant to be complete, robust, performant, etc.

@bkaradzic bkaradzic merged commit 6dea6a2 into bkaradzic:master Nov 25, 2023
13 checks passed
@mcourteaux
Copy link
Contributor Author

Yeah yeah, I fully understand. I was just surprised it took so long, as NanoVG seems so useful. For me, in SilverNode, it's part of the technology stack that makes it fully crossplatform: C++ < SDL2 < bgfx < nanovg < my own widget library. I thought many more people would use bgfx+nanovg to build cross platform user interfaces. That's why it seemed surprising.

However, I just found jdryg/vg-renderer, thanks to some comment you made somewhere. Maybe people use that more. If I understood correctly, you'd also recommend vg-renderer over nanovg, for robustness and speed?

@bkaradzic
Copy link
Owner

I was just surprised it took so long, as NanoVG seems so useful.

20-nanovg never broke during those years, so from my POV wasn't an issue.

you'd also recommend vg-renderer over nanovg, for robustness and speed?

I haven't used it, but it exist to solve issues with nanovg performance. And I think it's more robust since it has larger software shipped with it.

@bkaradzic
Copy link
Owner

For me, in SilverNode, it's part of the technology stack that makes it fully crossplatform

This?
https://silvernode.io/

Can you add something about it in "Who is using bgfx" section of README.md?

@mcourteaux
Copy link
Contributor Author

mcourteaux commented Nov 26, 2023

Yes, that's it! 😄 You asked me more than a year ago for the same, but I wanted to wait, before drawing attention, as I didn't have time to work on it, and I would just disappoint excited people, because there was nothing going on. Luckily, I'll quit my job at new years, and work on SilverNode full time soon! 🥳 Thanks for asking! I'll make work of it one of these weeks. When I do, is there a place in the README you'd want this to be? Bottom, or somewhere higher (as it's an application, rather than a game)?

@bkaradzic
Copy link
Owner

Bottom, or somewhere higher (as it's an application, rather than a game)?

Bottom. It's chronological based on time when it's added.

jay3d pushed a commit to jay3d/bgfx that referenced this pull request Dec 7, 2023
…commands whenever the next draw would overflow the uint16_t index type. (bkaradzic#3207)
mipek pushed a commit to mipek/bgfx that referenced this pull request Mar 2, 2024
…commands whenever the next draw would overflow the uint16_t index type. (bkaradzic#3207)
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.

Bad rendering with many filled path
2 participants