Looked-up color fails for -fx-background-color in JavaFX CSS file

[Duquette, Will (US 397J)]

Howdy!

I'm migrating a code-base from JavaFX 8 to JavaFX 11, and I'm seeing some weird 
behavior.  I have some CSS that looks like this (in part)

.root {
 -theme-button: #0679DE;
}

.button-primary {
 -fx-background-color: -theme-button;
}

It's been working great in JavaFX 8 for quite a long while.  When I load the 
same app, I get messages like this written to the console.

Jun 08, 2021 10:40:07 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Caught 'java.lang.ClassCastException: class java.lang.String cannot be 
cast to class javafx.scene.paint.Paint (java.lang.String is in module java.base 
of loader 'bootstrap'; javafx.scene.paint.Paint is in module 
javafx.graphics@11.0.11 of loader 'platform')' while converting value for 
'-fx-background-color' from rule '*.button-primary' in stylesheet 
file:/Users/will/github/dgs-tool/out/production/dgs-lib/dsn-dark.css

Surprisingly, the color seems to still get set—but warnings written to the 
console are worrying.

If I replace -theme-button with #0679DE in the .button-primary style, the 
message goes away.

I'm seeing this for -fx-background-color and -fx-border-color, but not for any 
other places where I use looked-up colors.

Any ideas?

Re: [External] : Re: Convenience factories for Border and Background

[Nir Lisker]

>
> (Though to be honest for my larger projects most of the time this stuff is
> set in a style sheet.  It’s smaller tools and utilities that I find this
> particularly tedious as the verbosity of the code becomes annoying.)
>

This was exactly my original question. If people use css most for most
cases, what do the small tools and utilities use? Width I can understand,
but is radius common enough for simple cases?

On Tue, Jun 8, 2021 at 6:47 PM Scott Palmer  wrote:

> +1 for having a variant that takes a width. Colour, width, and radius are
> the main parameters I need, so a variant that takes all three would help:
>
>  Border.stroke(Paint p, double width, double radii)
>
> (Though to be honest for my larger projects most of the time this stuff is
> set in a style sheet.  It’s smaller tools and utilities that I find this
> particularly tedious as the verbosity of the code becomes annoying.)
>
> Scott
>
> > On Jun 8, 2021, at 9:59 AM, Kevin Rushforth 
> wrote:
> >
> > I think that the convenience methods should just cover the most common
> cases, so I'd rather skip the dotted and dashed variants. It is a good
> question as to whether there ought to be a variant that takes width. I
> wouldn't do that as the only method, though. I'd lean towards not taking
> the width. Once you start getting into more parameters you can just use the
> constructors without much more trouble.
> >
> > As for the names, I have a slight preference for Border.stroke and
> Background.fill.
> >
> > -- Kevin
> >
> >
> > On 6/8/2021 4:25 AM, Nir Lisker wrote:
> >> Are dashed and dotted used frequently? I find that I only use solid
> unless I'm doing something fancy.
> >>
> >> On Tue, Jun 8, 2021 at 5:21 AM Michael Strauß  > wrote:
> >>
> >>What do you think about this variation?
> >>
> >>Border.solid(Paint color, double width) ->
> >>new Border(new BorderStroke(color, BorderStrokeStyle.SOLID,
> >>null, new BorderWidths(width)))
> >>
> >>Border.dashed(Paint color, double width)  ->
> >>new Border(new BorderStroke(color, BorderStrokeStyle.DASHED,
> >>null, new BorderWidths(width)))
> >>
> >>Border.dotted(Paint color, double width) ->
> >>new Border(new BorderStroke(color, BorderStrokeStyle.DOTTED,
> >>null, new BorderWidths(width)))
> >>
> >>Background.fill(Paint color) ->
> >>new BackgroundFill(color, null, null)
> >>
> >>This gives developers a good deal of customizability before needing
> to
> >>fall back to using constructors.
> >>
> >>
> >>Am Di., 8. Juni 2021 um 03:21 Uhr schrieb Nir Lisker
> >>mailto:nlis...@gmail.com>>:
> >>>
> >>> The new API:
> >>>
> >>> 1. `Border.of(Paint stroke)` or `Border.stroke(Paint stroke)`
> >>that does
> >>> `new Border(new BorderStroke(Paint stroke ,
> >>BorderStrokeStyle.SOLID, null,
> >>> null));`
> >>> 2. `Background.of((Paint fill)` or `Background.fill(Paint fill)`
> >>that does
> >>> `new Background(new BackgroundFill(Paint fill, null, null));`
> >>>
> >>> I don't mind either name choice.
> >>>
> >>
> >
>
>

Re: Support :focus-visible CSS pseudoclass

[Michael Strauß]

> Michael: It does sound like this is a generally useful feature to do in
> the near future, so it makes sense to proceed with the review. I presume
> that the API as you have described it in the JBS description is what you
> are currently proposing? If so, I'll take a look at the API + docs in
> the next couple of days and we can take it from there.

Yes, the JBS issue has the latest version of my proposal for this feature:
https://bugs.openjdk.java.net/browse/JDK-8268225

Re: Support :focus-visible CSS pseudoclass

[Kevin Rushforth]

Thanks to Tom and Pedro for their replies. That's good to know.

Michael: It does sound like this is a generally useful feature to do in 
the near future, so it makes sense to proceed with the review. I presume 
that the API as you have described it in the JBS description is what you 
are currently proposing? If so, I'll take a look at the API + docs in 
the next couple of days and we can take it from there.


As with any non-trivial feature, we won't rush it in -- it will go in 
when it's ready -- so it may already be too late for JavaFX 17, 
especially given the care that needs to be taken for any new CSS 
feature, and the number of other things in flight for JavaFX 17.


-- Kevin



On 6/7/2021 11:55 PM, Tom Schindl wrote:

Hi,

As an application developer I highly appreciate this built-in CSS 
support.


We had to roll our own version for focus-visible and focus-within when 
implementing our custom controls and L&F but we would leverage this 
built-in support to remove complexity from our codebase.


Beside Microsofts new L&F many Web-UI-Toolkits use this 
focus-visible-feature (and emulate it while not all browsers support it).


How we use focus-visible:

I think focus-visible has a fairly narrow scope so we use it to show
input focus differently depending on the way an element got focused
(mainly mouse-activation vs rest)

How we use focus-within:

We use focus-within to visiually indicate the area the focus is 
currently in by changing by lifting or coloring that area.


Tom

Am 08.06.21 um 01:08 schrieb Kevin Rushforth:
I'd be interested in hearing from application developers as to 
whether and how they would use such a feature. As with all 
non-trivial features, we don't want to do it if only one or two 
developers are asking for it. This seems a reasonably new CSS 
feature; Firefox just added support in FF 85 released earlier this 
year and Safari still doesn't. That suggests it might be a bit early 
to adopt this for JavaFX.


Also, one thing that isn't clear is whether there would be any user 
visible change using the standard themes. I presume that the answer 
is "no", meaning that unless your stylesheets do anything with 
focus-visible, it will not affect the look and feel of the app. That 
should be stated explicitly


I also presume that there would be no application visible changes 
when using the standard themes, other than the presence of the two 
new read-only boolean properties?


-- Kevin


On 6/7/2021 3:46 PM, Michael Strauß wrote:

I have expanded the scope of the proposal to include the :focus-within
pseudo-class, which completes the set of focus-related pseudo-classes.

The updated text of the proposal can be found in the JBS ticket:
https://bugs.openjdk.java.net/browse/JDK-8268225

I'm looking forward to comments.

Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v21]

[Kevin Rushforth]

On Mon, 7 Jun 2021 23:38:44 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed doc review comments

One more comment about this PR. As I mentioned earlier, I am no longer able to 
test lighting on my Ubuntu 20.04 Linux Guest running in VirtualBox on my 
Windows 10 Host. In this mode I have to set `-Dprism.forceGPU=true` to get any 
3D support at all, so it isn't a supported config, but point lights used to run 
before spotlight support was added to the shaders, and no longer do. This is 
not a stopper, but makes it harder for me to test (I need to arrange to get 
temporary access to a physical Linux box).

-

PR: https://git.openjdk.java.net/jfx/pull/334

Re: [EXTERNAL] Looked-up color fails for -fx-background-color in JavaFX CSS file

[Kevin Rushforth]

If you have a test program, can you file it here?

