godot icon indicating copy to clipboard operation
godot copied to clipboard

Add sample playback support

Open adamscott opened this issue 1 year ago • 7 comments

⚠️ Here be dragons ⚠️

When this PR will be merged for 4.3, this feature will be marked as experimental and could change in 4.4.

tl;dr

This PR adds (back) the concept of samples to the Godot Engine. Currently, the PR enables only the web platform to play samples.

It principally fixes #87329, as that issue would plague any non-threaded web releases with crackling audio.

Example

Single-threaded web example using streams
(old way)
Single-threaded web example using samples
(this PR)
https://adamscott.github.io/2d-platformer-demo-main-thread/ https://adamscott.github.io/2d-platformer-demo-main-thread-samples/

Introduction

Godot uses streaming to mix game audio. Each active stream is registered and then the engine mix on-the-fly the needed audio frames together to output audio based on the audio latency parameter. It works very well on modern platforms.

Samples are another way to handle sound instead of mixing streams. Instead of handling mixing sound and music by the game processes, it relies on off-loading it to the host system. While it doesn't permit full access to the mixing apparatus, it's super useful on systems that don't have a lot of processing power.

To use samples, you register a sample, and then tell the system to play it when needed. And to stop it. It's like a music player, you set the file, then you click on play. You don't control how the software do it, but you know it does.

Godot used to have samples back in Godot 1 and 2, especially to support platforms like the PSP, and the web (thanks to the Web Audio API).

As newer console platforms let developers handle their own mixing logic and that SharedArrayBuffers were introduced in browsers (permitting WebWorkers (web threads) to share memory with the game), samples support was dropped from Godot. Everything was fine.

Anyway, the implementation was somewhat lacking. You had to specifically want to play samples, you couldn't use common nodes to play both streams and samples.

The problem

But on the web platform, Spectre and Meltdown happened. And it completely changed where SharedArrayBuffers were able to be used. Enter "cross-origin isolated" websites, where it's impossible to contact other websites or display ads, and complicating hosting of simple games, greatly reducing the appeal for our web builds.

Hence the work on #85939 in order to compile Godot to run on the main thread. This enables exporting Godot games on the browser without having to cross-origin isolate your website. Unfortunately, this brought an unexpected issue: software mixing is pretty much incompatible with single-threaded games. Especially running on older/less powerful hardware.

Wanna hear for yourself? Try the single-threaded platformer demo (without this PR applied) on your phone or on a computer that doesn't have a great CPU.

The investigation

My colleague @Faless and I considered every solution imaginable: augmenting latency for the web, traced the processes on the web and on mobile and refactor the AudioWorklet processing the audio. But alas. Nothing substantial could have been done.

The only solution we found was to resort to Web Audio samples.

And it's not uncommon for web game engines. We were the uncommon ones not using web audio samples. So, a few weeks ago, I began work on this PR.

The sole requirement: seamlessness

My main focus was to reuse as possible as many features that already exist. It means that in order to play samples, I wanted the UX to keep as close as possible to existing tools.

Godot strives itself to offer the same experience for every target that it exports to. Imagine making the developer choose between having samples for the web export and streaming nodes for the rest. And having to manually add or remove nodes based on the platform with scripts.

This has such a big impact that it's a clear no go for us. We don't want that poor UX.

The solution

My solution is a big hack. (But it does works wonderfully.)

The idea is to reuse all the existing stream nodes and systems. And make the stream elements capable of producing samples.

This story begins with the new project setting audio/general/default_playback_type (hidden currently in the advanced options). Usually, it should stay with the value "Stream", as normally, that's how Godot works currently. But the magic happens with audio/general/default_playback_type.web set as "Sample".

That's because AudioStreamPlayer, AudioStreamPlayer2D and AudioStreamPlayer3D now have a new property called playback_type, which is set by default to... "Default". That's where the magic happens! On standard exports, the nodes will be defined as "Stream", but on web exports, "Sample" will be used instead!

The magic operates behind the scenes though.

The man behind the curtain

