feat: Option to use toImageSync in ImageComposition class #2593
Conversation
Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
packages/flame/lib/src/parallax.dart
Outdated
| @@ -55,6 +55,7 @@ extension ParallaxExtension on Game { | |||
| ImageRepeat repeat = ImageRepeat.repeatX, | |||
| Alignment alignment = Alignment.bottomLeft, | |||
| LayerFill fill = LayerFill.height, | |||
| bool composeSync = false, | |||
There was a problem hiding this comment.
I don't think we need to expose this option to the user, or what would be the reason for that?
Can we get a runtime crash if using the sync method when it starts rendering and the rasterization isn't done?
There was a problem hiding this comment.
In theory - yes, we can. But I used it everywhere since this feature introduction and never had any problems.
From perfectionists point of view - if we decide to use Sync everywhere, draw exception also should be catched everywhere in render methods, but I'm in doubts how to catch a thing I had never seen and have no idea how to reproduce..
There was a problem hiding this comment.
Let's skip this Boolean and just use sync then. So the sync method is pretty much fire and forget then, if it has no impact on parallelization like you say?
There was a problem hiding this comment.
I haven't noticed that, maybe you have launched my demo projects with alternative collision system recently - there is a lot of toImageSync operations per update, and everything works fine even in browser.
Also Flutter team promizes that everything should be async under hood: https://api.flutter.dev/flutter/dart-ui/Picture/toImageSync.html. I hope we can beleive them :-)
Of course I'd prefer to use more performant implementation instead old async version everywhere.
There was a problem hiding this comment.
Maybe we can switch this boolean to true by default in case if somebody will have an issue with that. After some time without bugreports we could to remove option completely. I see your point that unnecessary code is not good thing, but on other hand I am not a representative enougth, if somebody will have any problems with Sync on specific devices - he could just change the flag in constructor, without needing to fork whole framework to change just one line of code...
There was a problem hiding this comment.
Let's do the other way around instead, let's skip this code for now and if people report bugs regarding it we'll introduce the boolean again.
…age_sync_support' into asgalex/image_composition_to_image_sync_support
packages/flame/lib/src/sprite.dart
Outdated
| /// detailed description of possible benefits in performance. | ||
| Image toImageSync() { | ||
| final composition = ImageComposition() | ||
| ..add(image, Vector2.zero(), source: src); |
There was a problem hiding this comment.
The Vector2.zero should be possible to add as a static on this class instead, so that it doesn't need to be recreated for every call.
Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
…age_sync_support' into asgalex/image_composition_to_image_sync_support
Description
Added
ImageComposition.composeSync()functionChecklist
docsand added dartdoc comments with///.examplesordocs.Breaking Change?
Related Issues
Closes #2315