-
Notifications
You must be signed in to change notification settings - Fork 207
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
Add triangle fill/draw methods to images #5815
Comments
@AqeeAqee has expressed interest in implementing this, so i've pushed some starter code in pxt-common-packages that defines the shim: https://github.com/microsoft/pxt-common-packages/tree/triangle-fill In this branch I've added an There are two places this has to be implemented, I've added comments in both places:
To set up a developer environment:
This will launch pxt-arcade on localhost in your browser. When you make changes in pxt-common-packages, |
One thing to note: C++ changes take a long time to compile (it goes to our cloud compiler). I usually find that it's easiest to implement the function for the browser first and then port the code over to C++ to minimize the headache! |
ah whoops, there's a bug in the code i pushed. one moment! |
alright fixed! Also, @jwunderl has helpfully reminded me that @AqeeAqee can't actually test the sim implementation because it's a private repo. For that reason, don't worry about the sim! if you are still interested, implement the C++ and @jwunderl or I will port your code to the simulator. Lastly, you can greatly speed up the C++ build by deleting the following entries in pxt-arcade/pxtarget.json:
These define hardware targets for boards other than the meowbit! if you're just testing with the meowbit, then you can safely remove these and skip those compile steps |
Got it! |
Hi Richard I followed the steps you provided. After excute "pxt serve", the web page showing this forever output logs: https://github.com/AqeeAqee/temp/blob/main/npm_install.log |
@AqeeAqee can you run |
v10.15.1 |
I checked latest version is 18.15.0, v10 is too old LOL, installing... |
Yeah, unfortunately v10.15.1 is exactly one version too old to run as I believe textencoder was added by default in v11 (/v12 lts) -- I'm running v16lts fwiw; we should probably update pxt-arcade's package.json engines field like we do in pxt https://github.com/microsoft/pxt/blob/master/package.json#L60, I'm actually a little surprised it didn't get mad because of pxt's engine requirement but maybe it only works for top level package.jsons and not deps |
I am sure all methods added by richard have been pulled in source files. |
@AqeeAqee possible your repos became unlinked! Sometimes running npm install after the initial linking will do that. Try running |
Hey @AqeeAqee awesome you're taking a stab at this! I was thinking this approach might be a good one, given Arcade's low-end hardware spec: http://www.sunshine2k.de/coding/java/TriangleRasterization/TriangleRasterization.html#algo1 (Standard Algorithm section). Just a suggestion. Looking forward to seeing what you come up with. |
@riknoll BTW, there's other errors in |
@eanders-ms |
@AqeeAqee yup, those errors can be ignored! One day, we'll remove them. One day. |
Hi all, Mode1: Cache
Mode2: Yield
Fill Triangle Test Result: Cache: ScreenRange= 0.417ms , OverRange=0.543ms Polygon4:
Fill Polygon4 Test Result: |
[Note for others] |
Sorry, check in failed, I will try later. |
Seems I have no right to commit to pxt-common-packages. Need I fork it to my repo? |
Yes you will need to fork. For local testing, I'm hopeful we'll be able to sync your fork and |
Forked and checked in at |
awesome stuff @AqeeAqee! Playing around on my pygamer, it's even faster (as expected). As for the memory tradeoff, i say we go for checking in the non-cache strategy first and if speed becomes an issue try out the cache optimization. 640 bytes definitely isn't the worst thing in the world but it's not trivial either you should open this as a PR on pxt-common-packages! it's easier to give specific feedback that way. you can mark it as a draft if you're still working on it. at a glance, the code looks great! |
Great to hear you are satisfied the performence. Totally agree your choose, I think so, memory is criticle concern for Arcade devices. |
yeah, go ahead and include both the triangle and polygon4 in the PR! |
Created PR: microsoft/pxt-common-packages#1439, to https://github.com/microsoft/pxt-common-packages/tree/triangle-fill, is it right? |
@AqeeAqee yup, thanks! We'll review and continue this conversation over in that PR! |
Thanks for your reference again. The main structure of trigangle fill implement I checked in is just similar to the 2nd algorithm in this article.
|
Hi @riknoll and @jwunderl |
new PR microsoft/pxt-common-packages#1441 |
We should really have an efficient implementation of this in the sim/c++. There's been a lot of interest in the forum but performance is a bottleneck right now when implemented in user code.
The text was updated successfully, but these errors were encountered: