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

Fix Fill options for non Fixed width options #2018

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fishrockz
Copy link
Collaborator

@fishrockz fishrockz commented Oct 23, 2021

Hi, long time since I last contributed but I have been making a little GUI for something I was working on and notice that the image demo's filling seemed broken.

I have not go my head round the layout function so any feed back on how this should work would be appreciated :)

@fishrockz fishrockz marked this pull request as draft October 23, 2021 15:01
@fishrockz fishrockz force-pushed the willsalmon/fill_not_constrained branch 3 times, most recently from b648f92 to 94ad9c6 Compare October 23, 2021 16:02
@fishrockz fishrockz marked this pull request as ready for review October 25, 2021 21:12
@cmyr
Copy link
Member

cmyr commented Oct 26, 2021

I think that the current design is probably working as intended (I didn't write it) but maybe we should talk about whether or not that intention is correct?

Basically: currently, if the size of the image widget is not explicitly constrained, then we size it to be exactly the same as the underlying image.

For me, a "fill strategy" is only really relevant in a situation where we are fitting an image into a rectangle with different dimensions; if the image and the target dimensions match exactly, then all fill strategies should perform equally?

Another way to think about this: fill strategy is a function of the dimensions of the widget, but it should not influence the dimensions of the widget. In this current implementation, the size of the widget changes (often dramatically) based on the selected fill strategy. I'm not sure this is desirable.

In any case, I'm just trying to express my current understanding, it is very possible that I'm overlooking something and I'd be happy to discuss further!

@fishrockz
Copy link
Collaborator Author

Basically: currently, if the size of the image widget is not explicitly constrained, then we size it to be exactly the same as the underlying image.

So if the fill strategy is scale down then that is exactly what the widget dose, maybe this would be a better default, I'm happy to change that.

The issue i had is that if i just want the image to fill up the space available to the widget, I cant, I can explicitly set the size of the widget but if i just want to have a widget that take up the available space the widget could fill i have to write my own widget.

Another way to think about this: fill strategy is a function of the dimensions of the widget, but it should not influence the dimensions of the widget. In this current implementation, the size of the widget changes (often dramatically) based on the selected fill strategy. I'm not sure this is desirable.

I was thinking of it of how the image fills the space available to the widget. This change will never ask for more than bc.max() its just that the amount of bc.max() that is asked for is a function of https://api.flutter.dev/flutter/painting/BoxFit-class.html IIRC one of the druid team suggested using the flutter options and the druid README suggests that the layout function in the trait is inspired by https://github.com/linebender/druid#layout so I assumed this change made sense..

In my original PR i included a example were one of the images expanded to fill the width #557 as seen Screenshot from 2021-10-26 15-58-48 I am just trying to get back to a place were you can do that will at least one of the options. BTW major kudos to every one who has worked on the example since the current one is much better, and is really nice with all the options and things.

If this is ok I can ether add the svg layout to this PR or have a second as especially for that widget it make a lot of sense for it to fill up.

@JAicewizard
Copy link
Contributor

The image explicitly doesnt scale up to the max size it can take, because if you put the image in a Flex it will take up infinite space. By default the image will take up all available space, as long as you dont give it infinite space. So you should contain the image in either direction so that it takes the available space.

so

The issue i had is that if i just want the image to fill up the space available to the widget, I cant
it not true, the widget will take the available space unless you give it infinite space.

However I dont think the current code takes the fill_mode into account so that might be an improvement.

@fishrockz
Copy link
Collaborator Author

fishrockz commented Oct 29, 2021

However I dont think the current code takes the fill_mode into account so that might be an improvement.

Im happy to help but i don't really get what my responsibly/roll as a widget author is with regard to layout. This PR is my best gues but if you can give me a more detailed spec of what the layout function should do i can update it accordingly.

if you put the image in a Flex it will take up infinite space.

I don't know how flex works but dose it not pass a semi sensible bc.max() ? I have used this value for a widget in a flex::column thing and it seems to work really well and dose not take up infinite space but the amount of space needed to fill the window.

My use case is, buttons at the top that control a simulation and then a gif/image as a output. and I want the image/output to just fill the rest of the window.

@JAicewizard
Copy link
Contributor

There are a couple cases

  • bound in width but not height: take max space without adjusting aspect ratio
  • bound in height but not width: take max space without adjusting aspect ratio
  • unbound in all directions: take image size
  • bound in all directions: take max space without changing aspect ratio

This is what the code currently does, and I am not exactly sure what case you want to change and how. It also seems that your code doesnt retain aspect ratio.

@fishrockz
Copy link
Collaborator Author

fishrockz commented Oct 29, 2021

Ok so:

  • bound in width but not height: take max space without adjusting aspect ratio
  • bound in height but not width: take max space without adjusting aspect ratio
  • bound in all directions: take max space without changing aspect ratio

All just work if you have Contain

And if you want:

  • unbound in all directions: take image size

Then you want Scale down

AFAICT the only time the aspect ratio is not fixed is when using Fill which is what https://api.flutter.dev/flutter/painting/BoxFit-class.html is supposed to do. I think after chatting on zulip @futurepaul and @cmyr #557 (comment) asked me to make the image behave per that spec for the original PR when i first wrote the widget. If it is not at the original aspect ratio when not using Fill could you please let me know how to replicate so i can fix.

The reason that I made this PR is that this widget used to behave like https://api.flutter.dev/flutter/painting/BoxFit-class.html and now it dose not so i thought that was a bug that I can help fix.

This PR dose cater for your use cases, see above. But it also caters for use cases where the user wants to let the image be scaled up but not to a fixed amount. This is my use case and the image widget has regressed for my use case. If you just dont want to support this use case then I can close the PR.

@fishrockz
Copy link
Collaborator Author

fishrockz commented Oct 29, 2021

Here is a example of the sort of UI you could make with the original image widget that you can not do with the current implementation and that this PR fixes
Screenshot from 2021-10-29 18-01-31
.

@JAicewizard
Copy link
Contributor

But it also caters for use cases where the user wants to let the image be scaled up but not to a fixed amount.

What do you mean? To what do you want it to be scaled? to the width of the flex? That should already be possible, thats what it should currently do. (assuming the flex's width is fixed, and you have a column flex. height otherwise.)

@fishrockz
Copy link
Collaborator Author

But it also caters for use cases where the user wants to let the image be scaled up but not to a fixed amount.

What do you mean? To what do you want it to be scaled? to the width of the flex? That should already be possible, thats what it should currently do. (assuming the flex's width is fixed, and you have a column flex. height otherwise.)

I just want the image to scale up to the size of the window. The image example demonstrates it perfectly, untick the 2 constraints. and select something like fill:

  • Old image behaviour: the image fills the hole window apart from the options row (if the old widget was in the current demo).
  • Current behaviour: the image fills the window until the window is bigger than the widget. (the UI is so big that it only deforms in hight as the image is smaller than the UI)
  • This pr restores the behaviour so that the image can grow past its native resolution as the window grows past its size.

Comment on lines 202 to 207
if bc.is_width_bounded() {
width = width.min(max.width);
}
if bc.is_height_bounded() {
height = height.min(max.height);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the strategy to use affine_to_fill is probably good. However this doesnt take into account when both sides are unbounded.

In my if else chain the first 2 should remain the same, since this takes into account being inside unbounded parents. This code should only apply if its bounded on both sides, since affine_to_fill is not made for unbounded constraints. In the last case where all sides are unbounded it should use the bc.constrain(self.image_data.size()) code (Which could be simplified to self.image_data.size() since bc is unbounded).

The reason the code is the way it is now, is so that we dont return an infinite size when unbounded in either direction. Since your code doesnt take into account being unbounded on either side we still need these safeguards using a different implementation.

Copy link
Collaborator Author

@fishrockz fishrockz Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since your code doesnt take into account being unbounded on either side we still need these safeguards using a different implementation.

But this code never returns a unbounded result. and some of the fill strategies call for more than the image. So letting affine_to_fill determine the result dose all the work you are asking for when you want it and other things for the other fill strategies.

self.fill.affine_to_fill(max, image_size) takes the max size the widget may be to not change the window size, this gives it a max to work with. then it also knows the original image size, image_size. so for no scale or scale down the max it asks for the smallest of original image or the space available.

affine_to_fill is doing the guarding its self in the non-constrained case and most of the constrained cases, only for a few edge cases do the two if's every have to do anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at this code and realised I can simplify it without changing the behaviour of the demo at all. Also by having less branching it makes it simpler for the cpu so might be a tiny tiny bit faster.

I have added some comments that might make it easer to under stand as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this code never returns a unbounded result

It does, when the constraints are unbounded the resulting size of the image could be infinitely big. Since there are no checks for this this infinity will traverse it's way through the calculations into the final size.

or the space available.

that's the problem, the space available could be infinite in either direction.

@@ -190,19 +190,22 @@ impl<T: Data> Widget<T> for Image {
bc.debug_check("Image");

// If either the width or height is constrained calculate a value so that the image fits
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the fillstrat to determine the size of the image so that the the image will fit perfectly using this filstrat. If it is unconstrained by both width and height take the size of the image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that some of the fill strats do take the size of the image but other strategies do not so your alternative is self contradicting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see, I mean the size of the underlying image data.

@JAicewizard
Copy link
Contributor

I made some comments, I think I understand the problem now. Thanks for your comments.

Your patch indeed fixes the problems outlined by you, but it doesnt keep working in the problems outlined by me. So I suggested some changes so that it will keep working in cases I had problems with when I changed the code.

@fishrockz
Copy link
Collaborator Author

@JAicewizard and @cmyr i really don't understand "However this doesnt take into account when both sides are unbounded." or "Since your code doesnt take into account being unbounded on either side we still need these safeguards using a different implementation."

Screenshot from 2021-11-01 13-51-35
Screenshot from 2021-11-01 13-51-57

Here are two screen shots of the PR with scale down behaving exactly as i think you want it to behave and the second with my use case not taking up infinite space but instead taking up the space left over in the window. In that use case the image adjusts to fit the window.

If I am still miss understanding can you please give me details of how to recreate the issue?

@fishrockz fishrockz force-pushed the willsalmon/fill_not_constrained branch 2 times, most recently from 853603c to 064f71a Compare November 1, 2021 14:53
@JAicewizard
Copy link
Contributor

JAicewizard commented Nov 1, 2021

Here are two screen shots of the PR with scale down behaving exactly as i think you want it to behave and the second with my use case not taking up infinite space but instead taking up the space left over in the window. In that use case the image adjusts to fit the window.

This is because the examples constrains the image within the window. If you dont do that by using something scroll able it will give errors. You can check if the given bounds are constrained using bc.is_{height/width}_bounded, I am using that literal definition.

See #1189 for fix and issue. (the one where I changed the code to what it is now)

@fishrockz
Copy link
Collaborator Author

Here are two screen shots of the PR with scale down behaving exactly as i think you want it to behave and the second with my use case not taking up infinite space but instead taking up the space left over in the window. In that use case the image adjusts to fit the window.

This is because the examples constrains the image within the window. If you dont do that by using something scroll able it will give errors. You can check if the given bounds are constrained using bc.is_{height/width}_bounded, I am using that literal definition.

See #1189 for fix and issue. (the one where I changed the code to what it is now)

That makes sense, thanks for explaining the case. yes i can see that the original didnt do a good job in that case but this new code will work according to the flutter guidance in that case AFAICT.

I don't know what flutter dose in that use case with the fill strategy but the new code should behave sensibly for Contain, (FitHeight, or FitWidth depending on which scroll bars are used.) and None and ScaleDown,

But the fill strategies Cover Fill and FitHeight FixWidth depending on scroll bars dont really make sense for that use case so it makes sense that they don't work nicely.

I think its a shame to rule out a hole class of UI because some of the fill strats dont' make sense in that use case. If the image gets too far out of AR or toooo big then we could clip them?

Now I finally under stand your use case, i might be able think of something else..

@fishrockz
Copy link
Collaborator Author

This is what i meant by

I have not go my head round the layout function so any feed back on how this should work would be appreciated :)

I have not seen a case were bc.max() did not give a non integer or non sensible value. Even when non contained it still gives sensible values most of the time. it would be easy to check if width == F64::inf and then just fall back to width = image.width() or something like that etc. that would fix your use case and give flexiblity to others.

@cmyr / @JAicewizard are there any other supprising values of bc.max()?

@cmyr
Copy link
Member

cmyr commented Nov 1, 2021

Okay, I wrote a thing here but I think it's actually not the most useful response.

Instead, I want to just point to the flutter docs for BoxFit and FittedBox. There are some other useful links from there.

Some things to note:

  • there is some confusion in the code about the relationship between the box constraints and the fill strategy. When do we just use the box constraints? When do we use the fill strategy? etc.
  • importantly, the fill strategy should not change the size of the widget. It should only effect how the image is fit within the space that has been chosen.
  • One way to think about this: first we pick a size, and then we apply the fill start.
  • in flutter, the fill strat is applied by a distinct widget. It may be useful for us to consider this approach, since it clarifies the relationship between fillstrat and layout? Basically the FittedBox widget first passes through the layout constraints to the child, and then it applies the fillstrat to the result, and generates a transform that is applied to the child. This is a bit complicated for us since we don't support transforming arbitrary widgets, since it gets weird if they accept input?

Anyway: my basic position would be to emulate what flutter is doing, as closely as possible. Without adding a new widget, this means:

  • first just call bc.constrain(image.size()) to get the output size
  • have a function like applyBoxFit that takes image size and the calculated widget size and returns two rects describing the mapping of source->destination
  • use this to determine what to paint. You can find the paintImage method in the flutter docs

What is important here is that we separate the applying of the constraints from the determination of the widget size. It is not the responsibility of the image widget to choose a size, outside of ensuring that it satisfies the input constraints. If you need to do fancier things to grow or shrink your widget in a given scenario, you should be accomplishing that with some other layout widget.

@fishrockz
Copy link
Collaborator Author

Now I understand the case were you can have infinite bc.max() I think i have caught this edge case and do something sensible for it in the latest version of this PR.

But i had not understood the subtlety of flutter and druids layout so while this now restores the old functionality while also addressing the infinite edge case I'm not sure that the original behaviour was right.

But @cmyr how would you ever get the image to be big? even if it was in a layout widget the current code in master would always just return the image's max? would a layout widget just ignore the image widget and force the image bigger? what are the rules for the images layout function in that case? dose one of the examples cover something like this? even with a completely different widget?

@fishrockz fishrockz force-pushed the willsalmon/fill_not_constrained branch from 6f9c3f1 to 067b52a Compare November 1, 2021 22:00
@fishrockz
Copy link
Collaborator Author

Thinking about this a bit more.

Its worth noting that the code in master dose force the image larger if the widget is constrained. To me constrain dose not mean force to this size but no bigger, so in that case we should have this layout just always return the minimum between bc.max() and image.size()?

@cmyr
Copy link
Member

cmyr commented Nov 1, 2021

The parent layout widget would only influence the child by modifying the constraints. For instance if you had received constraints that were (0,0) (500,500) you could output 'tight' constraints that were (500,500), (500,500). This is achievable by built in widgets, and if you needed more complicated conditional logic you could have a custom container widget that implements it.

I definitely agree that the original (and all subsequent!) impls of the image widget, in druid, have had incomplete implementations of the layout fn.

I think the best approach is to step back and try to just think this through systematically, instead of trying to achieve some specific set of layout objectives.

@fishrockz
Copy link
Collaborator Author

fishrockz commented Nov 1, 2021

The parent layout widget would only influence the child by modifying the constraints. For instance if you had received constraints that were (0,0) (500,500) you could output 'tight' constraints that were (500,500), (500,500). This is achievable by built in widgets, and if you needed more complicated conditional logic you could have a custom container widget that implements it.

I don't understand this. Are the responsibilities of the layout fn documented some were, the trait docs were a bit light IIRC..

edit: eg: the layout returns a size , wich i understand to be (x,y) not (x,y) (x,y)

@cmyr
Copy link
Member

cmyr commented Nov 2, 2021

Sorry, I was composing that on my phone and I wasn't done, I guess it posted it after some timeout? Not sure.

The way that a parent widget influences a child's layout is by modifying the BoxConstraints, which are represented as a pair of (min, max) sizes. If these are equal, then we call the constraints "tight"; that is, ther is only one size that fits them.

For a good overview of how this works, check out the flutter docs for BoxConstraints.

@JAicewizard
Copy link
Contributor

Now I finally under stand your use case, i might be able think of something else..

You dont have to, your code works perfectly fine when everything is constrained and we can use it in that case. The only case it doesnt cover is when either sidie (or both ofc) is unconstrained. This is what my code handles, so you can combine them as I described in my review.

It is not the responsibility of the image widget to choose a size, outside of ensuring that it satisfies the input constraints.

Could you maybe explain a bit more? The I think the widget should chose a size it will actually use, aka not take more space than actually needed. And this being an image widget I would also say that it is usefull to take the maximum size we will end up using (with accordance to the fillstrat).

let mut max_width = bc.max().width;
let mut max_height = bc.max().height;
let mut fill_strat = self.fill;
if max_width == f64::INFINITY && max_height == f64::INFINITY {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use bc.is_xxx_bounded.

Copy link
Collaborator Author

@fishrockz fishrockz Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These detect times when it would note be safe to use the max so they work quite differently. ah yes

Comment on lines +210 to +223
fill_strat = match fill_strat {
FillStrat::FitWidth if max_width == f64::INFINITY => FillStrat::Contain,
FillStrat::FitHeight if max_height == f64::INFINITY => FillStrat::Contain,
FillStrat::Cover | FillStrat::Fill => FillStrat::Contain,
_ => fill_strat,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This effectively the same code as I had, but much more convoluted. It is more clear to use the already existing code in this case (and the if statement on line 200-205 as well). Maybe it would be nice to have a match on (bool,bool) for the boundness of each axis and have clean fast code for each case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this does behave very much like your code out side of the options that fall back to Contain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes I see I missed 2 cases.

@fishrockz
Copy link
Collaborator Author

@cmyr If i understand correctly fishrockz@a123167 would be much more idiomatic of fluter. BUT it will cause issues with the srollbar/srollbox with some fill options in paint (paint will crash as @JAicewizard points out). Am I right in thinking that flutter would error out at the layout stage in this case? https://flutter.dev/docs/testing/common-errors#vertical-viewport-was-given-unbounded-height this seems to imply that flutter just refuses to layout a widget if something silly was going to happen? Our layout dose not provide a option to return a error so I'm not sure what to do with that case, I presume you dont want the layout to panic? I can detect the issue at paint time and paint a big error message instead of crashing?

I think the most pragmatic option is to do the min/max thing but with some logic to detect is_infinite() and expand as sensibly as possible in that case? were sensible is what we think is least bad rather than idiomatic?

@cmyr
Copy link
Member

cmyr commented Nov 4, 2021

if the input constraints are not bounded, the image widget should ignore FillStrat completely and just choose its 'natural' size.

It is very possible in druid to produce a layout that is not meaningful/is poorly formed, like when you expand() inside of a scrollview. In these cases we tend to log an error and continue. In the future it would definitely be possible to have more debug tools available during layout for diagnosing these sorts of problems.

@fishrockz
Copy link
Collaborator Author

fishrockz commented Nov 4, 2021

It is very possible in druid to produce a layout that is not meaningful/is poorly formed, like when you expand() inside of a scrollview. In these cases we tend to log an error and continue. In the future it would definitely be possible to have more debug tools available during layout for diagnosing these sorts of problems.

Right but AFAICT the reason that @JAicewizard updated this widget last was that druid was not logging a error and continuing but crashing horribly. In that case what does logging a error and continuing look like for a image? The edge case i am trying to deal with in my mind is image in a size.expand() in a vertical scroll bar this does not make sense but we dont want druid to crash during the paint so what would you like to happen?

if layout returns the minimal box constraint which in that case would be fixed width but inf height, then at paint we detect the inf and just paint everything red or just return early without doing anything to the context? Would we log some errors in the paint function or the layout function? do you have a example of this?

@fishrockz
Copy link
Collaborator Author

fishrockz commented Nov 4, 2021

Also expanding on the image inside a expanded size in side a vertical scroll bar.

If FillSrat is Fill then clearly the right answer is to gracefully error. but if the FillStrat is to FillWidth,Contain,etc. then i think it makes much more sence to return a layout size that makes sense and allow for the image to fill the width and enough of the inf height to display the image correctly.

I don't really understand flutter enough to know what the write answer is.

Also in general:

If I'm only ever going to paint some of the screen because I have ScaleDown or None for the fill strat should the image's layout function respect the min of the tight constraint and return a size that is bigger than it will ever paint or just return the max that it will ever paint at layout time?

@xStrom
Copy link
Member

xStrom commented Jan 12, 2023

What's the status of this PR? Are we still moving foward with it @fishrockz?

If so, then please rebase on master to resolve the merge conflicts.

@xStrom xStrom added the S-waiting-on-author waits for changes from the submitter label Jan 12, 2023
@fishrockz
Copy link
Collaborator Author

@xStrom I'm happy to fix this, I got blocked as @cmyr wants this to be as much like flutter as possible but I got blocked on actually testing flutter as the docs were not comprehensive enough.

I'm out atm but I'll have a look when I get home

@xStrom
Copy link
Member

xStrom commented Jan 12, 2023

Excellent, having this rebased will help me test the code too.

I'll read through the earlier messages here tomorrow to better understand the history of concerns.

@fishrockz fishrockz force-pushed the willsalmon/fill_not_constrained branch from 067b52a to e69fa2a Compare January 12, 2023 21:45
@fishrockz
Copy link
Collaborator Author

@xStrom i have rebased to master

If you check out master and run cargo run --example image --features="image png" if you untick the fixed width then no mater what fill strat you pick you can not get the image to fill the space. This is a fetcher i find helpful.

But if you checkout this branch and run cargo run --example image --features="image png" and untick the fixed width then you can get the old behaviour from before with None or scale down but you can also use the other fill strategies effectively and how they worked when they were fist merged before JAicewizard changed the behaviour in PRs after i introduced this widget.

It seems cmyr wanted the flutter behaviour than rather than ether of our behaviours.

I did try to get closer to the flutter behaviour, i even have another non pushed branch trying to get there but without a working flutter system to test ageist i was struggling to understand exactly what flutter did.

@xStrom xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-review waits for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants