RFR: 8217472: Add attenuation for PointLight

[Ambarish Rapte]

On Sun, 17 Nov 2019 04:15:34 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

> CSR: https://bugs.openjdk.java.net/browse/JDK-8218264

I have added few comments, but have not run tests and sample yet.

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGPointLight.java line 71:

> 70:     public void setCa(double ca) {
> 71:         this.ca = ca;
> 72:         visualsChanged();

`if (this.ca != ca)`  check can be added here. Similar for `la`, `qa` and `maxRange`

modules/javafx.graphics/src/main/java/com/sun/prism/d3d/D3DContext.java line 556:

> 555:     void setPointLight(long nativeMeshView, int index, float x, float y, float z,
> 556:             float r, float g, float b, float w, float ca, float la, float qa,  float maxRange) {
> 557:         nSetPointLight(pContext, nativeMeshView, index, x, y, z, r, g, b, w, ca, la, qa, maxRange);

There is an extra space before `float maxRange`

modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java line 97:

> 96:      * The maximum range of this {@code PointLight}. For a pixel to be affected by this light, its distance to
> 97:      * the light source must be less than or equal to the light's maximum range. Any negative value will treated
> 98:      * as 0.

=> will `be` treated as 0.

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGPointLight.java line 43:

> 42:     private static final double DEFAULT_MAX_RANGE = Double.POSITIVE_INFINITY;
> 43: 
> 44:     public NGPointLight() {

Will it be a good idea to move these constants to `PointLight` class?
However they look good here too.

modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java line 41:

> 40:  * unless it belongs to a {@code Shape3D} outside of its {@code scope}.
> 41:  * <p>
> 42:  * The light's intensity can be set to decrease over distance by attenuating it. The attenuation formula

Can the behavior be explained in terms of `Shape `or `Node `instead of `Pixel`.
May be something like this,

`Any node within the range of the light will be illuminated by this light, except the nodes that are added to the exclusion scope of this light.`

modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java line 103:

> 102:      * {@code maxRange} value by finding the where the attenuation is close enough to 0.
> 103:      * <p>
> 104:      * Nodes that are inside the light's range can still be excluded from the light's effect

{@code maxRange} value by finding the `distance` where the attenuation is close enough to 0.

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGPointLight.java line 64:

> 63: 
> 64:     private double ca = DEFAULT_CA;
> 65: 

The coefficients are not directly used in any arithmetic on java side. They are converted to `float` and passed to GL or D3D pipeline. Should these be `float `instead of `double` ?

modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 175:

> 174:         lightsAttenuation[a++] = lights[i].attenuation[2];
> 175:         lightsAttenuation[a++] = 0;
> 176: 

In the initial commit this filed was used for `maxRange`. I think that was good idea. Was there any issue with that approach ?

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

Changes requested by arapte (Reviewer).

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