On Mon, 26 Feb 2024 16:41:05 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> Overhaul to the `PhongMaterial` documentation (and a bit to its superclass
>> `Material`). Except for the introduction, I divided the documentation into 3
>> sections: qualitative explanation, mathematical model (I wouldn't think it
>> necessary, but the current doc explains it), and examples.
>>
>> The reason for the verbosity of the doc is that I envisioned 2 target
>> audiences for this class. One is a Java developer who wants to understand
>> the terminology and workings of computer graphics or of the artists who are
>> already familiar with this domain. (How many Java developers know what
>> diffuse, specular and normal maps are?) The other is an artist who is
>> already familiar with the domain, but wants to see how this class compares
>> with other renderers. For this reason, I looked at the terminology used by
>> engines like Blender, Maya, UE4 and Unity and tried to mention the
>> comparisons (like bump vs. height vs. normal maps, or specular vs.
>> roughness/smoothness).
>>
>> The examples I chose and some of the schematics are not the best, looking at
>> it retroactively, but I want to give enough time for reviewers and get this
>> into 22.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed typo
Nice work @nlisker
Providing few comments, shall continue reviewing.
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 66:
> 64: * reflective metals, water, and reflective ceramics. Neither does light
> refract (bend) when passing through transparent
> 65: * or translucnet materials such as water, glass, or ice. These materials
> rely on <i>Fresnel effects</i> that are not
> 66: * implemented for this material.
Suggestion: Please check if it can be rephrased into something like below, not
exact though:
PhongMaterial is not suitable for surfaces that reflect or refract the
incident light.
Few examples of reflective material are mirror, water, reflective metals,
reflective ceramics
Few examples of refractive material are water, glass, or ice.
These materials rely on Fresnel effects that are not implemented for the
PhongMaterial.
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 78:
> 76: * directions via scattering (purple) depend on the diffuse component;
> rays that are reflected (orange), which depend on
> 77: * the incident angle, are controlled by the specular component.
> 78: * <p>
`<p>` here causes a warning message:- `warning: empty <p> tag`
Moving `<p>` to after line#92 resolves the warning and achieves same formatting
as current.
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 104:
> 102: * that is not reflected directly from the surface and instead enters
> the material.<br>
> 103: * The alpha channel of the diffuse component controls the light that
> passes through it (transmitted). Decreasing this
> 104: * value increases the transparency of the material and causes the
> object to appear translucent, and ultimately makes
Decreasing this value -> Decreasing the alpha value (?)
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 115:
> 113: * <p>
> 114: * <b>Important:</b> there is currently a bug that causes objects with 0
> opacity to not render at all (despite having a
> 115: * specular or a self-illumination component). Setting the opacity to
> 1/255 instead will give the desirable result.
Is there is JBS issue to track this? The JBS issue should have a pointer for
this comment to be removed when fixed.
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 132:
> 130: *
> 131: * A larger specular power simulates a smoother object, which
> 132: * results in a smaller reflection.
Could be combined in one line.
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 135:
> 133: * <p>
> 134: * The specular component interacts only with lights that have
> directionality (not {@code AmbientLight}) as it depends
> 135: * on the incident ray direction, and also on the viewer position since
> it depends on the reflectance direction.
viewer position -> viewer **(camera)** position
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 170:
> 168: * <i>V</i> - the vector from the surface to the viewer (camera);<br>
> 169: * <i>R</i> - the reflection vector of <i>L</i> from the surface.
> <i>R</i> can be calculated from <i>L</i> and <i>N</i>:
> 170: * <i>R=2(L⋅N)N - L</i>.
Should these values be described as respect to point instead of surface ?
for example: the vector from the surface to the light source -> the vector from
the **point** to the light source
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 172:
> 170: * <i>R=2(L⋅N)N - L</i>.
> 171: * <p>
> 172: * The diffuse and and specular components are comprised of 3 factors:
> the geometry, the light's color, and the
Typo: please remove extra _and_ : diffuse and and specular
-------------
Changes requested by arapte (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1378#pullrequestreview-1901677984
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1504072545
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1503102040
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1504075363
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1504106529
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1504111786
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1504112992
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1504129159
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1504131458
