mapeditor / tiled

Flexible level editor
https://www.mapeditor.org/
Other
11.27k stars 1.76k forks source link

Javascript plugin stdlib #2780

Open konsumer opened 4 years ago

konsumer commented 4 years ago

First of all, great work on tiled. I love it, and have used it for years. It pretty much works great for anything that uses tiles I have ever tried it with. Excellent software.

Recently, I was using tiled-to-godot-export, which uses the javascript plugin API to generate godot map & tile definitions. It works pretty well, but in order to support relative paths, I had to roll my own path function (that is not nearly as good as node's path.resolve.)

It'd be really useful for plugin API to have some basic js stdlib type stuff like node's exec/os/path/fs for manipulating paths, spawning processes, introspecting the environment, and manipulating files, directly. In my opinion, keeping it as close as possible to the node API would be really great, since we can go light on documentation, and just point people there, and it will be familiar to experienced javascript devs. "It works like path.resolve" would be super-helpful to people like me.

I briefly explored the exposed Qt object, hoping it would have something in it, but I couldn't find anything related.

I am making this issue to start a discussion about it, and I am happy to help with anything I can, even though I don't do too much C++/Qt dev, I can get around ok, have a lot of experience with node, and could probly work out how to add support.

How should this look? Maybe a first step would be to make path bindings? If the posix and win32 specific sets are stripped out, you get a pretty small surface, so it might be a useful & relatively easy starting point to expose:

path.basename(path[, ext])
path.delimiter
path.dirname(path)
path.extname(path)
path.format(pathObject)
path.isAbsolute(path)
path.join([...paths])
path.normalize(path)
path.parse(path)
path.relative(from, to)
path.resolve([...paths])
path.sep
path.toNamespacedPath(path)
konsumer commented 4 years ago

With my basic understanding, I am guessing I need to add an object to script-manager, like this? How does Stdlib global sound for this kind of stuff? Like I could start with Stdlib.path, then users can do this:

const { path: { basename } } = Stdlib
console.log(basename(file))

which looks very similar to equivalent in node:

const { basename } = require('path')
console.log(basename(file))
konsumer commented 4 years ago

Or maybe it should go in ScriptModule, so it's all available in tiled object? I'm really not sure.

bjorn commented 4 years ago

Thanks @konsumer for the kind words and the thorough suggestion!

Most of the functionality behind those functions appears to be available in QDir and QFileInfo.

I'm personally a bit undecided whether to stay closer to the Qt API or whether to mimic the Node API. There's another JavaScript-powered tool that I'm using which is Qbs, the build system I use for Tiled. It comes with a FileInfo service that also provides most of this functionality. I would probably have gone for that API, and it is also what the TextFile and BinaryFile are based on and what the future XML API (#2735) would be based upon. Actually we could probably take most of its services and port them over (they need porting because Qbs uses the deprecated QScriptEngine whereas in Tiled we're using QJSEngine).

Would you have a strong opinion either way, or do either APIs sound fine?

konsumer commented 4 years ago

Nah, I see the value of that. I personally gravitate towards being similar to node, just because I am already familiar with it, and it seems sensibly sort of like other things (like python or ruby standard lib) but honestly I don't think it matters at all, and I do like that it's already built in, and also that it will be easier to maintain in tiled. I think plugin-devs will pick it up, even if it's completely different. Since tiled isn't in the business of making a total node-compatibility environment in the scripting environment, it might be a lot of unnecessary work & overhead, and I have definitely come up against a kind of uncanny valley before, that can be frustrating when things don't work exactly the same, but are sort of similar. I think it might also encourage new devs to "think in Qt" which will empower them when they come up against other things to solve.

That said, I'm not quite sure how to start, as a total Qt-noob. Would it be possible for someone who knows how it fits together better to get started on a FileInfo class and expose it to the script, maybe with just 1 method (baseName might be a good candidate, to start with) and I can fill in the rest? I dunno if that's helpful, but I'm eager to help, even if I don't fully understand it, yet. Would it be methods of the other objects, or a separate utility class?

bjorn commented 4 years ago

@konsumer Alright, I think the two most important arguments are indeed that it may not be helpful to have an API working like Node but not exactly like Node (and possibly related, I would not want to go through he trouble of providing any asynchronous API right now). And second that it will save a lot of work if we can reuse the code and documentation from the Qbs project.

I don't have much time to explain now, but please look at how TextFile and BinaryFile are implemented. They are implemented in scriptfile.h/cpp and exposed to the script engine in ScriptManager::setupEngine.

