Skip to content

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Oct 14, 2024

Using Promise.all() with current addFile() logic without keeping track of index does not allow us to comply with render list order

This creates various problem, including breaking the promise of render target config controlling the order of rendering.

Creating this as a Draft PR for now to discuss, and see if we can keep the Promise logic for performance while keeping track of the order

Using Promise.all() with current `addFile()` logic without keeping track of index does not allow us to comply with render list order

This creates various problem, including breaking the promise of render target config controlling the order of rendering.
@cderv cderv force-pushed the fix/keep-render-target-order branch from 02c8a40 to f861b0a Compare October 14, 2024 13:36
…n for each file

However, be sure to sort the files in the same order as the input array to insure that render target order is respected
@cderv
Copy link
Collaborator Author

cderv commented Oct 14, 2024

see if we can keep the Promise logic for performance while keeping track of the order

Second commit is modifying addFile() and addDir() logic so that order of render target is respected, while benefiting of asynchronous engine resolution that I believe was part of the purpose of #8787

I am now looking to add test for this

@cderv cderv marked this pull request as ready for review October 14, 2024 14:07
@cderv
Copy link
Collaborator Author

cderv commented Oct 14, 2024

I am now looking to add test for this

Not sure that is a reliable way though... 🤔

[skip ci]
@cscheid
Copy link
Collaborator

cscheid commented Oct 14, 2024

Very cool, thanks for taking this on.

I remember this bit of code very well, because @dragonstyle and I had a large number of attempts to fix this at some point around 1.5.

We definitely need to fix this, but I want to be very careful around it, especially since (as you've noted) this is currently pretty hard to test.

@cscheid
Copy link
Collaborator

cscheid commented Oct 14, 2024

What if we split the logic into two steps, one that enumerates all of the files in the correct order, and another that filters out the files from the original enumeration?

@cderv
Copy link
Collaborator Author

cderv commented Oct 14, 2024

Let's close this in favor of #11062

@cderv cderv closed this Oct 14, 2024
@cderv cderv deleted the fix/keep-render-target-order branch October 18, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants