Replace direct QtScript usage with an isolating interface. #1200
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
digisomni
added
do not merge
odysseus654
changed the title
odysseus654
force-pushed
the
feature/scripting
branch
6 times, most recently
from
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(). |
odysseus654
force-pushed
the
feature/scripting
branch
2 times, most recently
from
a7191c6
to
cbe608a
Compare
odysseus654
force-pushed
the
feature/scripting
branch
2 times, most recently
from
975d390
to
ab68c02
Compare
odysseus654
force-pushed
the
feature/scripting
branch
from
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. |
odysseus654
force-pushed
the
feature/scripting
branch
3 times, most recently
from
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) |
odysseus654
force-pushed
the
feature/scripting
branch
from
cdfde39
to
d9ddcb5
Compare
…letion of the proxy interface
odysseus654
force-pushed
the
feature/scripting
branch
from
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. |
|
It seems like it crashes after some time, or some amount of use. After crash properties of objects on the list are not displayed anymore: |
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? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.