Re: RFR: 8314147: Updated the PhongMaterial documentation [v10]
On Wed, 28 Feb 2024 18:48:21 GMT, Nir Lisker 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: > > Resize specular color images Thanks everyone for the quick and detailed review! - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1971049164
Re: [jfx22] RFR: 8314147: Updated the PhongMaterial documentation
On Thu, 29 Feb 2024 11:46:11 GMT, Nir Lisker wrote: > This pull request contains a backport of commit > [d9645730](https://github.com/openjdk/jfx/commit/d9645730f1e76e95e0bb93ceaeb5550390bf95c1) > from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. Clean backport of doc-only fixed approved for `jfx22` - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1385#pullrequestreview-1908709976
[jfx22] RFR: 8314147: Updated the PhongMaterial documentation
This pull request contains a backport of commit [d9645730](https://github.com/openjdk/jfx/commit/d9645730f1e76e95e0bb93ceaeb5550390bf95c1) from the [openjdk/jfx](https://git.openjdk.org/jfx) repository. - Commit messages: - Backport d9645730f1e76e95e0bb93ceaeb5550390bf95c1 Changes: https://git.openjdk.org/jfx/pull/1385/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1385=00 Issue: https://bugs.openjdk.org/browse/JDK-8314147 Stats: 521 lines in 39 files changed: 461 ins; 13 del; 47 mod Patch: https://git.openjdk.org/jfx/pull/1385.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1385/head:pull/1385 PR: https://git.openjdk.org/jfx/pull/1385
Re: RFR: 8314147: Updated the PhongMaterial documentation [v10]
On Wed, 28 Feb 2024 18:48:21 GMT, Nir Lisker 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: > > Resize specular color images Looks good. Very well explained docs. - Marked as reviewed by arapte (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1378#pullrequestreview-1908063245
Re: RFR: 8314147: Updated the PhongMaterial documentation [v10]
On Wed, 28 Feb 2024 18:48:21 GMT, Nir Lisker 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: > > Resize specular color images Looks good. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1378#pullrequestreview-1907046997
Re: RFR: 8314147: Updated the PhongMaterial documentation [v10]
> 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: Resize specular color images - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/e5466476..60b41ca7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=09 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=08-09 Stats: 0 lines in 5 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: RFR: 8314147: Updated the PhongMaterial documentation [v9]
On Wed, 28 Feb 2024 18:24:30 GMT, Nir Lisker 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: > > Rename and resize images Actually, there are still 5 files that could use to be resized: modules/javafx.graphics/src/main/docs/javafx/scene/paint/doc-files/specular_color/copper_high.png: PNG image data, 820 x 820, 8-bit/color RGB, non-interlaced modules/javafx.graphics/src/main/docs/javafx/scene/paint/doc-files/specular_color/copper_low.png: PNG image data, 820 x 820, 8-bit/color RGB, non-interlaced modules/javafx.graphics/src/main/docs/javafx/scene/paint/doc-files/specular_color/copper_medium.png: PNG image data, 820 x 820, 8-bit/color RGB, non-interlaced modules/javafx.graphics/src/main/docs/javafx/scene/paint/doc-files/specular_color/gold_high.png: PNG image data, 820 x 820, 8-bit/color RGB, non-interlaced modules/javafx.graphics/src/main/docs/javafx/scene/paint/doc-files/specular_color/gold_low.png: PNG image data, 820 x 820, 8-bit/color RGB, non-interlaced - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969600849
Re: RFR: 8314147: Updated the PhongMaterial documentation [v9]
On Wed, 28 Feb 2024 18:24:30 GMT, Nir Lisker 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: > > Rename and resize images The images look good. I'll do a final pass on the docs. - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969590760
Re: RFR: 8314147: Updated the PhongMaterial documentation [v9]
On Wed, 28 Feb 2024 18:24:30 GMT, Nir Lisker 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: > > Rename and resize images I think I got all the images' sizes reduced. Except for a few small points in discussion with Ambarish, I think that this is ready to go in in time for 22. - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969586936
Re: RFR: 8314147: Updated the PhongMaterial documentation [v9]
> 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: Rename and resize images - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/d586d748..e5466476 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=08 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=07-08 Stats: 32 lines in 50 files changed: 0 ins; 0 del; 32 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]
On Wed, 28 Feb 2024 16:01:15 GMT, Nir Lisker wrote: > > One other thing I noticed is that the file names have spaces in them, which > > is not a best practice and causes problems for scripts. Can you change all > > spaces to _ in the names of the newly-added files? > > Yes. What about folders? Also, do you prefer camelCase or the `_` naming? Generally we use all lower case for directory names, but as long as it isn't used as a package name, it isn't as important. - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969357960
Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]
On Wed, 28 Feb 2024 14:21:55 GMT, Kevin Rushforth wrote: > One other thing I noticed is that the file names have spaces in them, which > is not a best practice and causes problems for scripts. Can you change all > spaces to _ in the names of the newly-added files? Yes. What about folders? Also, do you prefer camelCase or the `_` naming? - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969301404
Re: RFR: 8314147: Updated the PhongMaterial documentation [v8]
> 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 typos from review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/2989624d..d586d748 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=07 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=06-07 Stats: 15 lines in 1 file changed: 1 ins; 3 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]
On Tue, 27 Feb 2024 12:13:09 GMT, Ambarish Rapte wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed typo > > modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java > line 170: > >> 168: * V - the vector from the surface to the viewer (camera); >> 169: * R - the reflection vector of L from the surface. >> R can be calculated from L and N: >> 170: * R=2(L⋅N)N - L. > > 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 The sentence above says "Four normalized vectors are considered for each point on the surface", so I think this is clear. The problem with looking at points is that they have no direction, unlike a surface, so you can't have a "normal of a point", you can only have "normal of a surface", and the reflection vector also depends on the directionality of the surface. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1506187511
Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]
On Tue, 27 Feb 2024 11:24:08 GMT, Ambarish Rapte wrote: > PhongMaterial is not suitable for surfaces that reflect or refract the > incident light. But it does reflect the incident light as explained in the paragraphs before. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1506179403
Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]
On Wed, 28 Feb 2024 13:50:38 GMT, Nir Lisker 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: > > Update images Btw, if this ends up missing the deadline for JavaFX 22 (about 24 hours from now) it can go into 22u for JavaFX 22.0.1. The release date for 22.0.1 is only a few weeks after 22, and the deadline to get fixes into 22.0.1 is almost 2 weeks away. - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969223783
Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]
On Wed, 28 Feb 2024 13:50:38 GMT, Nir Lisker 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: > > Update images I looked at the images in the javadoc-generated web page, and the rendered image size looks good. I recommend scaling most of the images down by at least a factor of 4 (at least a factor of 2 in both width and height), since they are much higher resolution than needed for the size they are rendered at in the page. I see that the animated gif for the writeable image is 6 Mbytes, and could probably be scaled down even more without losing quality. - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969143245
Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]
On Wed, 28 Feb 2024 13:50:38 GMT, Nir Lisker 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: > > Update images Thank you for generating new images. Some of the new files are quite large, and the total added storage is 35 Mbytes, which is a lot for binary image files stored in the repo. I haven't generated the docs and looked at them yet (doing that now), but it might be better to use lower resolution images. One other thing I noticed is that the file names have spaces in them, which is not a best practice and causes problems for scripts. Can you change all spaces to `_` in the names of the newly-added files? - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969088841
Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]
On Tue, 27 Feb 2024 11:53:35 GMT, Ambarish Rapte wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed typo > > modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java > line 115: > >> 113: * >> 114: * Important: 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. No, not yet. There a few oddities in the shader that I intend to look at for jfx23, this will be one of them. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1506020787
Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]
On Wed, 28 Feb 2024 13:50:38 GMT, Nir Lisker 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: > > Update images I generated the new background is an engine. I also enlarged the images a bit and added another one in the transparency section with not highlight for comparison. - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1969032540
Re: RFR: 8314147: Updated the PhongMaterial documentation [v7]
> 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: Update images - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/60d45bc8..2989624d Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=06 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=05-06 Stats: 28 lines in 20 files changed: 4 ins; 1 del; 23 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]
On Mon, 26 Feb 2024 16:41:05 GMT, Nir Lisker 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
Noticed a couple typos.
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 65:
> 63: * {@code PhongMaterial} is not suitable for surfaces that act like
> mirrors and reflect their environment, such as
> 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 Fresnel effects that are not
Typo: translucnet-> translucent
modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java
line 211:
> 209: * the interaction between the reflected light and the viewer position:
> R⋅V. As similarly explained in the
> 210: * diffuse component section, the geometric contribution is strongest
> when the viewer is aligned with the reflection
> 211: * vector and is non-existant when they are perpendicular.
Typo: non-existant -> non-existent
-
PR Review: https://git.openjdk.org/jfx/pull/1378#pullrequestreview-1903833417
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1504425654
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1504429803
Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]
On Mon, 26 Feb 2024 16:41:05 GMT, Nir Lisker 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 Fresnel effects 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: *
`` here causes a warning message:- `warning: empty tag`
Moving `` 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.
> 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: *
> 114: * Important: 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: *
> 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: * V - the vector from the surface to the viewer (camera);
> 169: * R - the reflection vector of L from the surface.
> R can be calculated from L and N:
> 170: * R=2(L⋅N)N - L.
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: * R=2(L⋅N)N - L.
> 171: *
> 172: * The diffuse and and specular components are comprised of 3 factors:
> the geometry,
Re: RFR: 8314147: Updated the PhongMaterial documentation [v6]
> 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 - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/83bf2eb1..60d45bc8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=05 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: RFR: 8314147: Updated the PhongMaterial documentation [v5]
On Sat, 24 Feb 2024 14:40:25 GMT, Nir Lisker 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: > > Remove redundant image copies This is a great docs update and exhaustive explanation of how Phong model works. I found one typo to iron out, otherwise LGTM (I will do a second pass once backgrounds are updated). modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java line 227: > 225: * the total specular component contribution is S * > CSM. > 226: * > 227: * Slef-Illumination Typo - `s/Slef/Self` - Changes requested by lkostyra (Committer). PR Review: https://git.openjdk.org/jfx/pull/1378#pullrequestreview-1901233188 PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1502832050
Re: RFR: 8314147: Updated the PhongMaterial documentation [v5]
> 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: Remove redundant image copies - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/e367c882..83bf2eb1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=04 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=03-04 Stats: 65 lines in 3 files changed: 0 ins; 65 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: RFR: 8314147: Updated the PhongMaterial documentation [v4]
> 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: Clarifications for the diffuse component - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/0130d0f5..e367c882 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=03 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:28:40 GMT, Nir Lisker 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: > > Revert unintended formatting I did a first pass of the updated PhongMaterial docs. Nice work! I'll need more time to go through it in detail (and definitely will want Ambarish and Lukasz to review it as well), but I like the structure, the explanations, and the illustrations. - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1962177376
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 23:11:55 GMT, Nir Lisker wrote: >> The only thing I can point at is the OCA (which is linked in the CONTRIBUTOR >> guidelines), which states that contributions need to be an original work. >> >> So to double-check: Aside from that image (and the ones derived from it), >> are all the others your original work? > > Yes, I created the textures in a generator, the schematics I drew myself, and > the screenshots are from the 3D Lighting app. Thanks for confirming. I'll review the text of the doc changes tomorrow. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500083607
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 23:04:50 GMT, Nir Lisker wrote:
>> StringJoiner seems to provide little benefit for maps or key=value pairs.
>> Do you know of an alternative?
>
> A `StringJoiner` seems suitable to me in this case:
>
> new StringJoiner(",", "PhongMaterial[", "]")
> .add("diffuseColor=" + getDiffuseColor())
> .add("specularPower=" + getSpecularPower())
> ...
>
> I guess you can make a helper method that takes a property and creates the
> string for you, like `diffuseColor.getName() + "=" + diffuseColor.getValue()`
> and then it won't look like a key-value pair in the joiner.
>
> You might want to search for the default implementation of records and see
> how they create the `name = value` representation.
It looks like compiler auto-generates code for records.
The last solution is malloc galore: multiple StringBuilders, much worse than
the original concatenation.
All right, sorry for discussion unrelated to this PR.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500054617
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 23:07:21 GMT, Kevin Rushforth wrote: >> Perhaps the contribution guidelines should mention what resources can be >> included. In all other open source projects I worked on it was enough that >> the resource wasn't licensed (or had something like >> https://en.wikipedia.org/wiki/WTFPL). > > The only thing I can point at is the OCA (which is linked in the CONTRIBUTOR > guidelines), which states that contributions need to be an original work. > > So to double-check: Aside from that image (and the ones derived from it), are > all the others your original work? Yes, I created the textures in a generator, the schematics I drew myself, and the screenshots are from the 3D Lighting app. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500052213
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:51:56 GMT, Nir Lisker wrote: >> sorry, Nir. ;-) > > Perhaps the contribution guidelines should mention what resources can be > included. In all other open source projects I worked on it was enough that > the resource wasn't licensed (or had something like > https://en.wikipedia.org/wiki/WTFPL). The only thing I can point at is the OCA (which is linked in the CONTRIBUTOR guidelines), which states that contributions need to be an original work. So to double-check: Aside from that image (and the ones derived from it), are all the others your original work? - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500048531
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:50:06 GMT, Andy Goryachev wrote:
>>> also, the first `+` is unnecessary
>>
>> I think it was done for uniformity with the other properties. I would use a
>> `StringJoiner` these days for such a task anyway.
>
> StringJoiner seems to provide little benefit for maps or key=value pairs. Do
> you know of an alternative?
A `StringJoiner` seems suitable to me in this case:
new StringJoiner(",", "PhongMaterial[", "]")
.add("diffuseColor=" + getDiffuseColor())
.add("specularPower=" + getSpecularPower())
...
I guess you can make a helper method that takes a property and creates the
string for you, like `diffuseColor.getName() + "=" + diffuseColor.getValue()`
and then it won't look like a key-value pair in the joiner.
You might want to search for the default implementation of records and see how
they create the `name = value` representation.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500046959
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:44:05 GMT, Nir Lisker wrote: >> Weird, I specifically reverted the auto-formatting, not sure how it got in. >> Will revert. > >> also, the first `+` is unnecessary > > I think it was done for uniformity with the other properties. I would use a > `StringJoiner` these days for such a task anyway. StringJoiner seems to provide little benefit for maps or key=value pairs. Do you know of an alternative? - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500032969
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:42:18 GMT, Andy Goryachev wrote: >> OK, it will be after the weekend when I can come back to this. Reviewers can >> assume that only the background will change. > > sorry, Nir. ;-) Perhaps the contribution guidelines should mention what resources can be included. In all other open source projects I worked on it was enough that the resource wasn't licensed (or had something like https://en.wikipedia.org/wiki/WTFPL). - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500034313
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:20:56 GMT, Nir Lisker wrote: >> modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java >> line 848: >> >>> 846: + ", specularPower=" + getSpecularPower() + ", >>> diffuseMap=" + getDiffuseMap() + ", specularMap=" >>> 847: + getSpecularMap() + ", bumpMap=" + getBumpMap() + ", >>> selfIlluminationMap=" + getSelfIlluminationMap() >>> 848: + "]"; >> >> could we restore the formatting please? >> also, the first `+` is unnecessary > > Weird, I specifically reverted the auto-formatting, not sure how it got in. > Will revert. > also, the first `+` is unnecessary I think it was done for uniformity with the other properties. I would use a `StringJoiner` these days for such a task anyway. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500028809
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:40:59 GMT, Nir Lisker wrote: >> All of them, since they were derived from the original. I can still tell >> what the source is. > > OK, it will be after the weekend when I can come back to this. Reviewers can > assume that only the background will change. sorry, Nir. ;-) - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500027550
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:36:06 GMT, Kevin Rushforth wrote: >> Do I only need to switch the ones in the introduction that show the >> multiplication of the color and the map, or all of the new images that use >> it as a background? The latter will be a lot of work. > > All of them, since they were derived from the original. I can still tell what > the source is. OK, it will be after the weekend when I can come back to this. Reviewers can assume that only the background will change. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500026555
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:31:59 GMT, Nir Lisker wrote: >> To add to this, such approval would be unlikely, and I am not in favor of >> making the request. Perhaps you can find suitable images already in the >> repo? Here is one that might work: >> >> https://github.com/openjdk/jfx/tree/e0b88bc7cfede46afe28cbb4a2e333df933b5100/apps/toys/FX8-3DFeatures/src/resources > > Do I only need to switch the ones in the introduction that show the > multiplication of the color and the map, or all of the new images that use it > as a background? The latter will be a lot of work. All of them, since they were derived from the original. I can still tell what the source is. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500023069
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
On Thu, 22 Feb 2024 22:21:49 GMT, Kevin Rushforth wrote: >> What would an approval require? > > To add to this, such approval would be unlikely, and I am not in favor of > making the request. Perhaps you can find suitable images already in the repo? > Here is one that might work: > > https://github.com/openjdk/jfx/tree/e0b88bc7cfede46afe28cbb4a2e333df933b5100/apps/toys/FX8-3DFeatures/src/resources Do I only need to switch the ones in the introduction that show the multiplication of the color and the map, or all of the new images that use it as a background? The latter will be a lot of work. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1500019932
Re: RFR: 8314147: Updated the PhongMaterial documentation [v3]
> 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: Revert unintended formatting - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/34bbc561..0130d0f5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=01-02 Stats: 53 lines in 1 file changed: 4 ins; 0 del; 49 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: RFR: 8314147: Updated the PhongMaterial documentation [v2]
On Thu, 22 Feb 2024 22:18:38 GMT, Andy Goryachev wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update copyright year > > modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java > line 848: > >> 846: + ", specularPower=" + getSpecularPower() + ", >> diffuseMap=" + getDiffuseMap() + ", specularMap=" >> 847: + getSpecularMap() + ", bumpMap=" + getBumpMap() + ", >> selfIlluminationMap=" + getSelfIlluminationMap() >> 848: + "]"; > > could we restore the formatting please? > also, the first `+` is unnecessary Weird, I specifically reverted the auto-formatting, not sure how it got in. Will revert. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r157027
Re: RFR: 8314147: Updated the PhongMaterial documentation [v2]
On Thu, 22 Feb 2024 22:19:27 GMT, Nir Lisker wrote: >> But it's still third-party content and cannot go in without approval. > > What would an approval require? To add to this, such approval would be unlikely, and I am not in favor of making the request. Perhaps you can find suitable images already in the repo? Here is one that might work: https://github.com/openjdk/jfx/tree/e0b88bc7cfede46afe28cbb4a2e333df933b5100/apps/toys/FX8-3DFeatures/src/resources - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r159761
Re: RFR: 8314147: Updated the PhongMaterial documentation [v2]
On Thu, 22 Feb 2024 22:17:27 GMT, Nir Lisker 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: > > Update copyright year modules/javafx.graphics/src/main/java/javafx/scene/paint/PhongMaterial.java line 848: > 846: + ", specularPower=" + getSpecularPower() + ", > diffuseMap=" + getDiffuseMap() + ", specularMap=" > 847: + getSpecularMap() + ", bumpMap=" + getBumpMap() + ", > selfIlluminationMap=" + getSelfIlluminationMap() > 848: + "]"; could we restore the formatting please? also, the first `+` is unnecessary - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r150870
Re: RFR: 8314147: Updated the PhongMaterial documentation [v2]
On Thu, 22 Feb 2024 22:13:33 GMT, Kevin Rushforth wrote: >> No, it's a standard in image processing: https://en.wikipedia.org/wiki/Lenna. > > But it's still third-party content and cannot go in without approval. What would an approval require? - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r152472
Re: RFR: 8314147: Updated the PhongMaterial documentation [v2]
On Thu, 22 Feb 2024 22:10:35 GMT, Nir Lisker wrote: >> modules/javafx.graphics/src/main/docs/javafx/scene/paint/doc-files/color and >> map/map.png line 1: >> >>> (failed to retrieve contents of file, check the PR for context) >> Is this a licensed image? > > No, it's a standard in image processing: https://en.wikipedia.org/wiki/Lenna. But it's still third-party content and cannot go in without approval. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r146926
Re: RFR: 8314147: Updated the PhongMaterial documentation [v2]
On Thu, 22 Feb 2024 20:49:20 GMT, Andy Goryachev wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update copyright year > > modules/javafx.graphics/src/main/docs/javafx/scene/paint/doc-files/color and > map/map.png line 1: > >> (failed to retrieve contents of file, check the PR for context) > Is this a licensed image? No, it's a standard in image processing: https://en.wikipedia.org/wiki/Lenna. - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r144625
Re: RFR: 8314147: Updated the PhongMaterial documentation [v2]
> 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: Update copyright year - Changes: - all: https://git.openjdk.org/jfx/pull/1378/files - new: https://git.openjdk.org/jfx/pull/1378/files/b744540c..34bbc561 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1378=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1378=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
Re: RFR: 8314147: Updated the PhongMaterial documentation
On Thu, 22 Feb 2024 20:38:00 GMT, Nir Lisker 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. @lukostyra would be another good reviewer of this. - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1960405983
Re: RFR: 8314147: Updated the PhongMaterial documentation
On Thu, 22 Feb 2024 20:38:00 GMT, Nir Lisker 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. modules/javafx.graphics/src/main/docs/javafx/scene/paint/doc-files/color and map/map.png line 1: > (failed to retrieve contents of file, check the PR for context) Is this a licensed image? - PR Review Comment: https://git.openjdk.org/jfx/pull/1378#discussion_r1499917718
Re: RFR: 8314147: Updated the PhongMaterial documentation
On Thu, 22 Feb 2024 20:38:00 GMT, Nir Lisker 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. @kevinrushforth Please review this. Perhaps the second reviewer can be @jayathirthrao or @arapte. - PR Comment: https://git.openjdk.org/jfx/pull/1378#issuecomment-1960282365
RFR: 8314147: Updated the PhongMaterial documentation
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. - Commit messages: - Whitespace - Initial commit Changes: https://git.openjdk.org/jfx/pull/1378/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1378=00 Issue: https://bugs.openjdk.org/browse/JDK-8314147 Stats: 637 lines in 41 files changed: 525 ins; 17 del; 95 mod Patch: https://git.openjdk.org/jfx/pull/1378.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1378/head:pull/1378 PR: https://git.openjdk.org/jfx/pull/1378
