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