The main difference with FileInfo is that you should export it like done with the tiled module (so newQObject instead of newQMetaObject), because the scripts are not expected to instantiate instances of this type (you do this in C++ and pass the instance to newQObject).

Finally, Qbs is free software and its source code for its FileInfo service is available here. Note that this will look a lot nicer once ported to QJSEngine since the QObject derived instance will expose any function marked as Q_INVOKABLE automatically including dealing with different parameter types. I think looking at that code is mostly only useful to see how they implemented the functionality based on QFileInfo and QDir.

konsumer commented 4 years ago

I will take a look and see if I can figure it out.

More ideas:

I noticed ScriptModule already has platform and arch, which is what I would primarily use node's os for (or process) in this context, and TextFile and BinaryFile have read/write/etc, so I agree, really out of the basic stdlib stuff, the only things that are needed are FileInfo methods and maybe eventually something like node's exec (so people can write lil wrappers around existing command-line tools.) I could see tmpdir and homedir being useful, but also maybe it's ok to not encourage writing outside of output-dir too much, as a first iteration.

I also wonder if maybe __dirname and __filename might be useful for detecting the location of the current script-file (especially useful for CLI wrappers, because you can include them in the plugin-folder,) but I bet there is already something around that does that, and even if not, maybe that's ok for a first step.

konsumer commented 4 years ago

I created a start to this, but can't get it to link:

class ScriptFileInfo : public QObject
{
    Q_OBJECT

public:
    ScriptFileInfo(QObject *parent = nullptr);

    static QString fileName(const QString &fp);
    static QString baseName(const QString &fp);
};

I think I setup all the scaffolding and it's building it all correctly, but the linker isn't including the built cpp file. I'm sure it's something I did wrong:

qbs run -p tiled

ERROR: /usr/bin/ld: /home/konsumer/Documents/otherdev/tiled/default/tiled.1920fdf8/3a52ce780950d4d9/scriptmanager.cpp.o: in function `Tiled::ScriptManager::setupEngine()':
/home/konsumer/Documents/otherdev/tiled/src/tiled/scriptmanager.cpp:284: undefined reference to `ScriptFileInfo::ScriptFileInfo(QObject*)'
collect2: error: ld returned 1 exit status
ERROR: Process failed with exit code 1.
The following products could not be built for configuration default:
tiled

If someone can point out what I did wrong, I put it in this branch

bjorn commented 4 years ago

To get it to link you need to add the scriptfileinfo.cpp to the src/tiled/tiled.qbs project file. Please add it to src/tiled/tiled.pro as well to make sure the project can also still be built using QMake.

As for the ScriptFileInfo class. Note that you should not make its method static, but they should be marked with Q_INVOKABLE to make them callable from JavaScript.

And of course, thanks for your help with implementing this stuff!

konsumer commented 4 years ago

That worked!

So we can do this, in that branch, in a plugin:

console.log('BASENAME', FileInfo.baseName(fileName))

I tested with the godot plugin, and it's working great. I will fill in the rest. Thanks for your help!

konsumer commented 4 years ago

I'm noticing I need to copy a lot of included files to replicate QBS. I just noticed at the top of the docs it says

If you instead need to access the service from a JavaScript file, import it using the following statement at the top of your JavaScript file:

var File = require("qbs.File");

I tried this and it silently didn't load the godot plugin. Is there some other thing I could do to make this work, instead? it seems like it might be simpler & less redundant.

bjorn commented 4 years ago

I'm noticing I need to copy a lot of included files to replicate QBS.

Why? Notice that they are using QScriptEngine and not QJSEngine. Our code should look at lot simpler because it's just a matter of exposing the set of functions with Q_INVOKABLE. We don't need to jump through all those hoops like they're doing in Qbs sources. I was only referring to those sources so you can see how QDir and QFileInfo are used to implement the functionality.