https://bugreport.java.com/

Thanks.

-- Kevin


On 6/8/2021 2:12 PM, Duquette, Will (US 397J) wrote:

The problem arises when I switch out the stylesheet for an active GUI, with 
code like this:

root.getStylesheets().clear()
root.getStylesheets().add(styleSheetURL)

It's the clear() that's the problem.  If add the new stylesheet and then remove 
the old one, no messages are produced.

I have a simple test program, if you'd like to have this submitted to a bug 
tracker.

Will Duquette

On 6/8/21, 1:33 PM, "openjfx-dev on behalf of Duquette, Will (US 397J)" 
 wrote:

 Howdy!

 I'm migrating a code-base from JavaFX 8 to JavaFX 11, and I'm seeing some 
weird behavior.  I have some CSS that looks like this (in part)

 .root {
  -theme-button: #0679DE;
 }

 .button-primary {
  -fx-background-color: -theme-button;
 }

 It's been working great in JavaFX 8 for quite a long while.  When I load 
the same app, I get messages like this written to the console.

 Jun 08, 2021 10:40:07 AM javafx.scene.CssStyleHelper calculateValue
 WARNING: Caught 'java.lang.ClassCastException: class java.lang.String 
cannot be cast to class javafx.scene.paint.Paint (java.lang.String is in module 
java.base of loader 'bootstrap'; javafx.scene.paint.Paint is in module 
javafx.graphics@11.0.11 of loader 'platform')' while converting value for 
'-fx-background-color' from rule '*.button-primary' in stylesheet 
file:/Users/will/github/dgs-tool/out/production/dgs-lib/dsn-dark.css

 Surprisingly, the color seems to still get set—but warnings written to the 
console are worrying.

 If I replace -theme-button with #0679DE in the .button-primary style, the 
message goes away.

 I'm seeing this for -fx-background-color and -fx-border-color, but not for 
any other places where I use looked-up colors.

 Any ideas?

Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v21]

[Kevin Rushforth]

On Mon, 7 Jun 2021 23:38:44 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed doc review comments

I've finished reviewing the code for everything except the shaders themselves. 
That will be later this week I hope.

I left a few comments on the tests. I also have one suggestion for an 
additional system test. It would be useful to test that a `Spotlight` with 
values that simulate a `PointLight` -- `direction` of `(0, 0, 1)`, `falloff` of 
0, `innerAngle` of 0, `outerAngle` of 180 -- produce the same results as a 
`PointLight`.

tests/system/src/test/java/test/javafx/scene/lighting3D/LightingTest.java line 
59:

