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

Improve nan handling in surface plots #4735

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Jan 17, 2025

Description

This started from investigating why the "heatmaps & surface" test produces different nan colors in GLMakie and WGLMakie.

GLMakie WGLMakie
GLMakie_heatmaps   surface WGLMakie_heatmaps   surface

As far as I understand Three.js filters nan values from interleaved buffers. So if any positional argument is nan in WGLMakie, the instances/rects that include that position do not get rendered. That seems like the correct behavior to me. If any part of the position is nan, then we cannot draw a rect with it. I believe that is also the conclusion we came to with #2598 at Makiecon.

So it's GLMakie that's actually wrong. Turns out we never really dealt with the problem there on a position level. The rects are still drawn, it's just that their color is nan and therefore uses nan_color which is transparent by default.

Changes/TODO:

  • clean up the instance + vertex -> 2D index -> uv code
  • move interpolated position & normal generation code into shader to improve maintainability
  • add checks to "discard" rects if any x, y, z are nan
  • extend "Surface with NaN points" refimg to test nans in x, y, z, and color individually
  • (CairoMakie) discard nan rects already happens on position basis and would require reworking color handling/surface rendering pipeline for colors, which seems like more work than it's worth
  • activate "heatmaps & surface" in CairoMakie
  • consider adding a slight bias to uvs to make sure color sampling only considers its own rect (will this create seams? what about textures?)
  • consider differentiating per-vertex colors from textures when calculating uvs (by size?) (so that images use 0..1 range and intensities use pixel centered range)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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.

@MakieBot
Copy link
Collaborator

MakieBot commented Jan 17, 2025

Benchmark Results

SHA: 0afcb4cf4750ef1a45ac71478cb594dbcdbf9943

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

@SimonDanisch
Copy link
Member

How do we draw the position if it's Nan? This looks to me like the position comes from the x/y range, which then isn't Nan?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 18, 2025

Surface always includes Z. On master Z = nan gets replaced by 0 in GLMakie and doesn't produce a quad in WGLMakie. X and Y are not treated in GLMakie, so I assume it's undefined behavior. For me triangles with NaN in X or Y are not drawn and lighting breaks around them due to nan normals. In WGLMakie they get excluded.

CairoMakie is basically the same as WGLMakie, filtering nans in positions in surface_to_mesh. It does act differently with nan in color though, because it interpolates colors instead of intensity values

The refimg I adjusted looks like this for GLMakie: (Showing X/Y/Z nans for vector and Matrix inputs, and then color nans with nan_color transparent and red)

master pr
grafik grafik

@ffreyer
Copy link
Collaborator Author

ffreyer commented Jan 20, 2025

I wonder if we should view the lines of lowclip/highclip colors in "heatmaps & surface" as correct. I.e.:

data = hcat(LinRange(2, 3, 4), LinRange(2, 2.5, 4), LinRange(2.5, 3, 4), [1, NaN, NaN, 5])
surface(
    zeros(size(data)),
    color = data,
    colorrange = (2, 3),
    highclip = :red,
    lowclip = :black,
    nan_color = (:green, 0.5),
    shading = NoShading,
)

image

OpenGL is not mixing in the nan values there, because we're going straight from the color =1 (5) vertex to the color = 2.5 (3) vertex. While the test looks weird like this, it does seem reasonable to me.

@ffreyer ffreyer marked this pull request as ready for review January 20, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

3 participants