[openjdk/jfx] 8234712: Add pivot properties for scale and rotation in Node, ScaleTransition and RotateTransition (#53)

Sun Feb 23 08:08:55 UTC 2020

I was thinking about this some more. We could use ObjectProperty<Double> as
this would allow to set nulls in order to reset the value to the default
node's center, and will allow independent axis control and binding. It's
rather ugly though in my opinion.

On Tue, Dec 17, 2019 at 2:46 AM Nir Lisker <nlisker at gmail.com> wrote:

> As long as you do the following, it should work as expected:
>>
>>     T(pivotRot) * T(rotate) * T(-pivotRot) * T(pivotScale) * T(scale) *
>> T(-pivotScale) * object coords
>>
>
>  I think that this is what the PR does already. Line 5061 onward is:
>
> localToParentTx = localToParentTx.deriveWithTranslation(rotPivotX,
> rotPivotY, rotPivotZ)
>                                  .deriveWithRotation(rotAngle, rotAxisX,
> rotAxisY, rotAxisZ)
>                                  .deriveWithTranslation(-rotPivotX,
> -rotPivotY, -rotPivotZ);
> ...
> localToParentTx = localToParentTx.deriveWithTranslation(scalePivotX,
> scalePivotY, scalePivotZ)
>                                  .deriveWithScale(getScaleX(),
> getScaleY(), getScaleZ())
>                                  .deriveWithTranslation(-scalePivotX,
> -scalePivotY, -scalePivotZ);
>
> On Tue, Dec 17, 2019 at 2:07 AM Kevin Rushforth <
> kevin.rushforth at oracle.com> wrote:
>
>>
>>
>> On 12/15/2019 6:06 PM, Nir Lisker wrote:
>>
>> Replying on the mailing list to the questions raised on GitHub.
>>
>> The state of whether to use the computed center pivot or the value of the
>>> pivot attribute is implicit with no way for an application to know which it
>>> is, and no way to set it back to using the computed center (i.e., the state
>>> is sticky once you set it). Perhaps if you defined a null value as meaning
>>> "computed center" then an app could at least reset it to the "computed
>>> center" state, although there would still be no way for the app to know
>>> that it was in that state.
>>>
>>
>> In the JBS issue <https://bugs.openjdk.java.net/browse/JDK-8234712> I
>> alluded to this in point 5. I think that null should represent the default
>> (node center). However, if we use 3 doubles instead of a Point3D we might
>> need to use Double.NaN for this instead, which would also be the default
>> for this case. The docs will explain this.
>>
>>
>> Using Double.NaN as an out of band value would be odd, and probably not
>> what we want. In addition to the fact that it is somewhat artificial, it
>> would mean that X, Y, and Z would independently be treated as coming from
>> the set value or from the computed center. I guess this is one argument for
>> using a Point3D object, but as you note, there are other drawbacks.
>>
>> Do we need separate properties for scale pivot and center pivot?
>>>
>>
>> I say yes, otherwise the enhancement will be very limited. I think of
>> this enhancement as adding pivot control to Rotate/Scale transitions, and
>> adding them to Node is a necessary (and desirable) step.
>>
>>
>> Yeah, I figured this was the case.
>>
>>    ... you need to worry about what coordinate space the rotation pivot
>>> is defined in. Perhaps if the rotation pivot were defined in unscaled
>>> space, it might work.
>>>
>>
>> Isn't it already? If I set the rotation pivot to the edge of the node,
>> then scale it down, then rotate, the rotation pivot would be outside of the
>> node's boundaries. In scaled space it would still be on the edge. Or did I
>> misunderstand you?
>>
>>
>> I think I was wrong in the comment I added to the PR.
>>
>> What I meant is that you would need to define the coordinate space that
>> the pivot values are in, and it needs to be defined in a way that it is
>> consistent with current behavior. Today, the part of the matrix
>> transformation that does the scale and rotate, including the computed
>> pivot, is this:
>>
>>     T(pivot) * T(rotate) * T(scale) * T(-pivot) * object coords
>>
>> As long as you do the following, it should work as expected:
>>
>>
>>     T(pivotRot) * T(rotate) * T(-pivotRot) * T(pivotScale) * T(scale) *
>> T(-pivotScale) * object coords
>>
>> Importantly, this will work exactly as it does today when pivotRot ==
>> pivotScale which is the case I was most concerned about.
>>
>> In any case, I don't think that there's a single correct answer here.
>>
>>
>> It needs to be consistent with current behavior, and match what an
>> application would expect. I think it should not be a problem if defined as
>> above.
>>
>> Should the pivot be specified as a Point3D or 3 separate doubles?
>>> Separate doubles... there would be no out-of-band null value to use
>>
>>
>> See my point above about Double.NaN.
>>
>>
>> OK, there is no "natural" out of band value that is likely to be
>> satisfying. We'd probably end up wanting a boolean that controls computed
>> versus explicit (either a single flag or one for each of scalePivot and
>> rotatePivot)
>>
>> The doubles vs Point3D is an important choice. We might want to look into
>> the future even where Point3D (and 2D) could be made into an Inline class
>> with Valhalla, which will help with the performance aspect. Binding to one
>> of the coordinates is sill a problem there, however.
>>
>>
>> I think this (doubles vs Point3D) is really the main question. I don't
>> know what the best answer is, but I'd like to hear from other developers.
>>
>> -- Kevin
>>
>>
>> On Sat, Dec 14, 2019 at 6:25 PM Kevin Rushforth <notifications at github.com>
>> wrote:
>>
>>> This will need discussion on the openjfx-dev mailing list. Here are the
>>> questions that need to be resolved:
>>>
>>>    1.
>>>
>>>    The state of whether to use the computed center pivot or the value
>>>    of the pivot attribute is implicit with no way for an application to know
>>>    which it is, and no way to set it back to using the computed center (i.e.,
>>>    the state is sticky once you set it). Perhaps if you defined a null value
>>>    as meaning "computed center" then an app could at least reset it to the
>>>    "computed center" state, although there would still be no way for the app
>>>    to know that it was in that state.
>>>    2.
>>>
>>>    Do we need separate properties for scale pivot and center pivot? A
>>>    single pivot property would be easier to define, but wouldn't allow you to
>>>    set it from a RotationTransition and a ScaleTransition if you wanted
>>>    to apply both to the same Node. With two separate properties, as you
>>>    have defined it, it is more flexible, but you need to worry about what
>>>    coordinate space the rotation pivot is defined in. The current transform
>>>    chain looks like this:
>>>
>>> T(layout+translate) * T(pivot) * T(rot) * T(scale) * T(-pivot)
>>>     * transform[0] * transform [1] ...
>>>
>>> Perhaps if the rotation pivot were defined in unscaled space, it might
>>> work. The transform chain would then look like this:
>>>
>>> T(layout+translate) * T(pivotRot/scale) * T(rot) * T(-pivotRot/scale) * T(pivotScale) * T(scale) * T(-pivotScale)
>>>     * transform[0] * transform [1] ...
>>>
>>> In any case, you need to think about the implications of having one of
>>> scale or rotate being set explicitly and the other being the computed
>>> center.
>>>
>>>    1. Should the pivot be specified as a Point3D or 3 separate doubles?
>>>    I can see pros / cons of either approach. Separate doubles are more
>>>    consistent with the the pivot values in the Rotate and Scale
>>>    Transform objects, and are easier to using in binding. On the other
>>>    hand, there would be no out-of-band null value to use (see issue 1
>>>    above), so you would need a boolean property for each of scale pivot and
>>>    rotation pivot to indicate whether the computed value or the value of the
>>>    pivot properties should be used. I don't think that the fact that the
>>>    rotation axis is defined as a Point3D should have any bearing on
>>>    whether the pivot should be so defined. I'd probably lean towards separate
>>>    doubles.
>>>
>>> —
>>> You are receiving this because you authored the thread.
>>> Reply to this email directly, view it on GitHub
>>> <https://github.com/openjdk/jfx/pull/53?email_source=notifications&email_token=AI5QOM6KRPRRP7VS5OUH6QLQYUCF7A5CNFSM4JR3TYY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4GCGI#issuecomment-565731609>,
>>> or unsubscribe
>>> <https://github.com/notifications/unsubscribe-auth/AI5QOM4UKZQVDEN2A2HYYETQYUCF7ANCNFSM4JR3TYYQ>
>>> .
>>>
>>
>>