Repository navigation
Extend heatmap into 3D surface with framing and height scale - #2081
NAThompson wants to merge 1 commit into
Conversation
a9cfc97 to
2877318
Compare
loichuder
left a comment
loichuder
left a comment
There was a problem hiding this comment.
Thanks a lot, this looks fantastic!
I did a first high-level quick round of review so I plan to iterate smoothly afterwards
| export const NaNHoles = meta.story({ | ||
| args: { |
There was a problem hiding this comment.
| export const NaNHoles = meta.story({ | |
| args: { | |
| export const NaNHoles = meta.story({ | |
| name: 'With NaN' | |
| args: { |
The title displayed weirdly on my end
There was a problem hiding this comment.
Done, thanks! The story now uses name: 'With NaN'.
| {shading ? ( | ||
| <meshLambertMaterial key="lit" vertexColors side={DoubleSide} /> | ||
| ) : ( | ||
| <meshBasicMaterial key="unlit" vertexColors side={DoubleSide} /> | ||
| )} |
There was a problem hiding this comment.
Could you explain in what situations the shading is needed? On the tests I did, it seems to only lighten slightly the surface.
There was a problem hiding this comment.
I noticed this as well-setting my claude off to think on it.
There was a problem hiding this comment.
Good question. I checked pixel by pixel and shading only ever darkens: flat areas keep their colour (Viridis(0.5) = (33,145,140) off vs (32,143,138) on). My guess is that the lightening you saw was the toggle going from on to off.
The light is nearly overhead so that flat areas keep their exact colours, which makes the effect tiny on gentle relief, where it would matter most. An oblique light makes ripples readable but darkens flat areas, so they no longer match the colour bar. Left to right: off, on (this PR), oblique light (experiment, not in this PR):
When colour already shows height, shading adds little:
Since that's a design trade-off, I'd suggest removing shading from this PR and proposing it separately. Does that work for you?
There was a problem hiding this comment.
Thanks for the sum-up !
Sure, let's remove the shading for now and discuss it later 🙂
There was a problem hiding this comment.
Done: shading is removed (plain vertex colours, no lights or normals). #2086 is rebased on top.
| const invalidate = useThree((state) => state.invalidate); | ||
|
|
||
| const geometry = useMemo( | ||
| () => new SurfaceMeshGeometry({ dataArray, rows, cols }), |
There was a problem hiding this comment.
Could we not have the geometry computation kept in SurfaceMeshGeometry?
There was a problem hiding this comment.
Good point. The geometry is back in SurfaceMeshGeometry, following the useUpdateGeometry pattern: update() builds positions, holes and normals, and a separate updateColors() means changing the colour map doesn't rebuild positions. I also fixed the second triangle's winding, which was clockwise on main and would flip normals for lighting.
There was a problem hiding this comment.
Could you move the axis creation out of the PR? That means, just having the surface without axes/grid for now.
Don't get me wrong, I think it is really super cool but I would like to review it separately 😉
There was a problem hiding this comment.
@loichuder : Splitting them up for ease of review.
There was a problem hiding this comment.
Axes, grid and tick labels are now in #2086, stacked on this PR.
2877318 to
fd68f4a
Compare
NAThompson
changed the title
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fd68f4a to
ae6ea81
Compare
This branch has not been deployed
2 participants
Closes #1292, closes #1306. Adds a "3D" toggle to the Heatmap toolbar that extends the current slice into a surface sharing the heatmap's colour map, domain, scale and axes, with a height control and auto-framing (axes and grid in #2086). Removes the experimental Surface tab; breaking for the lib 18 beta (
showPoints,SurfaceMeshGeometryremoved).🤖 Generated with Claude Code