> 57: //  |/
> 58: //
> 59: public class LightingTest {

This class can be `abstract`.

tests/system/src/test/java/test/javafx/s

Re: [EXTERNAL] Looked-up color fails for -fx-background-color in JavaFX CSS file

[Duquette, Will (US 397J)]

The problem arises when I switch out the stylesheet for an active GUI, with 
code like this:

root.getStylesheets().clear()
root.getStylesheets().add(styleSheetURL)

It's the clear() that's the problem.  If add the new stylesheet and then remove 
the old one, no messages are produced.

I have a simple test program, if you'd like to have this submitted to a bug 
tracker.

Will Duquette

On 6/8/21, 1:33 PM, "openjfx-dev on behalf of Duquette, Will (US 397J)" 
 wrote:

Howdy!

I'm migrating a code-base from JavaFX 8 to JavaFX 11, and I'm seeing some 
weird behavior.  I have some CSS that looks like this (in part)

.root {
 -theme-button: #0679DE;
}

.button-primary {
 -fx-background-color: -theme-button;
}

It's been working great in JavaFX 8 for quite a long while.  When I load 
the same app, I get messages like this written to the console.

Jun 08, 2021 10:40:07 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Caught 'java.lang.ClassCastException: class java.lang.String 
cannot be cast to class javafx.scene.paint.Paint (java.lang.String is in module 
java.base of loader 'bootstrap'; javafx.scene.paint.Paint is in module 
javafx.graphics@11.0.11 of loader 'platform')' while converting value for 
'-fx-background-color' from rule '*.button-primary' in stylesheet 
file:/Users/will/github/dgs-tool/out/production/dgs-lib/dsn-dark.css

Surprisingly, the color seems to still get set—but warnings written to the 
console are worrying.

If I replace -theme-button with #0679DE in the .button-primary style, the 
message goes away.

I'm seeing this for -fx-background-color and -fx-border-color, but not for 
any other places where I use looked-up colors.

Any ideas?

Re: [External] : Re: Convenience factories for Border and Background

[Scott Palmer]

+1 for having a variant that takes a width. Colour, width, and radius are the 
main parameters I need, so a variant that takes all three would help:

 Border.stroke(Paint p, double width, double radii) 

(Though to be honest for my larger projects most of the time this stuff is set 
in a style sheet.  It’s smaller tools and utilities that I find this 
particularly tedious as the verbosity of the code becomes annoying.)

Scott

> On Jun 8, 2021, at 9:59 AM, Kevin Rushforth  
> wrote:
> 
> I think that the convenience methods should just cover the most common cases, 
> so I'd rather skip the dotted and dashed variants. It is a good question as 
> to whether there ought to be a variant that takes width. I wouldn't do that 
> as the only method, though. I'd lean towards not taking the width. Once you 
> start getting into more parameters you can just use the constructors without 
> much more trouble.
> 
> As for the names, I have a slight preference for Border.stroke and 
> Background.fill.
> 
> -- Kevin
> 
> 
> On 6/8/2021 4:25 AM, Nir Lisker wrote:
>> Are dashed and dotted used frequently? I find that I only use solid unless 
>> I'm doing something fancy.
>> 
>> On Tue, Jun 8, 2021 at 5:21 AM Michael Strauß > > wrote:
>> 
>>What do you think about this variation?
>> 
>>Border.solid(Paint color, double width) ->
>>new Border(new BorderStroke(color, BorderStrokeStyle.SOLID,
>>null, new BorderWidths(width)))
>> 
>>Border.dashed(Paint color, double width)  ->
>>new Border(new BorderStroke(color, BorderStrokeStyle.DASHED,
>>null, new BorderWidths(width)))
>> 
>>Border.dotted(Paint color, double width) ->
>>new Border(new BorderStroke(color, BorderStrokeStyle.DOTTED,
>>null, new BorderWidths(width)))
>> 
>>Background.fill(Paint color) ->
>>new BackgroundFill(color, null, null)
>> 
>>This gives developers a good deal of customizability before needing to
>>fall back to using constructors.
>> 
>> 
>>Am Di., 8. Juni 2021 um 03:21 Uhr schrieb Nir Lisker
>>mailto:nlis...@gmail.com>>:
>>>
>>> The new API:
>>>
>>> 1. `Border.of(Paint stroke)` or `Border.stroke(Paint stroke)`
>>that does
>>> `new Border(new BorderStroke(Paint stroke ,
>>BorderStrokeStyle.SOLID, null,
>>> null));`
>>> 2. `Background.of((Paint fill)` or `Background.fill(Paint fill)`
>>that does
>>> `new Background(new BackgroundFill(Paint fill, null, null));`
>>>
>>> I don't mind either name choice.
>>>
>> 
> 

Re: Support :focus-visible CSS pseudoclass

[Pedro Duque Vieira]

Hi,

I'm an application developer and a software designer. I've done UI
re-design of applications and designs for my clients. I also usually take
care of UX.

I've also developed a JavaFX theme which has been used by a number of high
profile javafx apps and has a significant number of users (developers using
it), called JMetro. JMetro is based on Fluent Design and I think it would
be great if I could take advantage of this proposed feature.

I think this would be great to have and thanks Michael Strau for taking the
time and effort to propose and develop it.

I also like how new themes can take advantage of this and it's not strictly
necessary for the default JavaFX theme to support it. I think we could make
the default JavaFX theme support it later.

Thanks,


-- 
Pedro Duque Vieira - https://www.pixelduke.com


Virus-free.
www.avg.com

<#m_-5982963222453468535_DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v21]

[Kevin Rushforth]

On Mon, 7 Jun 2021 23:38:44 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed doc review comments

I reviewed the API docs, and they look good. Can you update the CSR with the 
latest docs and move it to "Proposed"?

-

PR: https://git.openjdk.java.net/jfx/pull/334

Re: [External] : Re: Convenience factories for Border and Background

[Kevin Rushforth]

I think that the convenience methods should just cover the most common 
cases, so I'd rather skip the dotted and dashed variants. It is a good 
question as to whether there ought to be a variant that takes width. I 
wouldn't do that as the only method, though. I'd lean towards not taking 
the width. Once you start getting into more parameters you can just use 
the constructors without much more trouble.


As for the names, I have a slight preference for Border.stroke and 
Background.fill.


-- Kevin


On 6/8/2021 4:25 AM, Nir Lisker wrote:
Are dashed and dotted used frequently? I find that I only use solid 
unless I'm doing something fancy.


On Tue, Jun 8, 2021 at 5:21 AM Michael Strauß > wrote:


What do you think about this variation?

    Border.solid(Paint color, double width) ->
        new Border(new BorderStroke(color, BorderStrokeStyle.SOLID,
null, new BorderWidths(width)))

    Border.dashed(Paint color, double width)  ->
        new Border(new BorderStroke(color, BorderStrokeStyle.DASHED,
null, new BorderWidths(width)))

    Border.dotted(Paint color, double width) ->
        new Border(new BorderStroke(color, BorderStrokeStyle.DOTTED,
null, new BorderWidths(width)))

    Background.fill(Paint color) ->
        new BackgroundFill(color, null, null)

This gives developers a good deal of customizability before needing to
fall back to using constructors.


Am Di., 8. Juni 2021 um 03:21 Uhr schrieb Nir Lisker
mailto:nlis...@gmail.com>>:
>
> The new API:
>
> 1. `Border.of(Paint stroke)` or `Border.stroke(Paint stroke)`
that does
> `new Border(new BorderStroke(Paint stroke ,
BorderStrokeStyle.SOLID, null,
> null));`
> 2. `Background.of((Paint fill)` or `Background.fill(Paint fill)`
that does
> `new Background(new BackgroundFill(Paint fill, null, null));`
>
> I don't mind either name choice.
>

Re: RFR: 8244075: Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is removed from Scene [v2]

[Ambarish Rapte]

On Fri, 4 Jun 2021 21:04:51 GMT, Kevin Rushforth  wrote:

>> Ambarish Rapte has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   update @ignore() with bugid
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/AcceleratorParameterizedTest.java
>  line 352:
> 
>> 350: }
>> 351: 
>> 352: @Ignore("Passes only for Button")
> 
> Can you file the follow-on bug and then update this comment to use that bug 
> ID?