Essentially, when a stream is considered a "sample", it doesn't get mixed at all in the mixing phase. Instead, it relies on callbacks by the StreamPlayer nodes.

The StreamPlayer nodes, when their play() method is called, are calling internally AudioServer::start_sample_playback(). All the AudioServer does is to call AudioDriver::start_sample_playback(). If the driver doesn't implement that function, it just doesn't play any sound. But if it does, the driver can now tell the backend to play that sound.

The same thing happens for stop, pause, etc. You can even update the sample, like when the position of the node changes!

Isn't this fascinating?

Registering samples

Before playing the samples, it's important to register them first.

If played without previous registration, the player will make sure to register it first. Though, it's recommended to register manually streams. That's because, on single threaded games, memory transfer is synchronous, so it may make your game stutter. You register a stream as a sample by calling this method:

# optional step
const my_stream_resource = preload("res://assets/my_stream.wav")

AudioServer.register_stream_as_sample(my_stream_resource)

Under the hood, Godot will call the mix() method of the stream playback for the entire duration of the clip. This makes it so that it's possible to play any type of sound media that Godot supports (WAV, mp3, ogg vorbis).

Is it really seamless, though?

These demos were exported to the web (single-threaded) using samples without ever touching the project nodes, resources, nor files.

Demo title Playable link Source
2D Platformer Play Link
Dodge the Creeps Play Link
3D Platformer Play Link
Hell of Mirrors Play Link
Catburglar Play Link

Bugs yet to fix before merge

  • [x] ~~Buses don't chain properly~~
  • [ ] Only forward loop is supported (fix may not make it to the final release)

Known limitations

  • Effects don't apply (will certainly not be part of the initial release and GDScript based effects cannot be used)

Technical diagrams

Registering and playing samples

sequenceDiagram
    participant Script
    participant AudioStreamPlayer2D
    participant AudioStreamPlayerInternal
    participant AudioServer
    participant AudioDriverWeb
    participant JavaScriptAudioLibrary

    Script ->> AudioStreamPlayer2D: set_stream(Ref<AudioStream>)
    AudioStreamPlayer2D ->> AudioStreamPlayerInternal: set_stream(Ref<AudioStream>)

    Script ->> AudioServer: register_stream_as_sample(Ref<AudioStream>)
    activate AudioServer
    AudioServer ->> AudioServer: register_sample(Ref<AudioSample>)
    AudioServer ->> AudioDriverWeb: register_sample(Ref<AudioSample>)
    deactivate AudioServer
    AudioDriverWeb ->> JavaScriptAudioLibrary: godot_audio_sample_register_stream()

    Script ->> AudioStreamPlayer2D: play()
    activate AudioStreamPlayer2D
    AudioStreamPlayer2D ->> AudioStreamPlayerInternal: play_basic()
    activate AudioStreamPlayerInternal
    opt _is_sample() && stream->can_be_sampled() && stream_playback->sample_playback is null
        AudioStreamPlayerInternal ->> AudioServer: is_stream_registered_as_sample(Ref<AudioStream>)
        activate AudioServer
        deactivate AudioServer
        AudioServer ->> AudioStreamPlayerInternal: 
        opt stream is not registered
            AudioStreamPlayerInternal ->> AudioServer: register_stream_as_sample(Ref<AudioStream>)
            activate AudioServer
            AudioServer ->> AudioServer: register_sample(Ref<AudioSample>)
            AudioServer ->> AudioDriverWeb: register_sample(Ref<AudioSample>)
            deactivate AudioServer
            AudioDriverWeb ->> JavaScriptAudioLibrary: godot_audio_sample_register_stream()
        end
        AudioStreamPlayerInternal ->> AudioStreamPlayerInternal: set stream_playback->sample_playback
    end
    AudioStreamPlayerInternal ->> AudioStreamPlayer2D: Ref<AudioStreamPlayback>
    
    deactivate AudioStreamPlayerInternal

    opt stream playback sample exists
        AudioStreamPlayer2D ->> AudioStreamPlayer2D: Update stream_playback->sample_playback for specific AudioStreamPlayer2D stuff
        AudioStreamPlayer2D ->> AudioServer: start_sample_playback(Ref<AudioSamplePlayback>)
        deactivate AudioStreamPlayer2D

        AudioServer ->> AudioDriverWeb: start_sample_playback(Ref<AudioSamplePlayback>)
        AudioDriverWeb ->> JavaScriptAudioLibrary: godot_audio_sample_start()
    end

