Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Skwasm scene #40330
Skwasm scene #40330
Changes from 22 commits
cfccaa7294cff96dc514413816e4ab7e9376184becfd915b3aade26bd49f73dc473192bbd206bb651d656fbb8d04796b64d79c1438350e48d8bfb0ff7a22dbad6427817a8e6cc89584cd4462024ca56364f85df1a722c25b4f98fca5d798d1cb35146cb0f1232299b2c2ca50c64d5728a2392663d90da0da3be30e0892d607f6f750d94512cd5607685674bc457059fa0dbca2a6f8d1e26a54c06a410702d52b90File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flutter tool calls this
--start-pausedwhich I think is a more self-explanatory name and keeps names consistent across repos/tools.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is probably a better name. One detail is that this option also causes us to run in a window instead of headless, but that probably is fine to just leave implied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's how it works in the flutter tool too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is being called from a synchronous method:
engine/lib/web_ui/lib/src/engine/platform_dispatcher.dart
Lines 708 to 710 in 2e5cdea
I don't think we should change the signature of
Renderer.renderScene. Sinceskwasmis the only one that needs the asynchronicity, let's only changeSkwasmRenderer.renderScene's signature? And I would argue that evenSkwasmRenderer.renderScenedoesn't need to be asynchronous. Nothing is awaiting it, so effectively, we are firing the future and forgetting about it.One concern that I have here though is race conditions. E.g. what if the previous
renderScenecall takes a while and we receive a new one? Should we somehow force these calls to go in sequence (by pushing them into a queue for example)? Again, if this is something we plan to handle later, let's add a TODO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fine for this to be called and not awaited from a synchronous method in a "fire and forget" manner, and that's what is still going on right now. Making this method async at the renderer level only makes things a bit more expressive and flexible. The existing renderers which do not asynchronously render can return an immediately resolved future.
The main impetus behind doing this is because the new golden screenshot capabilities in the golden tests need to wait for the rendering to be actually complete before attempting a screenshot. So really, this future is only being awaited in tests.
As for your concern in the last paragraph, I have a lot of the same thoughts. That's sort of the reason why I'd like to expose this as an async method. In situations where the render thread is slower than our target FPS, we may want to keep track of pending futures and stop actually submitting new frames if it starts getting backed up. Otherwise, if we continue pushing more frames, the render thread will get continually more backed up, when it's better to to just drop frames. We can't do this with our current "fire-and-forget" method, but making the method async opens us up to doing this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with Mouad, this should be synchronous for consistency with the mobile Flutter engine and our public facing API. The Skwasm renderer can have a way to
awaitfor the scene to actually be rendered which can be used in tests. As for the problem with the render thread getting backed up, this is already something that can happen in Flutter mobile and I think they just drop everything but the latest frame.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a suggested alternative? This
Renderer.renderSceneAPI is web-specific, so there is nothing in the mobile Flutter engine to keep it consistent with, and I'm also not changing the public facing API (PlatformDispatcher.renderis still a synchronous method). Basically the reason I want to return a Future here is:A
Futurereturned directly from the method seems like the most natural way to achieve this, but if there is something different you all have in mind I'm open to suggestions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I could have
Renderer.renderScenereturnFutureOr<void>, so that the subclasses ofRendererthat aren't asynchronous can still just returnvoid?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should've clarified my motivation for keeping things as
syncas possible. It's because I want future maintainers to be aware that this is used as a sync API and it's better to stay that way. And if they really need the asynchronicity, then they can make the deliberate decision of converting it to async.If we make all of them async today, I'm afraid it becomes too easy in the future to add async stuff in there without thinking about the consequences. I want to conversion to async to be a deliberate decision based on a real need (like in the case of skwasm).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the
FutureOr<void>change not address your issue?SkwasmRenderer's render will returnFuture<void>, but the other renderers still returnvoid. So it would require us to explicitly change a renderer's return value to make it asyncThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't address the issue. The developer making change to
renderScenewill think it's okay to add anawaitin there because the signature already "supports" async. I want them to have the extra burden of changing the signature, which will make them look around and check whether it's fine to convert it to async or not.That said, I don't feel strongly about it, and maybe this is all just me doing premature design optimizations :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, if a developer is editing the
renderScenefunction on one of the existing renderers (whoserenderScenemethod is synchronous), and they want to add an await, they will have the extra burden of changing the signature, since the signature of the existing renderers is stayingvoid renderScene(...). They would have to explicitly change it toFuture<void> renderScene(...). The abstractRendererclass has the signatureFutureOr<void> renderScene(...), but the actual concrete renderer classes themselves either explicitly returnvoidor explicitly returnFuture<void>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh okay got it now. I misunderstood your previous comment, thought you were making subclasses
FutureOr<void>too (should've looked more closely at the code 🤦♂️)Ok, this actually addresses my concerns, thanks!