Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix transforming surface normals #3722

Merged
merged 6 commits into from Mar 21, 2024
Merged

Conversation

singularitti
Copy link
Contributor

Description

Fixes #3702

As discussed in issue #3702, under the guidance of @kbarros, we made changes that solve the incorrect shading of ellipsoid surfaces of markers under light, displaying spherical reflections (mathematical formulae stated in #3702). The following figures show examples before and after this change. Please note that we are not experts in the GLSL language, so you may want to double-check whether this change may break some code. If you need changes to other backends, I am happy to help.

Testing code snippet:

using CairoMakie
# using GLMakie
# using WGLMakie
pts = Point3f[[0,0,0], [1,0,0]]
markersize = Vec3f[[0.5, 0.2, 0.5], [0.5, 0.2, 0.5]]
rotations = [qrotation(Vec3f(1,0,0), 0), qrotation(Vec3f(1,1,0), π/4)]
meshscatter(pts; markersize, rotations, color=:yellow)

CairoMakie

Before

cairomakie_before

After

cairomakie_after

GLMakie

Before

glmakie_before

After

glmakie_after

WGLMakie

Before

wglmakie_before

After

wglmakie_after

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@ffreyer
Copy link
Collaborator

ffreyer commented Mar 20, 2024

Thanks for the pr.

Since none of the refimg tests are failing we should add a new one here. It would probably be good to use more extreme shading like color=:white, diffuse = Vec3f(-2, 0, 4), specular = Vec3f(4, 0, -2) to make the differences more obvious. Otherwise the test image comparison may struggle to pick up on it. The test would go in here: https://github.com/MakieOrg/Makie.jl/blob/master/ReferenceTests/src/tests/examples3d.jl

Other than that this just needs an entry in https://github.com/MakieOrg/Makie.jl/blob/master/CHANGELOG.md

@jkrumbiegel
Copy link
Collaborator

Otherwise the test image comparison may struggle to pick up on it.

Thankfully the image comparison is a lot more sensitive nowadays, but it's still good practice to make failure modes as obvious as possible

@singularitti
Copy link
Contributor Author

Done! Please have a look @ffreyer @jkrumbiegel:

Before fixing rotations

cairomakie_before

After fixing rotations

CairoMakie

cairomakie_after

GLMakie

glmakie_after

WGLMakie

wglmakie_after

@SimonDanisch SimonDanisch merged commit d50dad0 into MakieOrg:master Mar 21, 2024
33 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Surface normals not properly transformed by markersize scaling.
4 participants