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

Load engine and data concurrently #8682

Merged
merged 5 commits into from Mar 26, 2024

Conversation

ekharkunov
Copy link
Contributor

@ekharkunov ekharkunov commented Mar 18, 2024

Now dmloader.js loads game data and game engine (wasm + js files) concurrently.

The default template builtins/manifests/web/engine_template.html has been changed. If you use your own custom template, make sure to update it with these changes. Pay attention to css changes for progress bar and how progress bar animation now handling. Also change how extra_params passed according to new documentation https://defold.com/manuals/html5/#extra-parameters

Fixes #7456.
Possible fixes #7039.

Technical changes

Rework html5 loading flow. Now dmloader.js loads archive_list.json, then loads data and engine concurrently.
Additionally part of js code was moved from engine_template.html to dmloader.js.
dmloader.js used as template during bundle process. It means that some of project values can be used to substitute inside template (see dmloader.js for more details).
Add animation transition for progress bar width change to update progress bar more smooth. Made with transform property (thanks to @thinknathan ).

Manual update: #8682

PR checklist

  • Code
    • Add engine and/or editor unit tests.
    • New and changed code follows the overall code style of existing code
    • Add comments where needed
  • Documentation
    • Make sure that API documentation is updated in code comments
    • Make sure that manuals are updated (in github.com/defold/doc)
  • Prepare pull request and affected issue for automatic release notes generator
    • Pull request - Write a message that explains what this pull request does. What was the problem? How was it solved? What are the changes to APIs or the new APIs introduced? This message will be used in the generated release notes. Make sure it is well written and understandable for a user of Defold.
    • Pull request - Write a pull request title that in a sentence summarises what the pull request does. Do not include "Issue-1234 ..." in the title. This text will be used in the generated release notes.
    • Pull request - Link the pull request to the issue(s) it is closing. Use on of the approved closing keywords.
    • Affected issue - Assign the issue to a project. Do not assign the pull request to a project if there is an issue which the pull request closes.
    • Affected issue - Assign the "breaking change" label to the issue if introducing a breaking change.
    • Affected issue - Assign the "skip release notes" is the issue should not be included in the generated release notes.

Example of a well written PR description:

  1. Start with the user facing changes. This will end up in the release notes.
  2. Add one of the GitHub approved closing keywords
  3. Optionally also add the technical changes made. This is information that might help the reviewer. It will not show up in the release notes. Technical changes are identified by a line starting with one of these:
    1. ### Technical changes
    2. Technical changes:
    3. Technical notes:
There was a anomaly in the carbon chroniton propeller, introduced in version 8.10.2. This fix will make sure to reset the phaser collector on application startup.

### Technical changes
* Pay special attention to line 23 of phaser_collector.clj as it contains some interesting optimizations
* The propeller code was not taking into account a negative phase.

