• Type: Bug
• Component: javafx
• Sub-Component: graphics
• Affected Version: 8

• Priority: P2
• Status: Closed
• Resolution: Fixed

• Submitted: 2013-10-24
• Updated: 2015-06-17
• Resolved: 2013-11-28

JDK 8
8Fixed

We have tests on it. So I consider it verified, as it is fixed some time ago. 31-12-2013 I've reorganized shaders a little so that the fact that specular power is in the alpha channel of the specular color constant is more clear. Changed variable name to sPower in GLSL shaders as well and added comments about luma conversion. 28-11-2013 Changeset: f2b2903b65fa Author: vadim Date: 2013-11-28 14:47 +0400 URL: http://hg.openjdk.java.net/openjfx/8/graphics/rt/rev/f2b2903b65fa 28-11-2013 OK. I see. I'm fine if you prefer to keep AUTO but please document its meaning. However TEXTURE is a better match to the pattern used in ES2PhongShader. I just found another place you might want to consider changing. This is in the 2 new GLSL files (specular_mix.frag and specular_auto.frag). Do you also want to use sPower instead of sLevel since you made that change in the hlsl shader (Mtl1PS.hlsl)? Can you add a comment that those constants vec3(0.299, 0.587, 0.114) are weights for NTSC conversion? The rest looks good to me. I have also verified that the GLSL shaders compile and run fine on Linux. Approved! 28-11-2013 Here is the updated webrev: http://cr.openjdk.java.net/~vadim/RT-33792/webrev.01/ diff from webrev.00 is here: http://cr.openjdk.java.net/~vadim/RT-33792/webrev.00to01/ 28-11-2013 Ok, I'll change it to the "texture", we are using 1-letter abbreviation in the generated shader names, so "map" is not suitable. Auto was chosen for the fact that we use NTSC luminance conversion for deriving specular power from the RGB component of the specular map. 28-11-2013 It looks good to me, too, although I didn't look at the shader changes or native D3D changes in detail. As soon as Chien gives the go, you can push it. 28-11-2013 This implementation looks good to me overall. Visual specular result of my simple test on d3d and es2 look identical. I only have one minor suggestion on naming: >There are now 4 specular shader variants - none, auto, color and mix >None is without map or color, auto is map only, color is color only, and mix is both map and color. Do you think it is better to change auto to map (or texture)? The "auto" is really confusing to me when I first read the code. I'm good to make the default specularPower to 32. It is clearer to spec. it out than bake it in the implementation. 28-11-2013 There is one minor nit with this particular equation, if specular power is low (about 1), the specular highlight can be seen outside diffuse spot. So actually the specular component should be taken into account only if dot product in the diffuse component is greater than zero. It's one more cmp instruction in the shader for the hlsl, but it seems I'm doing something wrong in glsl because it doesn't work for me there. 27-11-2013 Chien, Kevin, Please review the fix: http://cr.openjdk.java.net/~vadim/RT-33792/webrev.00/ Summary of changes: From the API point of view, the only change is new default of 32 for PhongMaterial.specularPower I've removed all references to textures with alpha channels (we might want to revisit this later when we get non-premultiplied images) The specular color and power are now always passed as the shader constant. There are now 4 specular shader variants - none, auto, color and mix None is without map or color, auto is map only, color is color only, and mix is both map and color. Power is always passed and can be used to control all variants. While updating es2 pipeline it occurred to me that most of the native part seems to be unused, we might want to take a closer look at the details) The test app is toys FX8-3DFeatures SpecularColorTestApp. I've taken my old CameraController from the closed apps. The controls are Alt+LMB for orbit, Alt+RMB for zoom, Alt+MMB for pan/track, there are more controls, that's basic ones. It's actually tracked by RT-26649, so this could be a fix for it also. I've not included bump mapping and more lights in the test app, but I've tested them. 27-11-2013 The actual equation should be: specular component = specularColor * specularMap * ( R . V ) ^ (specularMap * specularPower) This will result in more realistic lighting without adding too much complexity in the shader. 20-11-2013 Hi Vadim, I chatted with Kevin yesterday. We still prefer the first option as it is easier to implement (if I'm correct we have a working prototype for the es2 pipe already) and it supports ease of use for some common use cases. We can expand the API to add a separate specular power map in the future. 20-11-2013 I'm quoting the specular equation here: specular component = specularColor * specularMap * ( R . V ) ^ specularPower Ideally we want to control both color (and thus level) and power of specular highlights. This can be achieved by using RGB components of the specular map for the color, further multiplying by the specular color material property. And the specular power could be used from the alpha channel of the specular map. Since we don't have non-premultiplied images yet ( RT-17494 ) we would rather have specular power as the material property. Or we could expand the API and add separate specular power (gloss) map for this. Another option is to how it's done now in D3D and how 3ds max reference suggests: use specular map for _both_ specular color and power, so the equation could look like this: specular component = specularColor * specularMap * ( R . V ) ^ (specularMap * specularPower) So we need to pass down specularColor and specularPower properties from the material. 20-11-2013 How is this going? This bug is marked as critical. Vadim, do you have everything you need to implement this? 18-11-2013 Partial patch is attached to RT-32252 01-11-2013