-
Notifications
You must be signed in to change notification settings - Fork 176
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
Replace direct QtScript usage with an isolating interface. #1200
base: master
Are you sure you want to change the base?
Conversation
e20b6cd
to
3308d06
Compare
I've been looking this over a bit. Overall it looks good, but this is going to need documentation about what you're doing here, what's changing, and what the desired end state is. So for instance:
I think it's important to have a plan upfront here to be clear on what's changing, why, and to have a good idea of what's not done yet, what's intentional, and what's a bug. |
Currently there are three interfaces being introduced here:
These interfaces cannot be directly created and do not inherit from QObject (directly), so cannot have any slots or signals. All interfaces are pointers which is why ScriptValue is now a pointer (where QScriptValue is generally not a pointer). Using QSharedPointer for these objects also means that copying them doesn't involve calling into the scripting engine to make a copy of the actual value. The choice of QSharedPointer over std::shared_ptr is mostly as our project is a Qt project. Choosing one over the other won't really affect much. ScriptManager is a QObject containing most of the code that is common between the scripting engines. It will encapsulate the main Run() function as well as all the Vircadia objects that are shoved into globalObject(). |
a7191c6
to
cbe608a
Compare
975d390
to
ab68c02
Compare
ab68c02
to
5e36015
Compare
I'm unsure what to do to make sure "tests" and "tests-manual" work, offhand they don't appear to be part of the project that gets compiled. |
b6c159c
to
cdfde39
Compare
If it helps, here are one-line descriptions for at least the classes in script-engine: http://protocol.odys.se/script-engine/annotated.html [ScriptInterface] are new abstract classes to create engine-independent interfaces everything else is as minimally touched while having scripting references redirected to engine-independent interfaces. Very surprisingly... I'm not seeing any linker dependencies from script-engine to other libraries (except perhaps shared) |
cdfde39
to
d9ddcb5
Compare
…letion of the proxy interface
d9ddcb5
to
b52624c
Compare
The following links are available:
build (macOS-10.15, client) build (macOS-10.15, full) build (windows-2019, full) |
FWIW I'm not just updating the branch to HEAD, I am actually trying to develop patches here. Probably not quite ready yet though. |
My recent PR #1622 has some conflicts with this, so wanted to give a heads up and context on it. There are a lot of files affected, but most of it should be straight forward to merge, since I didn't really change much, just moved things around, basically splitting the shared library in two. |
I tested it thoroughly and noticed a few problems: In voxels.js app on subtracting voxels:
createCow.js fails with:
Blocks app fails with:
Seems to appear randomly:
Create App fails to create shapes, they appear at 0,0,0 and not near avatar:
Create app emits these when switching between Create and Properties tabs:
Create app emits these when switching between objects in Entity Lists:
On toggling grid visibility in create app:
Doppelganger app doesn't work. After a crash and selecting to reset non-essential settings it immediately crashes with this message:
Call stack:
Later it keeps crashing in tutorial world with SIGABRT but no message
|
The scripting console doesn't seem to work. Any command entered there results in following debug message: |
Bug preventing various scripts from creating entities at given position (like for example createCow.js, or createPistol.js) is due to Vec3.multiply not working correctly.
Returns Edit: Reversed argument order gives correct result, so it's only one of the overloaded operators that is failing. |
Thank you for looking at this, these kinds of "function overload evaluation" is brand-new code and likely one of the weakest points (and most need of testing) Was hoping to get more done this weekend but... have some energy right now so we'll see how many of these simpler issues I can resolve |
…'s used as a return datatype in a script-callable function (and doesn't get auto-registered)
… accept and discard script parameters beyond that count
…ts when we're ignoring superclasses
Initial thoughts, hoping to push once I actually do a bit of testing (dunno how much though):
|
Not fully satisfied with the changes but pushed what I have. Getting a crash on launch (which doesn't immediately seem relevant) and a crash on shutdown (which seems more relevant) |
The following links are available: |
I built and tested the most recent version. Scripting console now works :) Explore App: Create app doesn't display properties, causes these errors:
in voxels.js createPistol.js fails:
|
I tested it again, and create app throws a lot of errors, but properties display correctly now. Adding new objects also works as indended now.
|
This is a common message in production builds. Just fyi. |
File dialog in running scripts:
Overloading still doesn't work: |
There are also TypeErrors during shutdown:
|
Hello! Is this still an issue? |
Hello! Is this still an issue? |
This is documenting my attempts to isolate QtScript to interfaces that we control, to enable us to add or replace scripting engines in the future.