@@ -999,14 +999,7 @@ public void buildEnginePlatform(IProgress monitor, File buildDir, File cacheDir,

final String variant = appmanifestOptions.get("baseVariant");

List<String> defaultNames = platform.formatBinaryName("dmengine");
List<File> exes = new ArrayList<File>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used.

@@ -203,7 +215,7 @@ void writeJson(JsonGenerator generator) throws IOException {

generator.writeFieldName("pieces");
generator.writeStartArray();
int offset = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix type because file size returns long.

@@ -257,7 +272,6 @@ public void bundleApplication(Project project, Platform platform, File bundleDir
String enginePrefix = BundleHelper.projectNameToBinaryName(title);

BundleHelper.throwIfCanceled(canceled);
File projectRoot = new File(project.getRootDirectory());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used.

generator.writeEndArray();
generator.writeNumberField("total_size", totalSize);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calculate total game data size to avoid unnecessary calculation during runtime.

xhr.onprogress = function(e) {
if (onprogress) onprogress(xhr, e);
xhr.onprogress = function(event) {
if (onprogress) onprogress(xhr, event, xhr._loadedSize);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pass already loaded amount of bytes to calculate delta (use later to update progress bar).

return;
}
currentAttempt = currentAttempt + 1;
if (onretry) onretry(xhr, event, xhr._loadedSize, currentAttempt);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add onretry callback. It uses to decrease loaded amount of bytes and update progress bar with correct values.

}
request.onprogress = function(xhr, e, ls) {
var delta = e.loaded - ls;
onprogress(delta);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplify onprogress because we don't need to know total size from response headers (see #7039)

wasm_size: 2000000,
wasmjs_size: 250000,
asmjs_size: 4000000,
wasm_size: {{ DEFOLD_WASM_SIZE }},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bake final size of files inside dmloader.

// load engine (asm.js or wasm.js + wasm)
// left as entrypoint for backward capability
// start loading archive_files.json
// after receiving it - start loading engine and data concurrently
load: function(appCanvasId, exeName) {
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 left EngineLoader.load function as entrypoint because it may produce breaking change for developer.

@@ -241,9 +302,6 @@ var GameArchiveLoader = {
this._onGameArchiveLoaderCompletedListeners = [];
this._onAllTargetsBuiltListeners = [];
this._onFileDownloadErrorListeners = [];

this._currentDownloadBytes = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to calculate it separately.

// calculate total download size of all files
for(var i=0; i<this._files.length; ++i) {
this._totalDownloadBytes += this._files[i].size;
var isWASMSupported = Module['isWASMSupported'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main change:
Now it starts loading game data and game engine concurrently after receiving archive_files.json. Here it calculates total amount of bytes that need to be downloaded.

@@ -457,19 +520,6 @@ var Progress = {
progress_id: "defold-progress",
bar_id: "defold-progress-bar",

listeners: [],

addListener: function(callback) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left Progress object only for visualization.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that critical if we break something in templates, because we always can notify users and ask them to update their template (btw could you please a it into your PR message e.g. like here) but it would be nice to keep existing extensions compatible if it's possible. For example:

https://github.com/defold/extension-poki-sdk/blob/main/poki-sdk/manifests/web/engine_template.html

If it's impossible to avoid - sure, we have to notify users and probably introduce it as a breaking change, but here it feels like something only about naming. I suggest to keep Progress for logic and create ProgressView just for you, to keep it back compatible

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 left Progress object as wrapper around new ProgressView and ProgressUpdater. Let me know if such approach is ok. @AGulev

@@ -499,6 +544,50 @@ var Progress = {
}
};

var ProgressUpdater = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduce new object ProgressUpdater. It should control and recalculate downloading progress and notify all listeners.

@@ -856,6 +890,119 @@ var Module = {
},
};

// common engine setup
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of code that was moved from engine_template.html.

@@ -13,6 +13,7 @@
background-color: #1a72eb;
text-align: center;
line-height: 20px;
transition: width 1s ease;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For smoother progress bar width update.


stream_wasm: false,
updateWasmInstantiateProgress: function(totalDownloadedSize) {
EngineLoader.wasm_instantiate_progress = totalDownloadedSize * 0.1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calculate what amount of total downloadable size is 10% and use if as part of progress that indicates wasm instantiating.

… Add dmloader.js creation during bundle process. Cleanup some unused code from Bob.

Rework Progress in dmloader. Add totlaSize field to archive_list.json.
Add onretry calbback. Decrease downloaded amount of bytes if request failed and retried. Add easing for base progress bar width change.
@ekharkunov ekharkunov force-pushed the issue-7456-web-loading-parallel branch from efb37f1 to 53beeb0 Compare March 18, 2024 15:26
@ekharkunov ekharkunov marked this pull request as ready for review March 18, 2024 15:26
@ekharkunov ekharkunov marked this pull request as draft March 18, 2024 17:12
@@ -510,6 +599,7 @@ var Module = {
_archiveLoaded: false,
_preLoadDone: false,
_waitingForArchive: false,
_isEngineLoaded: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduce new flag to control if engine already downloaded or not. It's need to handle case when game data loaded faster than engine binaries.

FileLoader.options.retryInterval = params["retry_time"] * 1000;
if (typeof params["can_not_download_file_callback"] === "function") {
GameArchiveLoader.addFileDownloadErrorListener(params["can_not_download_file_callback"]);
if (Module._archiveLoaded) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If game data already loaded - call main function.

Module.noInitialRun = false;
} else {
Module.callMain(Module.arguments);
if (Module._isEngineLoaded) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check if engine binary loaded and JS runtime is initialize (it means that 'onRuntimeInitialized' was called).

@ekharkunov ekharkunov marked this pull request as ready for review March 18, 2024 22:08
@ekharkunov
Copy link
Contributor Author

Note to reviewers:
In the last commit implement case when game data loaded faster than engine binaries.

};
request.onerror = function(xhr, e) {
onerror("Error loading '" + url + "' (" + e + ")");
};
request.onload = function(xhr, e) {
if (xhr.readyState === 4) {
if (xhr.readyState === XMLHttpRequest.DONE) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use constant instead of magic number.

@@ -457,19 +520,6 @@ var Progress = {
progress_id: "defold-progress",
bar_id: "defold-progress-bar",

listeners: [],

addListener: function(callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that critical if we break something in templates, because we always can notify users and ask them to update their template (btw could you please a it into your PR message e.g. like here) but it would be nice to keep existing extensions compatible if it's possible. For example:

https://github.com/defold/extension-poki-sdk/blob/main/poki-sdk/manifests/web/engine_template.html

If it's impossible to avoid - sure, we have to notify users and probably introduce it as a breaking change, but here it feels like something only about naming. I suggest to keep Progress for logic and create ProgressView just for you, to keep it back compatible

Copy link
Contributor

@AGulev AGulev left a comment

Choose a reason for hiding this comment

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

LGTM!👍

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! 👍

@ekharkunov ekharkunov merged commit f41dffa into defold:dev Mar 26, 2024
1 check passed
@britzl britzl changed the title [HTML5] Load engine and data concurrently Load engine and data concurrently Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants