Re: [External] : Re: Pivot properties [was: Enhancements for JavaFX 18]
I think that „computed pivot“ is both an unnecessary terminology, and
really only one special case of a pivot coordinate in unit space where 0/0
is the top-left corner of the node, and 1/1 is the bottom-right corner of
the node (which is what I referred to as „relative“ coordinates).
Quite often, it can be more useful to define pivots in unit space, because
it’s an easy answer to a problem like „rotate the node 45 degrees around
its top-left corner“. Special-casing the 0.5/0.5 unit coordinate and not
allowing any other feels like a lackluster API, and increases the effort
for developers to solve a very common problem.
>From an implementation perspective, there’s not much of a difference in
development and maintenance cost. The projection from unit space to pixel
space needs to happen anyway (that’s the „computed“ part in „computed
pivot“), and allowing any coordinate rather than the 0.5/0.5 midpoint is
only a single multiplication away.
On Saturday, August 21, 2021, Kevin Rushforth
wrote:
> I didn't page in enough of the discussion before. I remember now why we
> need separate pivots for rotate and scale, so thank you for reminding me.
> Btw, I don't think that necessarily means we need a separate switch for
> whether to use computed vs explicit for each (but that would be a
> possibility).
>
> I agree with the points raised about relative versus "absolute". While it
> is an intriguing idea, it probably doesn't fit here.
>
> While it is a clever idea to use an ObjectProperty rather than a
> DoubleProperty so we have an out of band value to use for "compute this" I
> don't think that we should do it that way for a few reasons. First, it's
> inconsistent with all other number based properties, in a way that could
> cause usability issues (e.g., you couldn't just pass pivotXProperty()
> directly to a method that expects a DoubleProperty). Second, I don't like
> the implications of having separate "auto-compute" flags on each of X, Y,
> and Z. Third, is the documentation issue you mentioned.
>
> So amending my earlier answers to the three questions:
>
> 1. A single boolean property for whether to use a compute pivot (the
> default) vs an explicit pivot; this would apply to both rotate and scale
> pivots.
> 2. 3 separate doubles (using DoubleProperty).
> 3. Yes. Separate rotatePivot{X,Y,Z} and scalePivot{X,Y,Z} properties.
>
> -- Kevin
>
> On 8/21/2021 6:19 AM, Nir Lisker wrote:
>
> In the previous time this was discussed I suggested a 3rd option, in
> addition to ObjectProperty and 3 DoubleProperties, which is 3
> ObjectPropertys.
>
> Advantages:
> 1. Allows using null to remove any specified user values (null would also
> be the default), similar to how we would do it with
> ObjectProperty. No need for another property like a boolean value.
> 2. Allows changing individual axes and binding to each axis independently,
> which was a disadvantage of Point3D.
> 3. With how Valhalla is coming along, it's highly likely that there would
> be no auto-boxing cost as Double will behave like a primitive.
>
> Disadvantages:
> 1. Using null as the special value does not indicate well that the default
> behavior is using the center of the node, but there is no indication of it
> now, except in documentation which will be done anyway.
> 2. ObjectProperty does not have the specialized methods inherited
> from DoubleExpression (like #add), so it is more difficult to work with
> mathematically.
>
> This addresses questions 1 and 2. The 2nd point in the disadvantages makes
> this not too appealing for me, but the other options are also problematic.
>
> As for question 3, I think we have to. The main goal of the PR is adding
> pivot properties to ScaleTransition and RotateTransition. The various
> transitions work by changing values in Node (FadeTransform changes opacity,
> RotateTransition changes rotate and rotationAxis etc.). If we don't have
> separate pivots for rotation and scale in Node, how can we implement these
> in the transitions? They surely can't use the same one, both because it
> will be confusing and there will be synching issues. Think about creating 1
> RotateTransition and 1 ScaleTransition with different pivots. The order in
> which you start them will matter because one will change the other (and
> if forceSync is enabled then one will override the other mid-transition).
> Having these pivots on Node for direct usage is convenient, but I see it
> as a secondary goal. It is more of a necessity for the transitions. Notice
> that the JBS issue [1] is aggregating 3 issues because they are
> interdependent.
>
> As for Michael's proposal, it has the benefit of getting rid of the
> special computed value of the center of the node and making it clear what
> the default is.
> I'm not sure what Michael means by "absolute coordinates", I assume it's
> relative to the parent, and that "relative to the layout bounds" means
> relative to the node. This way, setting to 0.5 with "relative to the layout
> bounds"
Re: [External] : Re: Pivot properties [was: Enhancements for JavaFX 18]
I didn't page in enough of the discussion before. I remember now why we
need separate pivots for rotate and scale, so thank you for reminding
me. Btw, I don't think that necessarily means we need a separate switch
for whether to use computed vs explicit for each (but that would be a
possibility).
I agree with the points raised about relative versus "absolute". While
it is an intriguing idea, it probably doesn't fit here.
While it is a clever idea to use an ObjectProperty rather than a
DoubleProperty so we have an out of band value to use for "compute this"
I don't think that we should do it that way for a few reasons. First,
it's inconsistent with all other number based properties, in a way that
could cause usability issues (e.g., you couldn't just pass
pivotXProperty() directly to a method that expects a DoubleProperty).
Second, I don't like the implications of having separate "auto-compute"
flags on each of X, Y, and Z. Third, is the documentation issue you
mentioned.
So amending my earlier answers to the three questions:
1. A single boolean property for whether to use a compute pivot (the
default) vs an explicit pivot; this would apply to both rotate and scale
pivots.
2. 3 separate doubles (using DoubleProperty).
3. Yes. Separate rotatePivot{X,Y,Z} and scalePivot{X,Y,Z} properties.
-- Kevin
On 8/21/2021 6:19 AM, Nir Lisker wrote:
In the previous time this was discussed I suggested a 3rd option, in
addition to ObjectProperty and 3 DoubleProperties, which is 3
ObjectPropertys.
Advantages:
1. Allows using null to remove any specified user values (null would
also be the default), similar to how we would do it with
ObjectProperty. No need for another property like a boolean
value.
2. Allows changing individual axes and binding to each axis
independently, which was a disadvantage of Point3D.
3. With how Valhalla is coming along, it's highly likely that there
would be no auto-boxing cost as Double will behave like a primitive.
Disadvantages:
1. Using null as the special value does not indicate well that the
default behavior is using the center of the node, but there is no
indication of it now, except in documentation which will be done anyway.
2. ObjectProperty does not have the specialized methods
inherited from DoubleExpression (like #add), so it is more difficult
to work with mathematically.
This addresses questions 1 and 2. The 2nd point in the disadvantages
makes this not too appealing for me, but the other options are also
problematic.
As for question 3, I think we have to. The main goal of the PR is
adding pivot properties to ScaleTransition and RotateTransition. The
various transitions work by changing values in Node (FadeTransform
changes opacity, RotateTransition changes rotate and rotationAxis
etc.). If we don't have separate pivots for rotation and scale in
Node, how can we implement these in the transitions? They surely can't
use the same one, both because it will be confusing and there will be
synching issues. Think about creating 1 RotateTransition and 1
ScaleTransition with different pivots. The order in which you start
them will matter because one will change the other (and if forceSync
is enabled then one will override the other mid-transition).
Having these pivots on Node for direct usage is convenient, but I see
it as a secondary goal. It is more of a necessity for the transitions.
Notice that the JBS issue [1] is aggregating 3 issues because they are
interdependent.
As for Michael's proposal, it has the benefit of getting rid of the
special computed value of the center of the node and making it clear
what the default is.
I'm not sure what Michael means by "absolute coordinates", I assume
it's relative to the parent, and that "relative to the layout bounds"
means relative to the node. This way, setting to 0.5 with "relative to
the layout bounds" will indeed mean the center of the node.
I find some inconsistency issues with the relativePivot idea:
1. The Scale, Rotate, and Shear transforms use the parent coordinates.
That means that if I set a pivoted scale transform on the node, and
then use a pivoted scale transition (keeping the default), the pivot
will not be the same. This would be very confusing I think.
2. TranslateTransition has a toX property (and Y and Z) that always
uses the parent coordinates. That means that if I use a translate
transition and a scale transition (keeping the default), I will be
working in 2 coordinate systems simultaneously. Also confusing for me.
3. Switching between local and global coordinate systems is a more
general issue, and solving it locally just for pivots of 2 transitions
seems incomplete and out of place to me in this patch.
To expand on point 3, I see 3 generally useful systems: node, parent,
and scene. We have methods that transform from one to the other.
Despite those, I also find it a lot of work to transform between
coordinate systems. This leads me to believe that
Re: RFR: 8172095: Let Node.managed become CSS-styleable
On Wed, 18 Aug 2021 06:45:37 GMT, Abhinay Agarwal wrote: > Is there anything else pending to move this PR forward? Nothing else needed for now. The review can proceed. - PR: https://git.openjdk.java.net/jfx/pull/602
Re: Pivot properties [was: Enhancements for JavaFX 18]
In the previous time this was discussed I suggested a 3rd option, in addition to ObjectProperty and 3 DoubleProperties, which is 3 ObjectPropertys. Advantages: 1. Allows using null to remove any specified user values (null would also be the default), similar to how we would do it with ObjectProperty. No need for another property like a boolean value. 2. Allows changing individual axes and binding to each axis independently, which was a disadvantage of Point3D. 3. With how Valhalla is coming along, it's highly likely that there would be no auto-boxing cost as Double will behave like a primitive. Disadvantages: 1. Using null as the special value does not indicate well that the default behavior is using the center of the node, but there is no indication of it now, except in documentation which will be done anyway. 2. ObjectProperty does not have the specialized methods inherited from DoubleExpression (like #add), so it is more difficult to work with mathematically. This addresses questions 1 and 2. The 2nd point in the disadvantages makes this not too appealing for me, but the other options are also problematic. As for question 3, I think we have to. The main goal of the PR is adding pivot properties to ScaleTransition and RotateTransition. The various transitions work by changing values in Node (FadeTransform changes opacity, RotateTransition changes rotate and rotationAxis etc.). If we don't have separate pivots for rotation and scale in Node, how can we implement these in the transitions? They surely can't use the same one, both because it will be confusing and there will be synching issues. Think about creating 1 RotateTransition and 1 ScaleTransition with different pivots. The order in which you start them will matter because one will change the other (and if forceSync is enabled then one will override the other mid-transition). Having these pivots on Node for direct usage is convenient, but I see it as a secondary goal. It is more of a necessity for the transitions. Notice that the JBS issue [1] is aggregating 3 issues because they are interdependent. As for Michael's proposal, it has the benefit of getting rid of the special computed value of the center of the node and making it clear what the default is. I'm not sure what Michael means by "absolute coordinates", I assume it's relative to the parent, and that "relative to the layout bounds" means relative to the node. This way, setting to 0.5 with "relative to the layout bounds" will indeed mean the center of the node. I find some inconsistency issues with the relativePivot idea: 1. The Scale, Rotate, and Shear transforms use the parent coordinates. That means that if I set a pivoted scale transform on the node, and then use a pivoted scale transition (keeping the default), the pivot will not be the same. This would be very confusing I think. 2. TranslateTransition has a toX property (and Y and Z) that always uses the parent coordinates. That means that if I use a translate transition and a scale transition (keeping the default), I will be working in 2 coordinate systems simultaneously. Also confusing for me. 3. Switching between local and global coordinate systems is a more general issue, and solving it locally just for pivots of 2 transitions seems incomplete and out of place to me in this patch. To expand on point 3, I see 3 generally useful systems: node, parent, and scene. We have methods that transform from one to the other. Despite those, I also find it a lot of work to transform between coordinate systems. This leads me to believe that the idea of toggling between coordinate systems tries to solve a broader issue, one that touches both transforms and transitions (including node properties). As a result, I think that we should implement the current proposal of adding pivots by using the current coordinate system that everything else uses (parent), and then think about better ways of handling coordinate system transformations more generally. [1] https://bugs.openjdk.java.net/browse/JDK-8234712 On Thu, Aug 19, 2021 at 11:35 PM Kevin Rushforth wrote: > The idea of adding explicit pivot properties was met with general > agreement as long as we preserve compatibility (meaning that an > auto-computed pivot at the center of the bounds must remain the default). > > The questions are around what form the API should take. They basically > boil down to: > > 1. How do we indicate whether the pivot is computed vs explicitly > specified? > 2. Should the pivot position be 3 doubles or a Point3D > 3. Do we need a separate pivot for rotation vs scaling > > My quick thoughts (although I might be able to be persuaded otherwise) are: > > 1) a boolean property > 2) 3 separate doubles > 3) no > > Michael's proposal of a couple months ago [1] would match those answers, > and is an interesting approach that I hadn't considered before. The > question then is whether want the additional flexibility that it provides > and whether it is worth the additional
Re: RFR: 8271398: GTK3 drag view image swaps red and blue color channels [v4]
On Fri, 20 Aug 2021 22:22:51 GMT, Thiago Milczarek Sayao wrote: >> It seems raw images need to be converted BRGA -> RGBA. >> >> It was being converted on gtk2 code path, but gtk3 only uses >> `gtk_drag_set_icon_pixbuf`. >> >> I have simplified the gtk2 `DragView::View::expose` to paint with >> `gdk_cairo_set_source_pixbuf` (that is available since Gtk 2.8) because the >> old way was somehow converting again. >> >> Run the issue sample with `-Djdk.gtk.version=2` to test the gtk2 code path. >> >> To test: >> >> `./gradlew apps` >> >> >> java @build/run.args -cp apps/toys/DragDrop/dist/DragDrop.jar >> dragdrop.DragDropWithControls >> java @build/run.args -cp apps/toys/DragDrop/dist/DragDrop.jar >> dragdrop.DragDropColor >> >> java -Djdk.gtk.version=2 @build/run.args -cp >> apps/toys/DragDrop/dist/DragDrop.jar dragdrop.DragDropWithControls >> java -Djdk.gtk.version=2 @build/run.args -cp >> apps/toys/DragDrop/dist/DragDrop.jar dragdrop.DragDropColor > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Review requests. Looks good to me now - Marked as reviewed by pbansal (Committer). PR: https://git.openjdk.java.net/jfx/pull/599