Samples and streams

classDiagram
    namespace AudioStreams {
        class AudioStream
        class AudioStreamWAV
        class AudioStreamPlayback
        class AudioStreamPlaybackWAV
    }
    namespace AudioSamples {
        class AudioSample
        class AudioSamplePlayback
    }

    %% Inheritance
    AudioStream <|-- AudioStreamWAV
    AudioStreamPlayback <|-- AudioStreamPlaybackWAV

    %% Links
    AudioStream ..|> AudioSample
    AudioStreamWAV ..|> AudioSample

    AudioStreamPlayback ..|> AudioSamplePlayback

    %% Cardinality
    AudioStreamPlaybackWAV "1" --> "many" AudioSamplePlayback

    AudioSample "1" --> "many" AudioStream
    AudioSamplePlayback "1" --> "many" AudioStream

    %% Classes
    class AudioStream {
        +can_be_sampled()*
        +get_sample()
    }

    class AudioStreamWAV {
        +can_be_sampled()
        +get_sample()
    }

    class AudioStreamPlayback {
        +set_is_sample()*
        +get_is_sample()*
        +set_sample_playback()*
        +get_sample_playback()*
    }

    class AudioStreamPlaybackWAV {
        -bool _is_sample
	    -Ref~AudioSamplePlayback~ sample_playback

        +set_is_sample()
        +get_is_sample()
        +set_sample_playback()
        +get_sample_playback()
    }

    class AudioSample {
        +Ref~AudioStream~stream
        +data
        +num_channels
        +sample_rate
        +loop_mode
        +loop_begin
        +loop_end
    }

    class AudioSamplePlayback {
        +Ref~AudioStream~stream
        +float offset
        +float volume_db
        +PositionMode position_mode
        +Vector3 position
        +StringName bus
    }

Fixes

Fixes #87329

adamscott avatar Apr 30 '24 23:04 adamscott

You're missing some class and enum bindings to properly expose the new APIs:

diff --git a/doc/classes/AudioSample.xml b/doc/classes/AudioSample.xml
new file mode 100644
index 0000000000..2d0beece95
--- /dev/null
+++ b/doc/classes/AudioSample.xml
@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<class name="AudioSample" inherits="RefCounted" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd">
+	<brief_description>
+	</brief_description>
+	<description>
+	</description>
+	<tutorials>
+	</tutorials>
+</class>
diff --git a/doc/classes/AudioServer.xml b/doc/classes/AudioServer.xml
index aa00b5b34f..05ed7919d5 100644
--- a/doc/classes/AudioServer.xml
+++ b/doc/classes/AudioServer.xml
@@ -356,5 +356,13 @@
 		<constant name="SPEAKER_SURROUND_71" value="3" enum="SpeakerMode">
 			A 7.1 channel surround setup was detected.
 		</constant>
+		<constant name="PLAYBACK_TYPE_DEFAULT" value="0" enum="PlaybackType">
+		</constant>
+		<constant name="PLAYBACK_TYPE_STREAM" value="1" enum="PlaybackType">
+		</constant>
+		<constant name="PLAYBACK_TYPE_SAMPLE" value="2" enum="PlaybackType">
+		</constant>
+		<constant name="PLAYBACK_TYPE_MAX" value="3" enum="PlaybackType">
+		</constant>
 	</constants>
 </class>
