Conversation
nadiaholmquist
force-pushed
the
master
branch
3 times, most recently
from
e6c2fb6 to
df0ab90
Compare
RSDuck
left a comment
RSDuck
left a comment
There was a problem hiding this comment.
The LuaThread is a bit dubious and probably needs some thoughts how this could be implemented better.
src/frontend/qt_sdl/LuaMain.cpp
Outdated
| luaThread = new LuaThread(); | ||
| connect(luaThread,&LuaThread::signalPrint,console,&LuaConsole::onGetText); | ||
| connect(luaThread,&LuaThread::signalClearConsole,console,&LuaConsole::onClear); | ||
| connect(buttonPausePlay,&QPushButton::clicked,luaThread,&LuaThread::luaTogglePause); |
There was a problem hiding this comment.
placement of spacing here and in a bunch of other places.
|
Still some more changes needed but majority of the refactoring is done, no more LuaThread, the Lua state is created and run by the emuThread. This should hopefully address any concerns around potential race conditions. I plan on cleaning up other issues on Sunday. |
|
I think this should be good for another review, I believe I have addressed all the major issues that have been brought up. If there is anything else that I need to change or fix please let me know. There are still more features / functions I'd like to add, but this is the kind of project that could easily lead to feature creep, so I'm gonna cut myself off here. |
|
there's probably still some more things, but this is what I found from a glance. |
|
OK addressed those issues, still some bugs I found that I need to fix, I plan on working on that tomorrow. |
|
OK ready for another review @RSDuck when you have time. |
RSDuck
left a comment
RSDuck
left a comment
There was a problem hiding this comment.
you need to go over all the changes and fix the spacing. Then once the merge conflicts have been solved I guess we can merge it.
|
OK went over everything, double checked to make sure the spacing was good. Fixed spelling errors in the markdown. Will probably need someone else to take a look at the merge conflicts at least for the .yml and CMakeLists files. |
|
blah something went wrong while resolving the merge conflict. |
|
hm I don't really understand how dependencies are managed on macOS, I can't find anything on how it's installed in the code. |
nadiaholmquist
force-pushed
the
master
branch
from
10c70eb to
a7575ec
Compare
|
To the person who talked about this PR on our IRC: If you are reading this, we are very sorry, the ban was due to the anti spam bot bot for some reason flagging your messages. Let us know how you can communicate again with us. |
|
adding on Generic's message: I removed the ban, so you can come back to the IRC. sorry about it. |
|
Hey sorry I have been kind of a ghost for a while! It's been over a year since I started this fork, I'm not super up to date on all the changes since then. I'd like to get this project working again with the current version, since I am able to continue working on it now. |
|
there have been a ton of changes on the frontend especially with regards to multi instance within one process support. At this point it would probably smarter to not rebase/merge and instead start fresh while moving over the more independent parts like the Lua interface. |
|
Understood! I'd prefer that option honestly. Most of the work was on the Lua interpreter side of things, I tried to keep the front end stuff to a minimum while working on this since I figured I'd probably need to remake the front end entirely at some point. So I won't be loosing much. Also gives me a chance to refresh my memory on how everything works. I'll make a new fork later today, once that's up I'll add a link to the new one and close this pull request. |
Hello, sorry I just saw this. I don't really know how to use IRC, and I just saw you guys added a link to the discord so I'll probably stick to that since it's more familiar. I actually started a very rudimentary merge of this branch to main. (It definitely doesn't work, I was just using the merge conflicts to track which parts of the code base I hadn't yet looked at.) Because I've been busy IRL, I haven't touched anything in about 2 weeks, but before that I was mainly trying to get an overview of all the changes made externally to the Lua scripting module itself (i.e. what kind of integration was even necessary to begin with) as well as trying to understand the general structure of the codebase (and also what pieces got refactored, which were many).
I don't see the new fork yet on your profile, but I'll push the half-merge (and some of my notes) I've got so far to mine if you want to take a look (I was in the middle of figuring out how/where to associate a Lua scripting engine for each instance, before starting to refactor the Lua module as a class that could have multiple instances). my fork with the partial merge |
|
I'll be sure to take a look at your notes, although I think for right now my goal is simply build that can get a Although yes, one of the first things that will need to be figured out is how to handle multiple instances, since that is the biggest change from back when I first started. I should be able to work on it some today, I originally wanted to start on this like 2 weeks ago but my availability has been... to keep it simple, rather unpredictable as of recently. Hopefully I will have the time to get a good start this week, however, just so everyone is aware, my progress will likely remain somewhat scattered for now. I'll be sure to post a link to new fork here once I can get, at the bare minimum, "hello world" working on a single instance so we can have a starting point. In the meantime feel free to ask me any questions! Also thanks again to everyone for the help and patience. |
|
Hello, any update on this project ? Thanks ! |
|
Small update for those interested, (thanks to everyone who reached out about this) For those who have reached out asking if they can help in any way, I only ask for a little more time to get a minimal viable version working that is compatible with the recent updates to melonDS. Once that basic frame work is setup, I will be sure to let everyone know, and will then gladly appreciate any help! |
|
Hype! |
-Still lots todo! but I have a (mostly) working version up and running!
|
(My PRs fail on macOS too, so I think it might be unrelated.) |
|
I applied the fix from #2527 locally and this builds fine for macOS 👍🏻 |
fixed some documentation added checklist of bizhawk functions currently implemented / planned to implement / not planned to implement
|
O.K. Had a chance to double check things, and updated some of the documentation. I also added a checklist under Seems the macOS check passes now after syncing the recent changes. Feel free to go ahead and review everything, the only small change I still want to make would be to include a small warning / disclaimer that Lua scripting support is still early in development. Also I would like to note currently the biggest difference between my implementation and biz-hawk's Lua scripting, is that in order to run a Lua script continuously in the background, biz-hawk requires that It's not too difficult to patch a biz-hawk script so that 'emu.frameadvance()' instead yields from a co-routine, ie one called from '_Update()' but any scripts currently written for biz-hawk WILL need to be changed slightly to work on melonDS. I'm not sure it would be worth implementing Other then that I'd say this PR is pretty much ready to be merged! (approval pending...) |
|
Lua 5.5.0 released recently and it has some nice stuff like lower memory usage, so it might be worth updating the workflow files to point to it? it seems like it's just the ones for BSD and Linux/Ubuntu that need changing although I'm not sure if Lua 5.5.0 has been properly pushed as a package for those systems yet. |
|
@goodusername123 Thanks for pointing that out, it seems the lua5.5 package is still pending for debian / ubuntu. I don't have any clue how long it will take for the new package to get pushed. My understanding is that since each major revision of lua introduces some non-backwards compatible changes, some package maintainers choose to create a new "lua5.x" package separate from the previous version of lua. Which is different to how the "lua" vcpkg works, which has already been updated to lua5.5.0 (vcpkg is what the mac / windows workflows use) . So as far as the workflows are concerned, seems we might just have to wait for a lua5.5 package to get added for the Ubuntu builds. TBH messing around with installing packages is something I still struggle to understand, so I'm not sure if there might be other ways to handle lua packages that's more consistent across platforms. 🤔 |
In my experience the versioning/release structure of Lua isn't well suited for package managers in general (or well at least it seems like a lot of package managers don't know or care about the explicit breaking changes done each major Lua release), IMO it might make sense to just bring Lua into the repo locally which would solve all the headaches around compatibility related concerns stemming from package managers not considering the release structure around Lua in specific. An example i've seen of this in the wild is a project which uses Lua 5.3 and vcpkg. vcpkg treats Lua as a singular thing and so when Lua 5.4 came out vcpkg switched over to it meaning that the project is currently stuck with Lua 5.3.5 instead of the last release Lua 5.3.6 because vcpkg never marked a version for it due to moving up to Lua 5.4. |
This reverts commit e5b395e.
|
After experimenting a bit with getting Lua5.5 to work with this project, it seems that it might be best to just leave things with lua5.4 for now. I'd much rather focus on getting this feature up and running so that others can start using and testing it, rather then spending time trying to get the newest version of lua to work on every system melonDS runs on. I do plan on eventually getting lua5.5 to work, but for now, I'm going to say that's outside the scope of this pull request. |
Tested the test lua script with ACWW on OpenBSD/amd64 7.8-stable
Not exactly sure what it is supposed to do, but this is what i see. is this correct? the top right "block" of the top screen that gets overlaid via OSD is flickering and giving me a headache. |
|
No it's not supposed to do that... 😓 What are the video settings you are using? |
with the logo added to that dir, its a little better. 
with the classic renderer it does this too. also- i noticed the program segfaults if you try to load a script before you load a game. we should really have a guard against that. |
|
OK it appears to be an issue with the Open GL rendering that I thought I had fixed, but seems to be broken now. But at least I am able to re-create the issue on my end so I'll work on fixing that. Also yes I am aware of the seg fault issue when there is no game loaded, I will add in some safeguards for that as well. |
|
I feel I should ask here. Is this PR adding a new depend for the program on lua5.4.x? If so, is it an optional runtime-depend, is it dlopen'd at runtime and thus is non-optional, or is it just a build depend? This matters to me as I maintain the OpenBSD port for melonDS, and this will change how I package it. If this is not documented as a depend already, it should be. |
15 participants
Here is a proposed starting point to adding Lua script support for MelonDS natively. Currently has bare minimal functionality, as I would like to have others review my work before making to many additions. This is my first project in c++ so please let me know if there are any errors, or parts that I need to re-do. The aim for this is to support accessibility features and trackers, not as a comprehensive debugging tool.