E2E tests broken.

[Mugen87]

Description

Since about two weeks the E2E testing as a part of the CI is broken. Jobs fails with the following error:

Run actions/upload-artifact@6546280
With the provided path, there will be 153 files uploaded
Artifact name is valid!
Root directory input is valid!
Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run

AFAIK, we have not recently changed the CI settings apart from the updates renovate performs.

[Mugen87]

BTW: This is one of the first PRs with broken E2E tests: #28226

[ycw]

#28229 last week
#28175 2 weeks ago <<< culprit
#27730 3 months ago

uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4 (last week, NOT OK, #28229)
uses: actions/upload-artifact@1746f4ab65b179e0ea60a494b83293b640dd5bba # v4 (2weeks ago, NOT OK, #28175)
uses: actions/upload-artifact@5d5d22a31266ced268874388b861e4b58bb5c2f3 # v4 (3months ago, STILL OK, #27730)

[Mugen87]

I'll revert #28229 and see if it helps.

[ycw]

try revert #28175 ?

[Mugen87]

Done! But I believe existing PRs must rebase otherwise they don't get the updated configuration file.

[ycw]

still no luck https://github.com/mrdoob/three.js/actions/runs/8972633808/job/24641039564?pr=28287 :(

maybe related: actions/upload-artifact#506

[Mugen87]

So it seems we have to apply a migration task so v4 of actions/upload-artifact works. I'll give it a try:

37e1cb5

[Mugen87]

Um, it seems the upload works again but I can't regenerate the example screenshots correctly. I've tried it on two systems (macOS and Windows) but many examples still fail during the CI. Because of the migration to setAnimationLoop(), some examples require new screenshots.

@ycw Would you mind helping and file a PR like #28301? Meaning run npm run make-screenshot locally and add the new screenshots to a commit? Maybe it works on your system...

[ycw]

all or some? there're only 70 images in #28301, what are they?

[Mugen87]

Try updating all by simply running npm run make-screenshot. I've tried updating a subset first but when realizing even the new ones still fail I've stopped.

[ycw]

ok, lets do it

[ycw]

done.

[ycw]

I notice that all animation-related screenshots made in #28306 lags behind the old ones by few frames

and this one is totally blank: examples/screenshots/webgl_animation_skinning_additive_blending.jpg

🤔

[Mugen87]

see #28306 (comment).

When using setAnimationLoop(), the scene is not immediately rendered compared to the previous call of animate(). I suspect the E2E tests are not fully compatible with the new approach.

If the timing of the tests would be 100% deterministic, screenshot generation should always produce the same result (maybe with very minor diff).

[RemusMar]

When using setAnimationLoop(), the scene is not immediately rendered compared to previous call of animate().

It's even worse.
For the first 100-200ms all the scene animations are out of sync.
They start with very high speed and reach the correct 60FPS after 100-200ms.
I've updated all my samples to use setAnimationLoop() but I dislike this behaviour.

[Mugen87]

@RemusMar Can you try to compute a delta time not via Clock but like so:

let prevTime = 0;

function animate( time ) {

    const delta = ( time - prevTime ) / 1000;
    prevTime = time;

Would this solve the problem on your side?

[RemusMar]

Michael,
I get the same behaviour with that change.
The problem here is not that delta, but how many times per second do we access the animate function.
And the problem is not on my side.
It's for 99% of the users, but probably they are not aware of this issue (or they don't care).

[RemusMar]

To summarize:
The browser/monitor are always in sync when using requestAnimationFrame( animate );
But they are not in sync for the first 100-200ms when using renderer.setAnimationLoop( animate );

[ycw]

But they are not in sync for the first 100-200ms when using renderer.setAnimationLoop( animate );

Expected because first animate frame is ... scheduled ... by setAnimationLoop(), whereas animate() renders first animation frame immediately, i.e. after merging PRs that replace animate() by setAnimationLoop(), screenshots should all be re-generated.

[RemusMar]

Expected because first animate frame is ... scheduled ... by setAnimationLoop()

That's true.
And here we need to find a solution or workaround.

[Mugen87]

Instead of directly rendering the first frame, the engine let the browser decide when to schedule the first one. If apps need a different behavior, they can always call their animate function right away after the init routine. That would be the workaround you are asking for.

[RemusMar]

Instead of directly rendering the first frame, the engine let the browser decide when to schedule the first one. If apps need a different behavior, they can always call their animate function right away after the init routine. That would be the workaround you are asking for.

In my opinion the current logic for renderer.setAnimationLoop( animate ) is not correct.
And you cannot expect to modify thousands of samples just to correct that.

[Mugen87]

Please do not hijack this issue to discuss the logic of setAnimationLoop(). We know how the method works and how it differs to the previous approach. This issue is about fixing the E2E tests.

Besides, using setAnimationLoop() is not mandatory, you can keep managing the animation loop on app level if you want. However, as soon as you switch to WebGPU some day, a more async startup behavior will be inevitable.

[RemusMar]

This issue is about fixing the E2E tests.

It's much more than that Michael.
Anyway, I won't "bother" you anymore.
good luck

[trusktr]

If the timing of the tests would be 100% deterministic

This is easy to do by replacing window.requestAnimationFrame with a custom one along with a new nextFrame(time) function that ticks it.

[mrdoob]

@Mugen87 I think it may be best to revert all the setAnimationLoop() PRs... 😕

[Mugen87]

And what do we do when we start migrating the examples to WebGPURenderer? Besides, should we remove the existing usage of setAnimationLoop() in the WebGPU demos as well?

To me, letting the renderer handle the animation loop is better than what we had before. It opens up interesting options like automatic time computations or node related internal update mechanism. We essentially loose that if we ask the app to manage the animation loop again. Since users often copy/paste example code, it would be good to promote setAnimationLoop() whenever possible.

If we can't fix the E2E tests, would it also be an option to remove E2E testing from the CI? The implementation of E2E testing is complex and the usage often fragile (false-positives). Notice the large exception list of demos that have never worked. That said, we could still use a strip-down version of the logic to generate screenshots for the website.

[Mugen87]

In any event, I've filed #28322 to see if it solves the E2E issues.