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

Implement transpiler support #8833

Merged
merged 3 commits into from May 10, 2024
Merged

Implement transpiler support #8833

merged 3 commits into from May 10, 2024

Conversation

vlaaad
Copy link
Contributor

@vlaaad vlaaad commented Apr 23, 2024

This changeset adds initial Bob and Editor support for plugins that transpile 3rd-party languages to Lua. Transpiler plugins need to implement an ILuaTranspiler interface. See the Teal extension for reference implementation.

Fixes #8291

@vlaaad vlaaad force-pushed the DEF-8291 branch 2 times, most recently from 35a435e to f15c09b Compare April 25, 2024 14:09
This changeset adds initial Bob and Editor support for plugins that transpile 3rd-party languages to Lua. Transpiler plugins need to implement an `ILuaTranspiler` interface. See [the Teal extension](https://github.com/defold/extension-teal) for reference implementation.

Fixes #8291
(:require [clojure.string :as string]
[dynamo.graph :as g]
[editor.build-target :as bt]
(:require [dynamo.graph :as g]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved script compilation to a different file (script_compilation.clj)

:modules preprocessed-modules
:proj-path (resource/proj-path resource)}
:deps preprocessed-go-prop-dep-build-targets
:dynamic-deps (mapv lua/lua-module->path preprocessed-modules)})]))))))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

99% of the code in this file is taken from script.clj. There is a small behavior difference: module dependencies are now :dynamic-deps that only reference dependent file paths instead of including the dependent build targets.

Comment on lines +673 to +681
(defn reload-plugins! [project touched-resources]
(g/with-auto-evaluation-context evaluation-context
(let [workspace (workspace project evaluation-context)
code-preprocessors (workspace/code-preprocessors workspace evaluation-context)
code-transpilers (code-transpilers project)]
(workspace/unpack-editor-plugins! workspace touched-resources)
(code.preprocessors/reload-lua-preprocessors! code-preprocessors workspace/class-loader)
(code.transpilers/reload-lua-transpilers! code-transpilers workspace workspace/class-loader)
(workspace/load-clojure-editor-plugins! workspace touched-resources))))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved plugin reload from workspace to project because it's needed for transpilers.

Comment on lines -106 to -136
(defn flatten-build-targets
"Breadth first traversal / collection of build-targets and their child :deps,
skipping seen targets identified by the :content-hash of each build-target."
([build-targets]
(flatten-build-targets build-targets #{}))
([build-targets seen-content-hashes]
(assert (set? seen-content-hashes))
(loop [targets build-targets
queue []
seen seen-content-hashes
result (transient [])]
(if-some [target (first targets)]
(let [content-hash (:content-hash target)]
(assert (bt/content-hash? content-hash)
(str "Build target has invalid content-hash: "
(resource/resource->proj-path (:resource target))))
(if (contains? seen content-hash)
(recur (rest targets)
queue
seen
result)
(recur (rest targets)
(conj queue (flatten (:deps target)))
(conj seen content-hash)
(conj! result target))))
(if-some [targets (first queue)]
(recur targets
(rest queue)
seen
result)
(persistent! result))))))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to editor/build.cj

@vlaaad vlaaad requested review from matgis and britzl May 3, 2024 08:41
Copy link
Contributor

@matgis matgis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks mostly good to me, but there are some open questions and concerns.

britzl
britzl previously approved these changes May 7, 2024
Copy link
Contributor

@britzl britzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I made a note about a code comment that could be potentially helpful.

:ddf-type AtlasProto$Atlas
:load-fn load-atlas
:icon atlas-icon
:icon-class :design
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly indentation, but also added :icon-class

:ddf-type GameSystem$CollectionProxyDesc
:load-fn load-collection-proxy
:icon collection-proxy-icon
:icon-class :property
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added :icon-class

:load-fn load-collision-object
:sanitize-fn sanitize-collision-object
:icon collision-object-icon
:icon-class :design
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added :icon-class

(code-transpilers project)
node-id
(resource/proj-path resource))]
(cond-> load-tx-steps transpiler-tx-steps (concat transpiler-tx-steps)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After addressing your concerns, the performance looks like this:

project size total load time (ms) transpiler overhead (ms) decline (%)
small 724.76 0.80 0.07%
big (without transpilers) 179477.23 19.34 0.01%
big (with transpilers) 174382.14 27.21 0.02%

I think this is fine!

@vlaaad vlaaad requested review from matgis and britzl May 8, 2024 14:02
Copy link
Contributor

@matgis matgis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to merge! I really like the addition of :icon-class to the resource types. ❤️

@vlaaad vlaaad merged commit da615eb into dev May 10, 2024
22 checks passed
@vlaaad vlaaad deleted the DEF-8291 branch May 10, 2024 07:21
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.

None yet

3 participants