-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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 4 commits
cfccaa7
294cff9
6dc5144
13816e4
ab7e937
6184bec
fd915b3
aade26b
d49f73d
c473192
bbd206b
b651d65
6fbb8d0
4796b64
d79c143
8350e48
d8bfb0f
f7a22db
ad64278
17a8e6c
c89584c
d446202
4ca5636
4f85df1
a722c25
b4f98fc
a5d798d
1cb3514
6cb0f12
32299b2
c2ca50c
64d5728
a239266
3d90da0
da3be30
e0892d6
07f6f75
0d94512
cd56076
85674bc
457059f
a0dbca2
a6f8d1e
26a54c0
6a41070
2d52b90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -213,5 +213,5 @@ abstract class Renderer { | |||||||
|
||||||||
ui.ParagraphBuilder createParagraphBuilder(ui.ParagraphStyle style); | ||||||||
|
||||||||
void renderScene(ui.Scene scene); | ||||||||
Future<void> renderScene(ui.Scene scene); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 One concern that I have here though is race conditions. E.g. what if the previous There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a suggested alternative? This
A There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I could have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should've clarified my motivation for keeping things as 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 commentThe reason will be displayed to describe this comment to others. Learn more. Does the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't address the issue. The developer making change to 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 commentThe reason will be displayed to describe this comment to others. Learn more. Just to clarify, if a developer is editing the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Ok, this actually addresses my concerns, thanks! |
||||||||
} |
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-paused
which 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.