Skip to content
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

Optimized Watch #318

Closed
Solido opened this issue Sep 23, 2024 · 18 comments · Fixed by #331
Closed

Optimized Watch #318

Solido opened this issue Sep 23, 2024 · 18 comments · Fixed by #331
Labels
enhancement New feature or request

Comments

@Solido
Copy link
Collaborator

Solido commented Sep 23, 2024

It would be a nice addition to have Watch manage child widget to avoid full rebuild.

It's possible with current API but we have to build the child outside and pass it to builder by context.

This can be a costly error for beginner who are unaware of the process.
The moment they start working with Asset like image the whole UI can start to blink.

Having a document Widget? child attribute would suffice to explain what's going on
or and extended constructor around an existing child that would rebuild on signals update.

@rodydavis
Copy link
Owner

rodydavis commented Sep 23, 2024

Oh interesting thought!

Could you share some pseudo code as an example?

The image widget is a good example use case for sure.

@Solido
Copy link
Collaborator Author

Solido commented Sep 23, 2024

Just fall for it this morning. Nothing I can share cause code is not public but the concept is exactly the same than AnimationBuilder where I use expensive ops (think large asset and transform).

I've a dedicated widget that just received a Watch in the Tree and everything start blinking because each update force the reload of asset and transformation is then applied to the whole context.

Want I want is just passing the child that is cached into memory and apply transformation.

As an alternative I can rely on ForegroundPainter but our case with Watch is more ubiquitous and even more dangerous if relying on signal animation capacities.

I wish I can show you what I've done with Signals it's beyond anything else I've done before ^^

@rodydavis
Copy link
Owner

Oh i see! I can add an optional child property to the builder!

Sounds super amazing!

I am also working with painters and transforms with signals, but I usually use the painter + custom multi child delegate:
https://rodydavis.github.io/flow_image_editor/

@Solido
Copy link
Collaborator Author

Solido commented Sep 23, 2024

Yep I'm following your commits, I know it is one of your special objective :)
I think I can share some of the last project with you. Let me move it forward enough.

@Solido
Copy link
Collaborator Author

Solido commented Sep 23, 2024

Ah no I did not saw this one :D
Really COOOL!

Are you aiming for a Blender Geometry Node tool?

You know I'm a big aficionado of coding my photos filter myself?
Even doing some dedicated LUT.

@rodydavis
Copy link
Owner

Thanks! It will be a package that anyone can define custom nodes and the package will handle all the interaction state.

All possible because of signals!

@rodydavis
Copy link
Owner

The flow image editor app I want to open source and allow for contributions to custom nodes and filters!

@Solido
Copy link
Collaborator Author

Solido commented Sep 23, 2024

Not the place to discuss it but if Dart is more than qualified to be the front
it's not the same on applying efficient matrix ops.
When you enter the field of photography, I doubt it can complete any ops on 16 bits Labs in a chain.

To my little hands down experience https://github.com/libvips/libvips is the way to go.
So FFI as a front would be a nice path only if you want a web solution.

Just the 2 cents of a LUT creator ;)

@rodydavis
Copy link
Owner

Oh that's awesome! And yeah I was meaning more for the nodes for creating a pipeline that is not really realtime.

The flow image editor only runs each operation when each dependency changes using a computed so it would I guess run the LUT once and cache it.

But yeah we can chat more offline 😎

@rodydavis rodydavis added the enhancement New feature or request label Oct 18, 2024
@NabilaWorks
Copy link
Contributor

NabilaWorks commented Nov 21, 2024

I looked into this briefly..

For high performance animations / widget caching, Flutter uses a different function signature for the builders in AnimatedBuilder, ListenableBuilder namely :

Widget Function(BuildContext, Widget?) builder

Whereas signals uses :

T Function(BuildContext context) builder

Adding an additional Widget? as part of the normal Watch builder will likely break everyone using signals and require them to migrate to the new signature?

I wonder what the smoothest way to add a child parameter would be in this scenario..

Adding an optional T Function(BuildContext, Widget?) builderWithChild to the Watch widget or maybe introducing a new constructor?

@rodydavis
Copy link
Owner

rodydavis commented Nov 21, 2024

Since we are launching v6 I think it would be a perfect time to change the signature.

I do know it would be a breaking change for anyone using it but we might could offer a codemod?

The alternative would be to add a named constructor and it could be ignored for the others (my preference).

We could also add a new widget called WatchBuilder or something

@Solido
Copy link
Collaborator Author

Solido commented Nov 21, 2024

Break the signature, it won't be much a burden to update the code plus it will align with Flutter. If possible limit the number of constructors/widgets/methods to keep the API surface accessible.

Others state-mngts felt for it and they're too complexe to learn or too specific while Signals may aim to be simple and accessible on first read.

@rodydavis
Copy link
Owner

Sounds good! Will break it for v6. I think it is the right call and especially as we make it easier to work with ValueNotifier and Streams

@rodydavis
Copy link
Owner

rodydavis commented Nov 22, 2024

@Solido
Copy link
Collaborator Author

Solido commented Nov 22, 2024

I'm reading your commits on 6.0!

@rodydavis
Copy link
Owner

Decided on a new path.

Create new widget WatchBuilder that includes the child property on the builder and callback.

Update Watch to use WatchBuilder internally.

This will allow for an easy migration to v6 while in the future being able to deprecate one.

@rodydavis rodydavis linked a pull request Nov 28, 2024 that will close this issue
Merged
@Solido
Copy link
Collaborator Author

Solido commented Nov 28, 2024

After migrating to the testing branch I expressed some code as

Watch((_,__) => ...)

As the second parameter is required even where child is not.
WatchBuilder will simplify that.

@rodydavis
Copy link
Owner

The latest removes that from Watch and only puts it in WatchBuilder!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants