-
Notifications
You must be signed in to change notification settings - Fork 41
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
Web exports guide #15
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Thanks for bringing everything together, it's looking great. 👍
Mostly nitpicks incoming now. 😄
src/toolchain/web-guide.md
Outdated
|
||
# Intro to Web Builds | ||
|
||
Web builds are a fair bit more to get started with compared to native builds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might sound a bit more natural:
Web builds are a fair bit more to get started with compared to native builds. | |
Web builds are a fair bit more difficult to get started with compared to native builds. |
src/toolchain/web-guide.md
Outdated
# Intro to Web Builds | ||
|
||
Web builds are a fair bit more to get started with compared to native builds. | ||
This will be a complete guide on how to get things compiled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be a bit more descriptive:
This will be a complete guide on how to get things compiled. | |
This will be a complete guide on how to properly compile your game using `gdext` to WebAssembly, which will allow you to use `gdext` in the web. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the extension, but I think "properly" is not needed 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! Feel free to remove "properly"
src/toolchain/web-guide.md
Outdated
|
||
Web builds are a fair bit more to get started with compared to native builds. | ||
This will be a complete guide on how to get things compiled. | ||
Setting up a web server is out-of-scope for this guide, and best found elsewhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being a bit more precise (also I think no hyphens are needed here):
Setting up a web server is out-of-scope for this guide, and best found elsewhere. | |
Setting up a web server to host your game in is out of scope for this guide, and is best explained elsewhere. |
src/toolchain/web-guide.md
Outdated
This will be a complete guide on how to get things compiled. | ||
Setting up a web server is out-of-scope for this guide, and best found elsewhere. | ||
|
||
- Web support with `gdext` is experimental and should be understood as such before proceeding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the gdext docs have support for warning boxes? I believe that'd be ideal here. Otherwise, we could add something in bold like WARNING:.
Also, I think it may be interesting to point out explicitly here that it "may have bugs we aren't aware of".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you'll get a compile error if you don't enable the experimental-wasm
feature flag. Beyond that I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have warning boxes, see ```admonish
tags elsewhere.
But only the 2nd bullet point is a warning. The first can also be moved up to the "This will be a complete guide..." section.
src/toolchain/web-guide.md
Outdated
## Installing Pre-Requisites | ||
|
||
- Install a nightly build of `rustc`, the `wasm32-unknown-emscripten` target for `rustc`, and `rust-src`. | ||
Assuming that Rust was installed with rustup, this is quite simple. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency:
Assuming that Rust was installed with rustup, this is quite simple. | |
Assuming that Rust was installed with `rustup`, this is quite simple. |
src/toolchain/web-guide.md
Outdated
- Edit the project's `.gdextension` to include the web path. | ||
This is probably at `godot/EXTENSIONNAME.gdextension`. | ||
The format will be similar to the following |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearer (what is the "web path"?)
- Edit the project's `.gdextension` to include the web path. | |
This is probably at `godot/EXTENSIONNAME.gdextension`. | |
The format will be similar to the following | |
- Edit the project's `.gdextension` file to include the path to the compiled WASM binary. | |
The file to edit will probably be at `godot/EXTENSIONNAME.gdextension`. | |
The format will be similar to the following |
src/toolchain/web-guide.md
Outdated
``` | ||
|
||
- Compile the code. | ||
It is necessary to both use the nightly compiler and specify to build std[^3], along with specifying the emscripten target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotta check if the monospace doesn't break the footnote, but I believe it shouldn't
It is necessary to both use the nightly compiler and specify to build std[^3], along with specifying the emscripten target. | |
It is necessary to both use the nightly compiler and specify to build `std`[^3], along with specifying the Emscripten target. |
src/toolchain/web-guide.md
Outdated
cargo +nightly build -Zbuild-std --target wasm32-unknown-emscripten | ||
``` | ||
|
||
[^3]: The primary reason for this is it's necessary to compile with `-sSHARED_MEMORY` enabled, but the shipped `std` is not, so building std is a requirement. Related info on about wasm support can be found [here](https://github.com/rust-lang/rust/issues/77839). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency and other things
[^3]: The primary reason for this is it's necessary to compile with `-sSHARED_MEMORY` enabled, but the shipped `std` is not, so building std is a requirement. Related info on about wasm support can be found [here](https://github.com/rust-lang/rust/issues/77839). | |
[^3]: The primary reason for this is it's necessary to compile with `-sSHARED_MEMORY` enabled, which the shipped `std` doesn't, so building `std` is a requirement. Related info on WASM support can be found [here](https://github.com/rust-lang/rust/issues/77839). |
src/toolchain/web-guide.md
Outdated
- Back from the main editor screen, there's an option to run the web debug build without needing to do an export or set up a web server. | ||
At the top-right, choose `Remote Debug > Run in Browser` and it will automatically open up a web browser. | ||
If your default browser is not Chrome or Edge, you'll need to copy the URL and paste it into one.[^4] | ||
![Location of built-in web server](images/web-browser_run.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Back from the main editor screen, there's an option to run the web debug build without needing to do an export or set up a web server. | |
At the top-right, choose `Remote Debug > Run in Browser` and it will automatically open up a web browser. | |
If your default browser is not Chrome or Edge, you'll need to copy the URL and paste it into one.[^4] | |
![Location of built-in web server](images/web-browser_run.png) | |
- Back to the main editor screen, there's an option to run the web debug build (_not_ release) locally without needing to fully export the game or set up a web server. | |
At the top right, choose `Remote Debug > Run in Browser` and it will automatically open up a web browser. | |
If your default browser is not Chromium-based (e.g. if you use Firefox or Safari, currently unsupported), you'll need to copy the URL (which is usually at `localhost:8060`) and open it in a supported browser, such as Chrome or Edge.[^4] | |
![Location of built-in web server](images/web-browser_run.png) |
src/toolchain/web-guide.md
Outdated
- Currently the only option for wasm debugging is | ||
[This extension](https://chromewebstore.google.com/detail/cc++-devtools-support-dwa/pdcpmagijalfljmkmjngeonclgbbannb?pli=1) | ||
for Chrome. It adds support for breakpoints and a memory viewer into the F12 menu. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency etc.
- Currently the only option for wasm debugging is | |
[This extension](https://chromewebstore.google.com/detail/cc++-devtools-support-dwa/pdcpmagijalfljmkmjngeonclgbbannb?pli=1) | |
for Chrome. It adds support for breakpoints and a memory viewer into the F12 menu. | |
- Currently the only option for WASM debugging is | |
[this extension](https://chromewebstore.google.com/detail/cc++-devtools-support-dwa/pdcpmagijalfljmkmjngeonclgbbannb?pli=1) | |
for Chrome. It adds support for breakpoints and a memory viewer into the F12 menu. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, this is a great write-up! 👍
Much more in detail than my short list 😅
Do you also want to mention the dodge-the-creeps example, which is somewhat prepared already?
The entire page is now consisting of bullet points, I don't think this is necessary. Tutorial style guides have continuous flow, so I think you could de-dent most text and use regular prose.
Unordered bullet lists can be used in places where you explain multiple things, but their order doesn't really matter. Ordered lists can be used when order matters or you later want to refer back to a certain point, e.g. you analyze a code snippet and explain the symbols occurring in order of appearance in code. But I wouldn't generally use lists for tutorial steps (as the whole book is such). See also Hello World page.
src/SUMMARY.md
Outdated
@@ -15,6 +15,7 @@ | |||
- [Compatibility and stability](toolchain/compatibility.md) | |||
- [Selecting a Godot version](toolchain/godot-version.md) | |||
- [Debugging](toolchain/debugging.md) | |||
- [Web Target Guide](toolchain/web-guide.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [Web Target Guide](toolchain/web-guide.md) | |
- [Export to Web](toolchain/export-web.md) |
src/toolchain/web-guide.md
Outdated
# Intro to Web Builds | ||
|
||
Web builds are a fair bit more to get started with compared to native builds. | ||
This will be a complete guide on how to get things compiled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the extension, but I think "properly" is not needed 😉
src/toolchain/web-guide.md
Outdated
This will be a complete guide on how to get things compiled. | ||
Setting up a web server is out-of-scope for this guide, and best found elsewhere. | ||
|
||
- Web support with `gdext` is experimental and should be understood as such before proceeding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have warning boxes, see ```admonish
tags elsewhere.
But only the 2nd bullet point is a warning. The first can also be moved up to the "This will be a complete guide..." section.
src/toolchain/web-guide.md
Outdated
- Install a nightly build of `rustc`, the `wasm32-unknown-emscripten` target for `rustc`, and `rust-src`. | ||
Assuming that Rust was installed with `rustup`, this is quite simple. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add that nightly is needed for the build flag -Zbuild-std
.
src/toolchain/web-guide.md
Outdated
|
||
```sh | ||
rustup toolchain install nightly | ||
rustup component add rust-src --toolchain nightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this 2nd step really needed? AFAIR I never did this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't necessary for me, but someone in the discord somehow didn't have rust-src. So, I think best to leave it there as a just in case? Shouldn't do anything if it's already installed, at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I was installing everything from scratch in a Linux VM and rustup didn't seem to install it by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it looks like it's required:
rust-src
— This is a local copy of the source code of the Rust standard library. This can be used by some tools, such as RLS, to provide auto-completion for functions within the standard library; Miri which is a Rust interpreter; and Cargo's experimental build-std feature, which allows you to rebuild the standard library locally.
src/toolchain/web-guide.md
Outdated
``` | ||
|
||
- At the same level as your `Cargo.toml`, create a `.config` directory and inside of that, | ||
create a `config.toml` file (if one doesn't already exist). The contents of this file should be as follows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a `config.toml` file (if one doesn't already exist). The contents of this file should be as follows | |
create a `config.toml` file (if one doesn't already exist). The contents of this file should be as follows: |
src/toolchain/web-guide.md
Outdated
``` | ||
|
||
- Edit the project's `.gdextension` file to include support for web exports. | ||
This file will probably be at `godot/EXTENSIONNAME.gdextension`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file will probably be at `godot/EXTENSIONNAME.gdextension`. | |
This file will probably be at `godot/{YourCrate}.gdextension`. |
to be consistent with Hello World.
src/toolchain/web-guide.md
Outdated
|
||
- Edit the project's `.gdextension` file to include support for web exports. | ||
This file will probably be at `godot/EXTENSIONNAME.gdextension`. | ||
The format will be similar to the following |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format will be similar to the following | |
The format will be similar to the following: |
src/toolchain/web-guide.md
Outdated
--- | ||
|
||
|
||
## **KNOWN CAVEATS** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not use SCREAMING CASE here.
## **KNOWN CAVEATS** | |
## Known caveats |
Or alternatively, an ```admonish warning title="Known caveats"
box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be named web-browser-run.png
(dashes)
src/toolchain/web-guide.md
Outdated
--- | ||
|
||
|
||
## Set up and Test in the Godot Editor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Set up and Test in the Godot Editor | |
## Godot Editor setup |
Thanks @Bromeon, didn't know about admonish stuff before :) Hopefully all is well, and I can squash to a single commit then. |
src/toolchain/export-web.md
Outdated
godot = { git = "https://github.com/godot-rust/gdext", branch = "master", features = ["experimental-wasm", "lazy-function-tables"] } | ||
``` | ||
|
||
At the same level as your `Cargo.toml`, create a `.config` directory and inside of that, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: turns out I had already suggested this above, sorry lol
Keeping this here anyway for future reference :p
At the same level as your `Cargo.toml`, create a `.config` directory and inside of that, | |
At the same level as your `Cargo.toml`, create a `.cargo` directory and, inside of that, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rendered it now and found a few more things 😉 the footnotes are great, thanks a lot for the thorough descriptions and external links!
In this repo, no need to squash in a single commit, I can do squash-on-merge.
src/toolchain/export-web.md
Outdated
This will be a complete guide on how to get things compiled. | ||
However, setting up a web server to host and share your game is considered out of scope of this guide, and is best explained elsewhere. | ||
|
||
Information to consider before starting: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should no longer be necessary, the yellow warning box draws enough attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 14?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, only this part:
Information to consider before starting:
src/toolchain/export-web.md
Outdated
--- | ||
|
||
|
||
## Installing Pre-Requisites |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Installing Pre-Requisites | |
## Installing pre-requisites |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
src/toolchain/export-web.md
Outdated
## Installing Pre-Requisites | ||
|
||
Install a nightly build of `rustc`, the `wasm32-unknown-emscripten` target for `rustc`, and `rust-src`. | ||
The reason that a nightly `rustc` is required is that nightly is required to build `std` (`-Zbuild-std`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason that a nightly `rustc` is required is that nightly is required to build `std` (`-Zbuild-std`). | |
The reason why nightly `rustc` is required is the unstable flag to build `std` ([`-Zbuild-std`][flag-build-std]). |
With
[flag-build-std]: https://doc.rust-lang.org/cargo/reference/unstable.html#list-of-unstable-features
src/toolchain/export-web.md
Outdated
rustup target add wasm32-unknown-emscripten --toolchain nightly | ||
``` | ||
|
||
Next, install Emscripten. The simplest way to achieve this is to install `emsdk` from the git repo. Recommended to use version 3.1.39 for now[^1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next, install Emscripten. The simplest way to achieve this is to install `emsdk` from the git repo. Recommended to use version 3.1.39 for now[^1] | |
Next, install Emscripten. The simplest way to achieve this is to install [`emsdk` from the git repo][emsdk-git]. | |
We recommend version 3.1.39 for now.[^1] |
with:
[emsdk-git]: https://github.com/emscripten-core/emsdk#readme
(also added trailing full-stop).
src/toolchain/export-web.md
Outdated
source ./emsdk.sh (or ./emsdk.bat on windows) | ||
``` | ||
|
||
It would also be HIGHLY recommended to follow the instructions in the terminal to add `emcc`[^2] to your `PATH`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be HIGHLY recommended to follow the instructions in the terminal to add `emcc`[^2] to your `PATH`. | |
It would also be **highly** recommended to follow the instructions in the terminal to add `emcc`[^2] to your `PATH`. |
src/toolchain/export-web.md
Outdated
- Create a `.cargo` directory at the same level as your `Cargo.toml`. | ||
- Inside of that directory, create a `config.toml` file. | ||
|
||
The contents of this file need to contain the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contents of this file need to contain the following: | |
This file needs to contain the following: |
src/toolchain/web-guide.md
Outdated
[target.wasm32-unknown-emscripten] | ||
rustflags = [ | ||
"-C", "link-args=-sSIDE_MODULE=2", | ||
"-C", "link-args=-sUSE_PTHREADS=1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the new flag already supported on our suggested minimum version?
- If yes, I don't see any reason to keep the deprecated one around.
- If no, we should use the deprecated one with a comment, that in newer Emscripten versions, a different flag is used.
src/toolchain/export-web.md
Outdated
web.debug.wasm32 = "res://../rust/target/wasm32-unknown-emscripten/debug/EXTENSIONNAME.wasm" | ||
web.release.wasm32 = "res://../rust/target/wasm32-unknown-emscripten/release/EXTENSIONNAME.wasm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web.debug.wasm32 = "res://../rust/target/wasm32-unknown-emscripten/debug/EXTENSIONNAME.wasm" | |
web.release.wasm32 = "res://../rust/target/wasm32-unknown-emscripten/release/EXTENSIONNAME.wasm" | |
... | |
[libraries] | |
... | |
web.debug.wasm32 = "res://../rust/target/wasm32-unknown-emscripten/debug/EXTENSIONNAME.wasm" | |
web.release.wasm32 = "res://../rust/target/wasm32-unknown-emscripten/release/EXTENSIONNAME.wasm" |
Also, would you need to replace EXTENSIONNAME
with {YourCrate}
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the flag! Actually didn't notice that I had an older one on my personal config file.
src/toolchain/export-web.md
Outdated
This file will probably be at `godot/{YourCrate}.gdextension`. | ||
The format will be similar to the following: | ||
|
||
```gdextension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ```ini here would at least provide some syntax highlighting.
Add a web export in the Godot Editor. In the top menu bar, go to `Project > Export...` and configure it there. | ||
Make sure to turn on the `Extensions Support` checkbox. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention to download export templates if they are not yet configured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you get a prompt if it's missing to download it, and should be downloaded at the same time you'd download templates for any other export. I'd have to intentionally trigger that and see about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the last time I tried it in my VM, I believe the Export window displays some red text at the bottom suggesting you need Export Templates, but I think you still need to click some buttons to prompt the download. Could be useful to list the steps needed, but very briefly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It's fine to not explicitly mention it then.
Add a web export in the Godot Editor. In the top menu bar, go to `Project > Export...` and configure it there. | ||
Make sure to turn on the `Extensions Support` checkbox. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It's fine to not explicitly mention it then.
src/toolchain/export-web.md
Outdated
[target.wasm32-unknown-emscripten] | ||
rustflags = [ | ||
"-C", "link-args=-sSIDE_MODULE=2", | ||
"-C", "link-args=-pthread", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing this!
Sorry I wasn't very clear before, maybe I'd still mention the old one just in case someone is used to that (otherwise they might wonder where it is).
That is:
"-C", "link-args=-pthread", | |
"-C", "link-args=-pthread", # was -sUSE_PTHREADS=1 in earlier versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What part not to mention? Sorry. If you mean line 112 then... that's kinda confusing. I can remove the images about the error for missing templates, I guess. They don't really add much besides push the page length; it feels like anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just apply my suggestion here, so that people familiar with the old flag aren't confused 🙂
I can remove the images about the error for missing templates, I guess. They don't really add much besides push the page length; it feels like anyway.
Yeah, would be good to focus more on the WASM topic, you can remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I marked some pending comments, so they're not overlooked.
After the web-missing-template.png is removed, there's still web-export.png which is 168 kB. Do you think it's possible to reduce the size without losing quality?
src/toolchain/export-web.md
Outdated
--- | ||
|
||
|
||
## Installing Pre-Requisites |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
src/toolchain/export-web.md
Outdated
It is likely to due a bug, but is still being investigated. Edit the line to something like the following: | ||
|
||
```toml | ||
godot = { git = "https://github.com/godot-rust/gdext", branch = "master", features = ["experimental-wasm", "lazy-function-tables"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
src/toolchain/export-web.md
Outdated
godot = { git = "https://github.com/godot-rust/gdext", branch = "master", features = ["experimental-wasm", "lazy-function-tables"] } | ||
``` | ||
|
||
If you do not already have `.cargo/config.toml` file, do the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
src/toolchain/export-web.md
Outdated
[target.wasm32-unknown-emscripten] | ||
rustflags = [ | ||
"-C", "link-args=-sSIDE_MODULE=2", | ||
"-C", "link-args=-pthread", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just apply my suggestion here, so that people familiar with the old flag aren't confused 🙂
I can remove the images about the error for missing templates, I guess. They don't really add much besides push the page length; it feels like anyway.
Yeah, would be good to focus more on the WASM topic, you can remove them.
src/toolchain/export-web.md
Outdated
[this extension](https://chromewebstore.google.com/detail/cc++-devtools-support-dwa/pdcpmagijalfljmkmjngeonclgbbannb?pli=1) | ||
for Chrome. It adds support for breakpoints and a memory viewer into the F12 menu. | ||
|
||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still a horizontal line 😉
## Debugging | ||
|
||
Currently the only option for WASM debugging is | ||
[this extension](https://chromewebstore.google.com/detail/cc++-devtools-support-dwa/pdcpmagijalfljmkmjngeonclgbbannb?pli=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add the name of the extension in the link text, and use the [<name of extension>][wasm-debugger]
format (i.e. move the actual link to the end of the section)?
Addressed the last few remarks, reduced PNG size and linked to Godot export tutorial in place of troubleshooting images. Thanks a lot for this great addition and everyone that contributed to it, especially @Esption for enduring the tough review! 🚀 |
Not sure about the markdown formatting and such, but otherwise I'm happy with this. Hoping to get something out there to start some discussion/suggestions.