Scirra
"Wait for actions" considers out of scope timeline-based timers
Problem description
Adding "Play Timeline" or "Play Tween" actions inside a synchronous function forces it to become asynchronous. This doesn't happen with "Wait" or "Wait for signal" etc.
Haven't find any info about this, so supposed it's a bug.
Attach a .c3p
https://drive.google.com/file/d/1o0e1mkl0DIgc-5U05emft5O1ul3nXQrO/view?usp=sharing
Steps to reproduce
- Play preview and open console
- Timer shows 3 sec after sync function called
Observed result
"Wait for prevoius actions" is waiting for synchronous function to be completed.
Expected result
"Wait for prevoius actions" triggers immideatly.
More details
Affected browsers/platforms: Chrome, Firefox
First affected release: from start i guess
System details
View details
Platform information Browser: Chrome Browser version: 96.0.4664.45 Browser engine: Chromium Context: webapp Operating system: Windows Operating system version: 10 Device type: desktop Device pixel ratio: 1 Logical CPU cores: 4 Approx. device memory: 8 GB User agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.45 Safari/537.36 C3 release: r272.2 (beta) Language setting: en-US
Local storage Storage quota (approx): 67 gb Storage usage (approx): 179 mb (0.3%) Persistant storage: No
Browser support notes This list contains missing features that are not required, but could improve performance or user experience if supported.
Nothing is missing. Everything is OK! WebGL information Version string: WebGL 2.0 (OpenGL ES 3.0 Chromium) Numeric version: 2 Supports NPOT textures: yes Supports GPU profiling: yes Supports highp precision: yes Vendor: Google Inc. (NVIDIA) Renderer: ANGLE (NVIDIA, NVIDIA GeForce GTX 1070 Direct3D11 vs_5_0 ps_5_0, D3D11-30.0.14.9676) Major performance caveat: no Maximum texture size: 16384 Point size range: 1 to 1024 Extensions:
EXT_color_buffer_float EXT_color_buffer_half_float EXT_disjoint_timer_query_webgl2 EXT_float_blend EXT_texture_compression_bptc EXT_texture_compression_rgtc EXT_texture_filter_anisotropic EXT_texture_norm16 KHR_parallel_shader_compile OES_texture_float_linear WEBGL_compressed_texture_s3tc WEBGL_compressed_texture_s3tc_srgb WEBGL_debug_renderer_info WEBGL_debug_shaders WEBGL_lose_context WEBGL_multi_draw OVR_multiview2 Audio information System sample rate: 48000 Hz Output channels: 2 Output interpretation: speakers Supported decode formats:
WebM Opus (audio/webm; codecs=opus) Ogg Opus (audio/ogg; codecs=opus) WebM Vorbis (audio/webm; codecs=vorbis) Ogg Vorbis (audio/ogg; codecs=vorbis) MPEG-4 AAC (audio/mp4; codecs=mp4a.40.5) MP3 (audio/mpeg) FLAC (audio/flac) PCM WAV (audio/wav; codecs=1) Supported encode formats:
WebM Opus (audio/webm; codecs=opus) Video information Supported decode formats:
WebM AV1 (video/webm; codecs=av01.0.00M.08) MP4 AV1 (video/mp4; codecs=av01.0.00M.08) WebM VP9 (video/webm; codecs=vp9) WebM VP8 (video/webm; codecs=vp8) Ogg Theora (video/ogg; codecs=theora) H.264 (video/mp4; codecs=avc1.42E01E) Supported encode formats:
WebM VP9 (video/webm; codecs=vp9) WebM VP8 (video/webm; codecs=vp8)
Oh no, that repro is not the most minimal i found earlier, it's an older one when i was trying to figure it out. There's no need in intermediary async function, just calling sync function does the same
https://drive.google.com/file/d/1ovAnAdi1xKr0e25e3yJHnkJb3maEGkad/view?usp=sharing
I figured out that it's not a function that becomes asynchronous, but timeline itself violates scopes. It's like becoming global for a caller event or something. It should not work like this, right?
https://drive.google.com/file/d/1pZAIVRL8KZOtzxghT346OFfMrlEIo4l9/view?usp=sharing
I think this is more a question of the design of "Wait for previous actions to complete". It's actually designed to be a catch-all that really does "wait for all async actions run since the previous top-level event to complete". In other words every single async action adds to a queue, no matter where they run, and "Wait for previous actions to complete" waits for all of them; and the queue is cleared at every top-level event.
The reason for this approach is (IIRC) for events like this to work more intuitively:
In this case I think it is right that "Wait for previous actions to complete" does wait for the timeline to finish playing - even though the action is in a sub-event, and so is in a different scope to the wait action.
This then also extends to function calls, so even if the timeline action is refactored to some other function, it still works.
I guess the question is: should it still work? This might also be a backwards compatibility issue if many projects are already built on the assumption it works.
@AshleyScirra
In my opinion the case you provided is not intuitive enough. I think the most important role for sub-events is managing scopes, and this design conficlts with what "Wait for previous actions" actually does.
I have a soultion to this, which is also solving compability problem. What if user get a choice what kind of "Wait" he need? For example a droplist with two options: "Global" and "Local/Scope". By default it will be set to "Global" so no existing project will break.
Of course I could be wrong and this design suggestion doesn't make sense, but when the whole logic of project is built in waiting actions, it's obviously becoming frustrating to make sure that these actions are not waiting for something unneccesary
This is a severe bug imo, this can lead to nearly impossible to track down issues, as shown in this example. This can lead to stuff being awaited from completely unexpected places. (yes it does wait 2 seconds instead of 0.1)
project from the screenshot: wait.zip
I think that in this case, 'Wait for previous actions to complete' should wait for the async action to complete:
However it may be difficult for the engine to tell that case apart from this case:
I'll look in to the feasibility of that, but you could interpret that as "wait for previous actions to complete" waiting for all the preceding actions, and so it would be correct it waits for the very indented tween action. However even with the current design, this is easy to work around: place 'Wait for previous actions to complete' after the very indented tween action, and that separates the wait for that action from the wait for the later action.
That workaround also applies to functions: placing 'Wait for previous actions to complete' at the end of the function in the test case from #8273 also solves the problem. However I think it is clearer that calling a non-async function and then using 'Wait for previous actions to complete' should not wait on anything that happened inside the non-async function, so that ought to be changed. I think that also has less backwards-compatibility implications to changing the former case - I can imagine a lot of projects out there either intentionally or accidentally rely on 'Wait for previous actions to complete' waiting for prior actions in such event sheets.
My take is that even in the first example you gave it should not wait for the timeline. Sub-events should not suddenly break their scope like that and this design does more harm than good imo.
This reads to me as the equivalent of something like this, which just seems wrong:
{
let myVariable = "scoped"
};
myVariable = "breakingTheLaw"
If backwards compat is a large concern maybe do it after next LTS
The problem is that approach will also break cases like this:
In that case, the async action is in a different sub-event branch, and under your proposal that should count as a separate scope and 'Wait for previous actions' should not wait for it. However I think it is both likely that people will assume this works as it currently does, and that existing projects do this and will be broken by the change (and I don't think LTS releases are themselves a justification to make potentially disruptive breaking changes). This could probably be dealt with by a second mode e.g. "Wait for previous actions in same sub-event scope", but now we're talking about a feature request so that should be dealt with separately - and it does appear to be easy to work around by adding an extra 'Wait for previous actions' as I mentioned before.
I think the only obvious thing to do is make sure 'Wait for previous actions' does not wait for async actions in non-async function calls. which is what the originally filed issue is about anyway. That is done for the next beta. Even that might have backwards compatibility implications but we'll see how it pans out in beta releases.
I mean construct does follow this strict scoping, i.e. with picking or local variables etc. This tween/timeline case is an outlier that breaks this rule in a counter intuitive way. So the argument that cases like that should work make absolutely no sense to me.
still, thank you for fixing the issue with the functions!
Yes, they use different patterns - but I don't think it's obvious that the 'Wait for previous actions to complete' feature should use the same rules as local variable scoping, especially as it breaks cases that look like they ought to work. It uses a pattern closer to this (which is actually a common pattern we use in Construct's code):
const promises = [];
if (foo)
{
promises.push(DoAsyncThing1());
if (bar)
{
promises.push(DoAsyncThing2());
}
}
promises.push(DoAsyncThing3());
// Wait for previous async work to complete
await Promise.all(promises);