diff --git a/servers/audio_server.cpp b/servers/audio_server.cpp
index cf19e3cca9..ef21de5361 100644
--- a/servers/audio_server.cpp
+++ b/servers/audio_server.cpp
@@ -1906,6 +1906,11 @@ void AudioServer::_bind_methods() {
 	BIND_ENUM_CONSTANT(SPEAKER_SURROUND_31);
 	BIND_ENUM_CONSTANT(SPEAKER_SURROUND_51);
 	BIND_ENUM_CONSTANT(SPEAKER_SURROUND_71);
+
+	BIND_ENUM_CONSTANT(PLAYBACK_TYPE_DEFAULT);
+	BIND_ENUM_CONSTANT(PLAYBACK_TYPE_STREAM);
+	BIND_ENUM_CONSTANT(PLAYBACK_TYPE_SAMPLE);
+	BIND_ENUM_CONSTANT(PLAYBACK_TYPE_MAX);
 }
 
 AudioServer::AudioServer() {
diff --git a/servers/register_server_types.cpp b/servers/register_server_types.cpp
index 932e339eb0..d3446a2dbc 100644
--- a/servers/register_server_types.cpp
+++ b/servers/register_server_types.cpp
@@ -173,6 +173,7 @@ void register_server_types() {
 
 	GDREGISTER_CLASS(AudioStream);
 	GDREGISTER_CLASS(AudioStreamPlayback);
+	GDREGISTER_CLASS(AudioSample);
 	GDREGISTER_CLASS(AudioSamplePlayback);
 	GDREGISTER_VIRTUAL_CLASS(AudioStreamPlaybackResampled);
 	GDREGISTER_CLASS(AudioStreamMicrophone);

akien-mga avatar May 02 '24 12:05 akien-mga

Testing the current version on https://github.com/johncoffee/ngj-2024, it works really well!

On thing I noticed is that volume on the AudioStreamPlayer's doesn't seem to work. That's why if you test that demo, the footstep and attack sounds are much louder on web than on desktop. Is that a limitation of samples, or something that can be fixed? I know we can't do fancy bus effects but volume is quite important for most games IMO.

akien-mga avatar May 02 '24 15:05 akien-mga

Is the configurable stereo separation strength supported as well? We have properties for this on a per-AudioStreamPlayer2D/3D node, as well as manual panning in AudioStreamPlayer and a project setting that acts as a global multiplier for stereo separation.

Calinou avatar May 02 '24 21:05 Calinou

Testing the current version on https://github.com/johncoffee/ngj-2024, it works really well!

On thing I noticed is that volume on the AudioStreamPlayer's doesn't seem to work. That's why if you test that demo, the footstep and attack sounds are much louder on web than on desktop. Is that a limitation of samples, or something that can be fixed? I know we can't do fancy bus effects but volume is quite important for most games IMO.

I'm on the volume issues!

adamscott avatar May 02 '24 23:05 adamscott

as well as manual panning in AudioStreamPlayer

@Calinou How? I was never able to do this.

adamscott avatar May 03 '24 00:05 adamscott

@akien-mga I think I fixed the volume issue.

adamscott avatar May 06 '24 12:05 adamscott

@Calinou How? I was never able to do this.

Nevermind, I thought we had this property in AudioStreamPlayer, but we don't.

Calinou avatar May 06 '24 17:05 Calinou

Remember to add descriptions for newly exposed classes/properties/methods in the class reference.

Yes! I'll add them for sure. Thanks for the reminder.

Also, are there are any advantages to using sample playback on non-web platforms? If so, this should be documented.

Absolutely not. Sample playback for non-supported platforms does nothing, so no sound.

adamscott avatar May 07 '24 11:05 adamscott

Also, are there are any advantages to using sample playback on non-web platforms? If so, this should be documented.

Absolutely not. Sample playback for non-supported platforms does nothing, so no sound.

I'm a bit concerned about this when it comes to the UX of having a new property on audio players. We now have a property on each player, and a global setting, to confirmed whether to use streams or samples. But on any platform other than web choosing anything else than stream would break the project.

Wouldn't it be better to make this an implementation detail that's handled behind the scenes in the web platform, and not expose this at all to the user? Or do we need users to still be able to choose running specific AudioStreamPlayers as streams even on web?

akien-mga avatar May 07 '24 11:05 akien-mga

Or do we need users to still be able to choose running specific AudioStreamPlayers as streams even on web?

For dynamic streams, absolutely, as they are not supported currently as samples. Maybe we could add a ⚠️ emoji if we see that an incompatible setting has been selected?

adamscott avatar May 07 '24 17:05 adamscott

Tested the current PR with https://github.com/gdquest-demos/godot-4-3d-third-person-controller, it seems to break the music playback.

akien-mga avatar May 15 '24 12:05 akien-mga

Tested the current PR with https://github.com/gdquest-demos/godot-4-3d-third-person-controller, it seems to break the music playback.

Added the bug in my list of TODOs before merging: the autoplay property doesn't seem to trigger a play.

adamscott avatar May 15 '24 16:05 adamscott

I've tested all the games mentioned in the OP. The sound worked pretty well for all of them on both FF 126 and Brave 1.66 on Linux. Only Truck Town had a bit of crackling on both, but I think it might just be something with the game itself. And yeah, as mentioned above, the autoplay does not trigger. I had to click once inside the game to start the sound. Everything was pretty smooth on mobile as well.

hakro avatar May 15 '24 17:05 hakro

And yeah, as mentioned above, the autoplay does not trigger. I had to click once inside the game to start the sound.

That's a Web feature (called autoplay blocking)! Sound doesn't play before an interaction event from the user is triggered.

I was rather talking about AudioStreamPlayer's internal "autoplay" feature.

adamscott avatar May 15 '24 19:05 adamscott

I can't compile this branch on my Godot build, it errors at the end with this wall of text:

em++: warning: -pthread + ALLOW_MEMORY_GROWTH may run non-wasm code slowly, see https://github.com/WebAssembly/design/issues/1271 [-Wpthreads-mem-growth]
file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:3567
  var err = new SyntaxError(message);
            ^

SyntaxError: Unexpected token (11023:19)
  	static _samples = new Map();
                   ^

    at Parser.pp$4.raise (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:3567:13)
    at Parser.pp$9.unexpected (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:766:8)
    at Parser.pp$9.expect (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:760:26)
    at Parser.pp$5.parseMethod (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:3321:8)
    at Parser.pp$8.parseClassMethod (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:1527:35)
    at Parser.pp$8.parseClassElement (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:1485:10)
    at Parser.pp$8.parseClass (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:1403:24)
    at Parser.pp$5.parseExprAtom (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:2909:17)
    at Parser.pp$5.parseExprSubscripts (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:2709:19)
    at Parser.pp$5.parseMaybeUnary (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:2675:17) {
  pos: 396985,
  loc: Position { line: 11023, column: 19 },
  raisedAt: 396986
}
em++: error: '/home/mbcx/sources/emsdk/node/16.20.0_64bit/bin/node /home/mbcx/sources/emsdk/upstream/emscripten/tools/acorn-optimizer.mjs /tmp/emscripten_temp_wvhct4ms/godot.web.template_debug.wasm32.js growableHeap --closureFriendly -o /tmp/emscripten_temp_wvhct4ms/godot.web.template_debug.wasm32.jso1.js' failed (returned 1)
scons: *** [bin/godot.web.template_debug.wasm32.js] Error 1
scons: building terminated because of errors.

MBCX avatar May 15 '24 23:05 MBCX

@MBCX Which version of emscripten are you using? I used the latest one, maybe it has an impact.

adamscott avatar May 16 '24 10:05 adamscott

Only Truck Town had a bit of crackling on both, but I think it might just be something with the game itself.

How much FPS do you get when running the project? Stuttering might happen at low FPS because the engine sound pitch only changes once per frame, so having low FPS (below 30 or so) will cause discrete pitch changes to be noticeable. A lot of racing games run into this issue as it's difficult to fix.

Calinou avatar May 16 '24 11:05 Calinou

@MBCX Which version of emscripten are you using? I used the latest one, maybe it has an impact.

@adamscott I am using emscripten 3.1.56, I'm going to update to 3.1.59 and see if that fixes it.

MBCX avatar May 16 '24 12:05 MBCX

@adamscott Nope, even after updating to latest emscripten (3.1.59), it still fails with same errors:

em++: warning: -pthread + ALLOW_MEMORY_GROWTH may run non-wasm code slowly, see https://github.com/WebAssembly/design/issues/1271 [-Wpthreads-mem-growth]
file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:3567
  var err = new SyntaxError(message);
            ^

SyntaxError: Unexpected token (11023:19)
  	static _samples = new Map();
                   ^

    at Parser.pp$4.raise (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:3567:13)
    at Parser.pp$9.unexpected (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:766:8)
    at Parser.pp$9.expect (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:760:26)
    at Parser.pp$5.parseMethod (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:3321:8)
    at Parser.pp$8.parseClassMethod (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:1527:35)
    at Parser.pp$8.parseClassElement (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:1485:10)
    at Parser.pp$8.parseClass (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:1403:24)
    at Parser.pp$5.parseExprAtom (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:2909:17)
    at Parser.pp$5.parseExprSubscripts (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:2709:19)
    at Parser.pp$5.parseMaybeUnary (file:///home/mbcx/sources/emsdk/upstream/emscripten/node_modules/acorn/dist/acorn.mjs:2675:17) {
  pos: 396985,
  loc: Position { line: 11023, column: 19 },
  raisedAt: 396986
}
em++: error: '/home/mbcx/sources/emsdk/node/16.20.0_64bit/bin/node /home/mbcx/sources/emsdk/upstream/emscripten/tools/acorn-optimizer.mjs /tmp/emscripten_temp_wvhct4ms/godot.web.template_debug.wasm32.js growableHeap --closureFriendly -o /tmp/emscripten_temp_wvhct4ms/godot.web.template_debug.wasm32.jso1.js' failed (returned 1)
scons: *** [bin/godot.web.template_debug.wasm32.js] Error 1
scons: building terminated because of errors.
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.59 (0e4c5994eb5b8defd38367a416d0703fd506ad81)
Copyright (C) 2014 the Emscripten authors (see AUTHORS.txt)
This is free and open source software under the MIT license.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

MBCX avatar May 16 '24 12:05 MBCX

@MBCX Could you share the exact build command you're using?

akien-mga avatar May 16 '24 12:05 akien-mga

@MBCX Could you share the exact build command you're using?

@akien-mga I build Godot in a automated manner through a script, this is the command that gets formed before executing:

SCRIPT_AES256_ENCRYPTION_KEY="<key>"

# Debug command (fails)
scons -j4 \ # ${THREADS_PER_JOB}
    platform="web"  precision="single" module_text_server_fb_enabled=yes \ # $common_options
    threads="yes" \ # $thread_mode
    target="template_debug"

# Release command (not tried yet)
scons -j4 \ # ${THREADS_PER_JOB}
    platform="web"  precision="single" module_text_server_fb_enabled=yes \ # $common_options
    threads="yes" \ # $thread_mode
    target="template_release" production="yes" use_closure_compiler="yes"

MBCX avatar May 16 '24 13:05 MBCX

@MBCX It seems related to using the closure compiler. I'll take a look why it has issues with my new JavaScript code.

adamscott avatar May 16 '24 14:05 adamscott

@MBCX It seems related to using the closure compiler. I'll take a look why it has issues with my new JavaScript code.

Well, I haven't tried compiling with closure compiler, compiling template_debug is enough to trigger the bug

MBCX avatar May 16 '24 14:05 MBCX

I can reproduce the issue too with scons p=web threads=yes target=template_debug, I guess you just haven't compiled this specific branch with threads enabled @adamscott (since the whole point is to improve things without threads).

Fixing the style issues from CI would likely also have surfaced this in the CI build :)

akien-mga avatar May 16 '24 14:05 akien-mga

Fixed the issue with closure-compiler. I made the "classes" prototypes instead.

adamscott avatar May 17 '24 12:05 adamscott

When testing, I always get an exception in the console.

Bus.count; -> return GodotAudio.Bus._buses.length; -> GodotAudio.Bus is null?

tmp_js_export.js:10089 Uncaught (in promise) TypeError: Cannot read properties of null (reading '_buses')
    at Function.get (tmp_js_export.js:10089:29)
    at Object.GodotAudioClassesInit [as classesInit] (tmp_js_export.js:10116:8)
    at tmp_js_export.js:14275:16
    at tmp_js_export.js:16297:8

I don't do anything crazy with the audio buses: image

Maran23 avatar May 18 '24 20:05 Maran23

When testing, I always get an exception in the console.

Bus.count; -> return GodotAudio.Bus._buses.length; -> GodotAudio.Bus is null?

tmp_js_export.js:10089 Uncaught (in promise) TypeError: Cannot read properties of null (reading '_buses')
    at Function.get (tmp_js_export.js:10089:29)
    at Object.GodotAudioClassesInit [as classesInit] (tmp_js_export.js:10116:8)
    at tmp_js_export.js:14275:16
    at tmp_js_export.js:16297:8

I don't do anything crazy with the audio buses: image

@Maran23 Can you reproduce the bug with my latest changes? If so, can you join a minimal reproducible project?

adamscott avatar May 30 '24 16:05 adamscott

Is it enforced by CI somehow? E.g. is the type definition used by the Closure Compiler (docs doesn't seem to mention typescript imports support).

And if not, how are going to make sure the type definition actually matches the JS code?

On Sun, Jun 2, 2024, 07:34 Adam Scott @.***> wrote:

@.**** commented on this pull request.

In platform/web/js/libs/library_godot_audio.d.ts https://github.com/godotengine/godot/pull/91382#discussion_r1623330140:

@@ -0,0 +1,158 @@ +export type PositionMode = "none" | "2D" | "3D";

Yes. It adds typing for easier coding. I'll review the code to make sure that it's still relevant.

— Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/pull/91382#discussion_r1623330140, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM4C3XZM5DKLNB3ECJ2IVTZFKVFLAVCNFSM6AAAAABHBFL5QWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJSGIYTQNZYGY . You are receiving this because you were mentioned.Message ID: @.***>

Faless avatar Jun 02 '24 07:06 Faless

As far as I can tell (see https://github.com/jsdoc/jsdoc/issues/1929 and https://github.com/jsdoc/jsdoc/issues/1645 ) it seems that the jsdoc syntax used to import from TS type definition is a VSCode dialect. I'm not sure the CC supports it, but we should be able to provide types using standard JSDoc (or a dialect supported by the CC). See https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler

On Sun, Jun 2, 2024, 09:06 Fabio Alessandrelli < @.***> wrote:

Is it enforced by CI somehow? E.g. is the type definition used by the Closure Compiler (docs doesn't seem to mention typescript imports support).

And if not, how are going to make sure the type definition actually matches the JS code?

On Sun, Jun 2, 2024, 07:34 Adam Scott @.***> wrote:

@.**** commented on this pull request.

In platform/web/js/libs/library_godot_audio.d.ts https://github.com/godotengine/godot/pull/91382#discussion_r1623330140:

@@ -0,0 +1,158 @@ +export type PositionMode = "none" | "2D" | "3D";

Yes. It adds typing for easier coding. I'll review the code to make sure that it's still relevant.

— Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/pull/91382#discussion_r1623330140, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM4C3XZM5DKLNB3ECJ2IVTZFKVFLAVCNFSM6AAAAABHBFL5QWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJSGIYTQNZYGY . You are receiving this because you were mentioned.Message ID: @.***>

Faless avatar Jun 02 '24 09:06 Faless

Is it enforced by CI somehow? E.g. is the type definition used by the Closure Compiler (docs doesn't seem to mention typescript imports support). And if not, how are going to make sure the type definition actually matches the JS code?

I removed the .d.ts file and replaced everything with Closure Compiler compatible JSDoc.

adamscott avatar Jun 02 '24 15:06 adamscott