RFR: 8311527: Region.snapInnerSpace*()

[John Hendrikx]

On Thu, 7 Sep 2023 16:24:47 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

> > Are you sure you'll be needing these methods for solving the table column resizing issues?
> 
> Se here is the rules the way I see it:
> 
> * For snapping the **min** constraint, we should be using snapSize, since the expected result should be the same or larger so as not to violate the constraint.
> * For snapping the **max** constraint, we should be using snapInnerSpace, since the expected result should be the same or smaller so as not to violate the constraint.

Are we going to be changing this, as that's not what current containers are doing?  All calculations (min/pref/max) use the same function.  That's probably a good idea as otherwise `min >= max` won't hold anymore if you use two different roundings.

I see very little problem in exceeding max in order to do snapping.  Snapping by definition has the leeway to slightly alter the constraints to align to pixels.  

> In the context of layouts, we should not have a situation when we lay out an inner Region and it goes beyond the boundaries of the enclosing container by one pixel, right? That's the rationale for having snapInnerSpace().

The off-by-one-pixel problem we've been seeing does not have its cause in using the wrong rounding AFAIK. It's because of accumulating errors in the float calculations that are not correctly handled.  The problem also occurs when containers have not reached their maximum size, so it does not have its cause in the max size specified (often max size for resizable containers is `Double.MAX_VALUE` or infinity anyway).

There's also a good chance that if you change the rounding without dealing with the accumulating errors, you'll just have the opposite problem (instead of the child being one pixel too big, it sometimes will be one pixel too small).

> I see at least two use cases:
> 
> 1. When laying out an unsnapped container which contains snapped children.  In order to get the available (snapped) space, we'd need to floor the unsnapped size.

I don't think we will want to start using different snap functions depending on the snapped status of the parent.  Snapping doesn't work when any parent is unsnapped, as the snapping occurs relative to the parent.   This was brought up before, and it's practically impossible to snap to nearest pixels when contained in an unsnapped parent (you also may not want to, because if the parent is animated this attempting to resnap a child will become visible as a weird jitter effect).

There are also problems that when a parent has its start and end point not aligned to pixels, you can't actually render the child correctly without either foregoing some snapping (at the borders, which will impact the inner borders and margins in turn), or misaligning the child's borders with the parent ones (which can show artifacts like a thin line of the parent's background showing through).

> 2. When laying out a complex constraint layout, such as a variant of TableLayout (see for example https://github.com/andy-goryachev/FxDock/blob/master/doc/CPane.md ), where there might exist external constraints on one or more columns and/or rows, there might be a need to snap the constraint value.  How it needs to be snapped depends on the actual constraint, for example when we have a **maximum width** constraint in an otherwise unconstrained situation, we need snapInnerSpace().

See above, I think min/pref/max should use the same roundings, and there is no need to strictly adhere to "min" or "max" when snapping is enabled -- snapping is allowed to ignore these values to align to the nearest pixel -- the docs can be made to mention this: `When snapping is enabled, positions, spacings and sizes may be violated up to a maximum of one pixel to ensure alignment to pixel boundaries` -- or you could be even more specific and mention that it is half a pixel up or down for spacings,  up to one pixel more for sizes, etc...

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1190#issuecomment-1710860028