shininess is not working in WGLMakie

[gdkrmr]

The following code produces a Javascript error in WGLMakie:

julia> using WGLMakie

julia> using FileIO

julia> brain = load(assetpath("brain.stl"));

julia> mesh(
           brain,
           color = [tri[1][2] for tri in brain for i in 1:3],
           colormap = Reverse(:Spectral),
           shininess = 64.0
       );

julia> mesh(
           brain,
           color = [tri[1][2] for tri in brain for i in 1:3],
           colormap = Reverse(:Spectral),
           shininess = 64.0
       )
┌ Warning: Error in Javascript: THREE.WebGLProgram: Shader Error No error - VALIDATE_STATUS false
│ 
│ Program Info Log:
│ Must have a compiled vertex shader attached:
│ SHADER_INFO_LOG:
│ ERROR: 0:43: 'double' : Illegal use of reserved word
│ ERROR: 0:43: 'double' : syntax error
│ VERTEX
│ 
│ ERROR: 0:43: 'double' : Illegal use of reserved word
│ ERROR: 0:43: 'double' : syntax error
│ 
│   38: vec3 get_ambient(){return ambient;}
│   39: uniform mat3 normalmatrix;
│   40: mat3 get_normalmatrix(){return normalmatrix;}
│   41: uniform vec4 nan_color;
│   42: vec4 get_nan_color(){return nan_color;}
│ > 43: uniform double shininess;
│   44: double get_shininess(){return shininess;}
...

[SimonDanisch]

Looks like we don't convert to Float32 automatically... For a workaround you could try 64f0

[gdkrmr]

that works, thanks!

[abmnmj]

Testing with the solution suggested in this discourse thread on type issues with shininess, that is, adding

convert_attribute(value, ::key"shininess") = Float32(value)

to conversions.jl (for example where convert_attribute(value, ::key"backlight") = Float32(value) is defined) seems to solve also this issue (tried with the code above and it produces the desired "brain plot" in the browser). So, it would seem that it's related to the same conversion problem.

In the Discourse thread it was suggested that I could make a PR with the above solution, but it seems I don't have the skill set for achieving this. Also, I could not find any example tests that I could base new tests on.

So, unfortunately I simply add to the present issue to have the (potential) solution documented.

[asinghvi17]

That's a temporary patch though. The real solution is that the shininess values should just have el32convert called on them in float32 backends?

[ffreyer]

No, it should just have a convert_attribute. The lighting math combines normalized normals, normalized directions and lighting attributes, so everything revolves around a 0..1 value range. There is no benefit in having extra precision or range there.

In the Discourse thread it was suggested that I could make a PR with the above solution, but it seems I don't have the skill set for achieving this. Also, I could not find any example tests that I could base new tests on.

It's fine if you just make a pr with the changes and ask for help with tests, changelog, etc.

I would add another row to this test and try all the lighting attributes with Float64 types there. I would also try to modify the attributes so it looks signifcantly different from the defaults and the case above, so we can verify that it isn't skipping anything

Makie.jl/ReferenceTests/src/tests/examples3d.jl

Lines 150 to 162 in eb1ad5b

	@reference_test "Basic Shading" begin
	f = Figure(size = (500, 300))
	# see PR #3722
	pts = Point3f[[0, 0, 0], [1, 0, 0]]
	markersize = Vec3f[[0.5, 0.2, 0.5], [0.5, 0.2, 0.5]]
	rotation = [qrotation(Vec3f(1, 0, 0), 0), qrotation(Vec3f(1, 1, 0), π / 4)]
	meshscatter(f[1, 1], pts; markersize, rotation, color = :white, diffuse = Vec3f(-2, 0, 4), specular = Vec3f(4, 0, -2))
	mesh(f[1, 2], Sphere(Point3f(0), 1.0f0), color = :orange, shading = NoShading)
	f
	end