Updated with bug ID 
[JDK-8268374](https://bugs.openjdk.java.net/browse/JDK-8268374)

-

PR: https://git.openjdk.java.net/jfx/pull/515

Re: RFR: 8244075: Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is removed from Scene [v2]

[Ambarish Rapte]

> Issue:
> There are several issues related to Accelerator of MenuItem of a ConextMenu 
> set on Control.
> 1. Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is 
> removed from Scene
> 2. Accelerator is not updated correctly when the Control is removed from a 
> Scene and Added to a different Scene
> 3. Accelerator is not removed from Scene when the anchor node is removed from 
> Scene and then it's ContextMenu is set to null
> 
> Fix:
> The accelerator should be added or removed correctly according to the Scene 
> property of the anchor node. 
> The issue#3 in above list is only fixed for Button and fails for other 
> Controls. A test is added for this scenario and I shall report a new issue to 
> address it.
> Added tests that fails before and pass after the fix.

Ambarish Rapte has updated the pull request incrementally with one additional 
commit since the last revision:

  update @ignore() with bugid

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/515/files
  - new: https://git.openjdk.java.net/jfx/pull/515/files/f50a0232..45a4db9b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=515&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=515&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/515.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/515/head:pull/515

PR: https://git.openjdk.java.net/jfx/pull/515

Re: RFR: 8244075: Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is removed from Scene [v2]

[Kevin Rushforth]

On Tue, 8 Jun 2021 11:48:20 GMT, Ambarish Rapte  wrote:

>> Issue:
>> There are several issues related to Accelerator of MenuItem of a ConextMenu 
>> set on Control.
>> 1. Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is 
>> removed from Scene
>> 2. Accelerator is not updated correctly when the Control is removed from a 
>> Scene and Added to a different Scene
>> 3. Accelerator is not removed from Scene when the anchor node is removed 
>> from Scene and then it's ContextMenu is set to null
>> 
>> Fix:
>> The accelerator should be added or removed correctly according to the Scene 
>> property of the anchor node. 
>> The issue#3 in above list is only fixed for Button and fails for other 
>> Controls. A test is added for this scenario and I shall report a new issue 
>> to address it.
>> Added tests that fails before and pass after the fix.
>
> Ambarish Rapte has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update @ignore() with bugid

Marked as reviewed by kcr (Lead).

-

PR: https://git.openjdk.java.net/jfx/pull/515

Re: [External] : Re: Convenience factories for Border and Background

[Nir Lisker]

Are dashed and dotted used frequently? I find that I only use solid unless
I'm doing something fancy.

On Tue, Jun 8, 2021 at 5:21 AM Michael Strauß 
wrote:

> What do you think about this variation?
>
> Border.solid(Paint color, double width) ->
> new Border(new BorderStroke(color, BorderStrokeStyle.SOLID,
> null, new BorderWidths(width)))
>
> Border.dashed(Paint color, double width)  ->
> new Border(new BorderStroke(color, BorderStrokeStyle.DASHED,
> null, new BorderWidths(width)))
>
> Border.dotted(Paint color, double width) ->
> new Border(new BorderStroke(color, BorderStrokeStyle.DOTTED,
> null, new BorderWidths(width)))
>
> Background.fill(Paint color) ->
> new BackgroundFill(color, null, null)
>
> This gives developers a good deal of customizability before needing to
> fall back to using constructors.
>
>
> Am Di., 8. Juni 2021 um 03:21 Uhr schrieb Nir Lisker :
> >
> > The new API:
> >
> > 1. `Border.of(Paint stroke)` or `Border.stroke(Paint stroke)` that does
> > `new Border(new BorderStroke(Paint stroke , BorderStrokeStyle.SOLID,
> null,
> > null));`
> > 2. `Background.of((Paint fill)` or `Background.fill(Paint fill)` that
> does
> > `new Background(new BackgroundFill(Paint fill, null, null));`
> >
> > I don't mind either name choice.
> >
>

[jfx11u] RFR: 8267314: Loading some animated GIFs fails with ArrayIndexOutOfBoundsException: Index 4096 out of bounds for length 4096

[Johan Vos]

…xception: Index 4096 out of bounds for length 4096

Reviewed-by: kcr, arapte

-

Commit messages:
 - 8267314: Loading some animated GIFs fails with 
ArrayIndexOutOfBoundsException: Index 4096 out of bounds for length 4096

Changes: https://git.openjdk.java.net/jfx11u/pull/25/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx11u&pr=25&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267314
  Stats: 130 lines in 2 files changed: 118 ins; 1 del; 11 mod
  Patch: https://git.openjdk.java.net/jfx11u/pull/25.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx11u pull/25/head:pull/25

PR: https://git.openjdk.java.net/jfx11u/pull/25

[jfx11u] RFR: 8210199: [linux / macOS] fileChooser can't handle emojis

[Johan Vos]

Reviewed-by: pbansal, kcr

-

Commit messages:
 - 8210199: [linux / macOS] fileChooser can't handle emojis

Changes: https://git.openjdk.java.net/jfx11u/pull/24/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx11u&pr=24&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8210199
  Stats: 15 lines in 2 files changed: 12 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx11u/pull/24.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx11u pull/24/head:pull/24

PR: https://git.openjdk.java.net/jfx11u/pull/24

[jfx11u] RFR: 8216377: JavaFX: memoryleak for initial nodes of Window

[Johan Vos]

8207837: Indeterminate ProgressBar does not animate if content is added after 
scene is set on window

Add or remove the windowShowingChangedListener when the scene changes

Reviewed-by: arapte, kcr

-

Commit messages:
 - 8216377: Fix memoryleak for initial nodes of Window

Changes: https://git.openjdk.java.net/jfx11u/pull/23/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx11u&pr=23&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8216377
  Stats: 120 lines in 2 files changed: 119 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx11u/pull/23.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx11u pull/23/head:pull/23

PR: https://git.openjdk.java.net/jfx11u/pull/23