Also, let's just add FileInfo to the root object. We don't need to add require, at least not for now (I think we do eventually need to look into enabling modules, but it's a different topic).

Btw, feel free to open a pull request early so I can provide feedback more easily.

konsumer commented 4 years ago

Forgive the basic Qt/C++ questions. I am a total noob. I can look around for some Qt forums if it is taking up too much of your time. I really want to help, but am way out of my area of expertise.

Why? Notice that they are using QScriptEngine and not QJSEngine. Our code should look at lot simpler because it's just a matter of exposing the set of functions with Q_INVOKABLE.

This may be the key area I don't understand. With fileName and baseName I just followed the includes back to fileinfo.cpp and copied the code. I can fill in the rest of them that work without other includes, as a starter, but a lot of the really useful methods (exists, path, etc) need other includes.

We don't need to jump through all those hoops like they're doing in Qbs sources. I was only referring to those sources so you can see how QDir and QFileInfo are used to implement the functionality.

Ok. I guess the "hoops" part is what I am having trouble with. If i just copy QBS code (as I did with fileName and baseName) I start needing lots of other things they setup like hostosinfo.h and filetime.h and persistence.h and a few others. It starts to feel like a rabbit-hole of redundant QBS implementations. I am probably doing it all wrong.

Also, let's just add FileInfo to the root object. We don't need to add require, at least not for now (I think we do eventually need to look into enabling modules, but it's a different topic).

Sounds good. I like that better, anyway, it just seemed like maybe it was already included in tiled with QBS, but if I'm following what you are saying, it's not, but fairly simple to add.

bjorn commented 4 years ago

Ok. I guess the "hoops" part is what I am having trouble with. If i just copy QBS code (as I did with fileName and baseName) I start needing lots of other things they setup like hostosinfo.h and filetime.h and persistence.h and a few others. It starts to feel like a rabbit-hole of redundant QBS implementations. I am probably doing it all wrong.

Puh, maybe I should have read the source code I was pointing at a little more carefully. I thought they would just be using Qt's API but it appears they reimplemented a bunch of things, possibly for performance reasons or otherwise.

In any case, then maybe don't look too much at their implementation and instead see if you can just use QDir and QFileInfo to implement the desired functionality.

Sorry about that!

konsumer commented 4 years ago

Sorry about that!

No prob! I am just fumbling around here, and your advice is extremely helpful.

In any case, then maybe don't look too much at their implementation and instead see if you can just use QDir and QFileInfo to implement the desired functionality.

Will do! I was starting to wonder if this is what you meant. That makes a lot of sense. I would way prefer to just wrap QFileInfo as thinly as possible. I think you've given me all the tools, I will go try to put them together.

konsumer commented 4 years ago

OMG! That is so much simpler. I got through most of the API, and it was a breeze.

I got all of them worked out, except those that use QFile::Permissions or QFile::FileTime. If you check out the branch, that's most of it!

I also made a demo plugin here to play around with it.

Here is some example output:

{
  "absoluteDir": "/home/konsumer/Documents/otherdev/tiled-example",
  "absoluteFilePath": "/home/konsumer/Documents/otherdev/tiled-example/town1.json",
  "absolutePath": "/home/konsumer/Documents/otherdev/tiled-example",
  "baseName": "town1",
  "birthTime": "2020-04-20T23:20:18.807Z",
  "bundleName": "",
  "caching": true,
  "canonicalFilePath": "/home/konsumer/Documents/otherdev/tiled-example/town1.json",
  "canonicalPath": "/home/konsumer/Documents/otherdev/tiled-example",
  "completeBaseName": "town1",
  "completeSuffix": "json",
  "dir": "/home/konsumer/Documents/otherdev/tiled-example",
  "exists": true,
  "fileName": "town1.json",
  "filePath": "/home/konsumer/Documents/otherdev/tiled-example/town1.json",
  "group": "konsumer",
  "groupId": 1000,
  "isAbsolute": true,
  "isBundle": false,
  "isDir": false,
  "isExecutable": false,
  "isFile": true,
  "isHidden": false,
  "isNativePath": true,
  "isReadable": true,
  "isRelative": false,
  "isRoot": false,
  "isShortcut": false,
  "isSymLink": false,
  "isSymbolicLink": false,
  "isWritable": true,
  "lastModified": "2020-04-20T23:20:18.807Z",
  "lastRead": "2020-04-20T23:20:18.807Z",
  "makeAbsolute": false,
  "metadataChangeTime": "2020-04-20T23:20:18.807Z",
  "owner": "konsumer",
  "ownerId": 1000,
  "path": "/home/konsumer/Documents/otherdev/tiled-example",
  "size": 1205,
  "suffix": "json",
  "symLinkTarget": ""
}

I still need to do a little testing, and fix the missing type methods, but otherwise, I'm very happy with this.

konsumer commented 4 years ago

I dunno if it's the right thing to do, but I cast the enums to uint and it seems to work. I'm going to PR, if that all seems good.

konsumer commented 4 years ago

Ok, I think I have packaged all this in Qbs4QJS, specifically File and FileInfo. Not sure how we should get it in here, though.

Qbs4QJS also includes lots of other standard things, like BinaryFile, TextFile, and the other useful services from Qbs. I have exposed enums wherever they are used, and I think it's a pretty accurate copy of Qbs JS services, at this point.

I'm happy to add any extra (not in Qbs) things that seem useful, as well.