-
Notifications
You must be signed in to change notification settings - Fork 350
Draw pen lines via fragment shader #438
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
Conversation
0910c42
to
20b70ed
Compare
It looks like disabling antialiasing (#502) also had an effect on pen lines: |
20b70ed
to
2546385
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty cool - I like the nicer edges! @fsih and I had some readability & maintenance concerns while going over it, listed below.
@BryceLTaylor We're considering putting in this PR that should improve the appearance of pen lines, but we need to benchmark it first to make sure that it doesn't significantly impact pen performance, especially on devices with lower-power GPUs. Can we work together with you sometime to design benchmarking tests? |
Review feedback has been addressed! |
Also, are you still interested in |
ea41998
to
29d290f
Compare
29d290f
to
6bc1c32
Compare
0ff0092
to
7d652b4
Compare
I've done some benchmarking of my own to compare the performance of:
All numbers given are lines per second, per this benchmark. "Workstation" (AMD Ryzen 7 1700 + NVIDIA GTX 780 Ti)
Old laptop (Intel Core 2 Duo T5800 + Intel Express 965)Note that SwiftShader was enabled by default in Chrome due to GPU blacklisting.
EDIT: a previous version of this post said that Chrome VS was 5.4% slower. This was incorrect--it is 5.4% faster. In general, it appears that:
I'd like to see a benchmark on a low-energy-consumption laptop (like an "ultrabook" and/or Chromebook), which would probably have a CPU comparable to my laptop but a much faster GPU. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I do have one question about the vertex coordinates, though.
Thanks for all the benchmarks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to do a bit more performance testing but the code looks good!
Sorry -- missed this comment. We're sort of re-evaluating our process for priorities so it's hard to know what should or shouldn't be prioritized until the dust settles. That said, I think code cleanups, such as addressing #450, are welcome but won't necessarily get a high priority. As long as you're OK with that, go for it ;) |
Resolves
Kinda resolves #420?
Also resolves scratchfoundation/scratch-gui#4313 [EDIT: and now resolves #306]
Proposed Changes
This changes the PenSkin's line-drawing routine from a primarily vertex-based operation to a fragment-based one. Lines are now drawn via a screen-space pixel shader that provides proper antialiasing.
PenSkin.drawLineOnBuffer()
now draws a singleaxis-alignedquad. The quad's fragment shader, given the endpoints, color, and thickness of the line, properly computes antialiased line coverage for each pixel inside it.This has two effects:
This still doesn't completely match 2.0. In 2.0, "size 1" lines appear thicker at certain angles, and pen points render at a different offset from pen lines. The former might be worth investigating, but in my opinion the latter behavior is too confusing/annoying to be worth emulating.
On My Machine™, it runs 236783324 at the same speed, suggesting the bottleneck is in the VM now, but more comprehensive benchmarking across different GPUs would be wise.
I've done very little WebGL, so a lot of cleanup/refinement is probably necessary.EDIT: This may be a good test project. Make sure to run this multiple times-- there's a lot of weird variance. It looks like most of the time (in both the current code and this PR's) is spent in setUniforms, but fixing that would likely require UBOs, which are a WebGL 2 feature.