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

Python language bindings #2184

Merged
merged 62 commits into from
May 14, 2023
Merged

Python language bindings #2184

merged 62 commits into from
May 14, 2023

Conversation

koltenpearson
Copy link
Contributor

I have been working with the dev of pocketpy to build better bindings for c (from c++), so that python can finally be one of the languages TIC-80 can use.

They are far enough along that I have been able to write a functional (small) subset of what needs to be done. The pull request as it stands can clear the screen and print messages with trace and the demo cart is working for that.

I will push this along when I can, but if anyone wants to help at this point it should be pretty straightforward to work on by just following along with what is already written. The C bindings for pocketpy try to follow the lua stack based binding api pretty closely so they are pretty easy to work with.

Even if you aren't comfortable in c, writing the demo cart, bunny cart, and carts to test all the bindings would also be very helpful.

Note that pockectpy does not support binding functions with keyword arguments with c. To deal with that I implement the low level binding and name it the api name with an underscore in front. Then I make a wrapper in python that just calls that binding but includes the keyword arguments. The pattern should be pretty clear from what I have written in api/python.c

@koltenpearson
Copy link
Contributor Author

So pocketpy uses c++17, which is the cause of most of the build errors it is getting at this moment.

I was able to get it to work on mac by bumping the target version to 10.15 (from 10.13)

It seems like the raspberry pi builds are using gcc version 7, when c++17 support was finalized in version 8. I am currently developing on a raspberry pi and have it working on my standard raspberry pi os install, so it seems like that should be easy to resolve.

it seems like the 3ds build might be an unrelated error since it seems like it was able to compile pocketpy, but not able to link at the end, so maybe I just need to adjust some of the linker flags in cmake for that target.

I am not sure what next steps to take on the android build but the error it is that it does not have an option for c++17

Is getting the build tools to the point where they are supporting c++17 something that would be desired @nesbox? And is it something I would be able to do or do I need full admin permissions to the repo to adjust these things? (I have never really been on the other end of github actions before)

@nesbox
Copy link
Owner

nesbox commented May 6, 2023

Hi,
you don't need any special rights to adjust Github actions.
All you need is in the https://github.com/nesbox/TIC-80/blob/main/.github/workflows/build.yml file.

@blueloveTH

This comment was marked as outdated.

@koltenpearson koltenpearson marked this pull request as ready for review May 11, 2023 03:31
@koltenpearson
Copy link
Contributor Author

Okay the bindings are done. I am sure there are some bugs in there, but it will be easier to find them if people are able to try it out and submit issues.

@nesbox nesbox merged commit bbd3744 into nesbox:main May 14, 2023
@nesbox
Copy link
Owner

nesbox commented May 14, 2023

Thank you for the PR, the only thing I want to be added, pls make vendor/pocketpy as a submodule to https://github.com/blueloveth/pocketpy
Also, pls add yourself to the credits https://github.com/nesbox/TIC-80#credits

@nesbox
Copy link
Owner

nesbox commented May 14, 2023

Also, we have to check if the embedded Python has access to the file system and network, if yes, we need to disable it somehow.

@koltenpearson
Copy link
Contributor Author

great! The os module for pocketpy is turned off, so it should not have access to the filesystem. Network access needs to be compiled in experimentally from what I can tell, and so is not active.

I am not sure how to go about making pocketpy a submodule, since the preferred distribution mode for pocketpy is as a single file source header. You are not supposed to build it from its repo as part of a bigger build process, you are just supposed to include the header, or at least that is what I was told . . .

I am guessing there might be a way with submodules to grab one of the release tar packages that github allows? I know @blueloveTH makes release versions of the pocketpy.h available in that way

@blueloveTH
Copy link
Contributor

blueloveTH commented May 15, 2023

Congratulations for this merge! It is possible to make it as a submodule. In this case, some custom build scripts need to be run before CMake command. I'd like to make a pr for this.

Also, pocketpy is in an early stage, the first production-ready version hasn't arrived. I will keep an eye on TIC-80's users' feedbacks on python and use these feedbacks to improve the interpreter itself.

@src3453
Copy link

src3453 commented May 15, 2023

I tried to import the math module and use math.radians, but it seems to give me an error, as if it is not defined.
image

@blueloveTH
Copy link
Contributor

@src3453 That's true. Only a subset of standard functions are implemented. You can use dir(math) to see what things are available.

math.radians and math.degrees will be added in the future.

@rwmpelstilzchen
Copy link

Thank you so much, @koltenpearson! ^_^

This is a dream come true 🙂. I tried do what you have done (using pocketpy in order to add Python support to TIC-80), but couldn’t manage to get it actually working (overall the changes are very similar, but mine missed some things).
TIC-80 + Python = 💘

This pull request was closed.
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.

5 participants