-
Notifications
You must be signed in to change notification settings - Fork 37
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
Adding PVector wrapper/helper class to pyodide mode #137
Conversation
to allow chaining, I guess
It would also fix #135 I believe, it would only be a question of substituting p5.Vector for PVector in the example given. |
@villares thanks for your PR and the code looks great for me! But, to merge this, I'll wait until we have something similar for transcrypt interpreter as well. IMHO, if we can't support this API for Transcrypt, I don't know if we should merge this PR. I mean, one thing is to support something only for pyodide due to transcrypt's limitations. Another thing is to expose helper APIs that works only for one of the 2 interpreters. I mean, in the long term, this can be misleading to users since |
Which parts aren't compatible? The annotations? Which 1s? The getters & setters? |
@GoToLoop right now we don't have a So, we'd have to port this class @villares is introducing to work in the current In my opinion, for transcrypt, we should at least expose the |
@villares can you link the |
This escalated quickly! The tests are here: https://gist.github.com/villares/e639a34eef756beba2f79a55203cd51e The only incompatible thing, as far as I can see, is the operator overloading that would need pragma flags or other global Transcrypt options that would impact performance (but I can be terribly wrong). |
So @berinhard, now that we could have #138 on Transcrypt mode, that mirrors most of the functionality added by this PR (except for the operator overloading) do you think we could proceed? Any other objection, or am I missing something else? |
I see a problem on this PR :( |
Thanks @villares |
I guess the docs should also be updated because I see it says |
According to suggestion by @felipesanches berinhard#137 (comment)
My attempt at solving #125
Kind of solves #20 for the pyodide mode