add mesh colorfield visualization support #180
Conversation
jf---
force-pushed
the
jf/scalarfields
branch
from
f196ca8 to
839b5cd
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds mesh colorfield visualization support to the visualize_slicer function, enabling users to visualize meshes colored by vertex or face attributes. The implementation includes a fix to the HeatGeodesicSolver to properly accept mesh data as a (V, F) tuple format, and updates example files to demonstrate the new colorfield visualization capabilities.
- Adds auto-detection logic to distinguish between vertex and face attributes for mesh coloring
- Uses the viridis colormap from matplotlib for scalar field visualization
- Fixes HeatGeodesicSolver initialization to accept mesh in (V, F) tuple format instead of direct Mesh object
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/compas_slicer/visualization/visualization.py | Adds mesh_colorfield parameter with auto-detection logic for vertex/face attributes and viridis colormap application |
| src/compas_slicer/pre_processing/preprocessing_utils/geodesics.py | Converts mesh to (V, F) tuple format before passing to HeatGeodesicSolver |
| examples/6_attributes_transfer/example_6_attributes_transfer.py | Demonstrates mesh visualization with 'overhang' face attribute coloring |
| examples/2_curved_slicing/ex2_curved_slicing.py | Demonstrates mesh visualization with 'scalar_field' vertex attribute coloring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scalars = {v: mesh.vertex_attribute(v, mesh_colorfield) for v in mesh.vertices()} | ||
| smin, smax = min(scalars.values()), max(scalars.values()) |
There was a problem hiding this comment.
When all scalar values are identical (smin == smax), the colormap will encounter a division by zero or produce undefined behavior when normalizing values. Consider handling this edge case by either using a single color or adding a small epsilon to avoid division issues.
| ) | ||
| else: |
There was a problem hiding this comment.
When all scalar values are identical (smin == smax), the colormap will encounter a division by zero or produce undefined behavior when normalizing values. Consider handling this edge case by either using a single color or adding a small epsilon to avoid division issues.
| # Add mesh if provided, colored by vertex or face attribute | ||
| if mesh and show_mesh: | ||
| viewer.scene.add(mesh, opacity=mesh_opacity) | ||
| cmap = ColorMap.from_mpl("viridis") | ||
|
|
||
| # check if colorfield is vertex or face attribute | ||
| first_vertex = next(mesh.vertices()) | ||
| if mesh.vertex_attribute(first_vertex, mesh_colorfield) is not None: | ||
| scalars = {v: mesh.vertex_attribute(v, mesh_colorfield) for v in mesh.vertices()} | ||
| smin, smax = min(scalars.values()), max(scalars.values()) | ||
| vertexcolor = {v: cmap(s, minval=smin, maxval=smax) for v, s in scalars.items()} | ||
| viewer.scene.add( | ||
| mesh, vertexcolor=vertexcolor, opacity=mesh_opacity, use_vertexcolors=True, show_lines=False | ||
| ) | ||
| else: | ||
| scalars = {f: mesh.face_attribute(f, mesh_colorfield) for f in mesh.faces()} |
There was a problem hiding this comment.
The new mesh colorfield visualization feature lacks test coverage for edge cases such as invalid attribute names, empty meshes, or uniform scalar values. While the feature is exercised in example integration tests, consider adding unit tests that specifically verify error handling and edge case behavior for the colorfield parameter.
| Opacity for mesh display (0-1). | ||
| mesh_colorfield : str |
There was a problem hiding this comment.
The documentation states that mesh_colorfield is a "Vertex attribute name" but the implementation actually supports both vertex and face attributes through auto-detection. The documentation should reflect this dual capability to avoid confusion.
|
|
||
| # check if colorfield is vertex or face attribute | ||
| first_vertex = next(mesh.vertices()) | ||
| if mesh.vertex_attribute(first_vertex, mesh_colorfield) is not None: | ||
| scalars = {v: mesh.vertex_attribute(v, mesh_colorfield) for v in mesh.vertices()} | ||
| smin, smax = min(scalars.values()), max(scalars.values()) | ||
| vertexcolor = {v: cmap(s, minval=smin, maxval=smax) for v, s in scalars.items()} | ||
| viewer.scene.add( | ||
| mesh, vertexcolor=vertexcolor, opacity=mesh_opacity, use_vertexcolors=True, show_lines=False | ||
| ) | ||
| else: | ||
| scalars = {f: mesh.face_attribute(f, mesh_colorfield) for f in mesh.faces()} |
There was a problem hiding this comment.
There's no error handling when the specified mesh_colorfield doesn't exist as either a vertex or face attribute. If a user provides an invalid attribute name, the code will fail at line 73 when trying to retrieve face attributes without a clear error message. Consider adding explicit validation and providing a helpful error message when the attribute is not found in either vertices or faces.
| if mesh.vertex_attribute(first_vertex, mesh_colorfield) is not None: | ||
| scalars = {v: mesh.vertex_attribute(v, mesh_colorfield) for v in mesh.vertices()} |
There was a problem hiding this comment.
If the mesh has no vertices or faces, the min and max operations on empty dictionaries will raise a ValueError. Consider adding a check to ensure scalars is not empty before computing smin and smax, or handle the case where the mesh might be empty.
| mesh, vertexcolor=vertexcolor, opacity=mesh_opacity, use_vertexcolors=True, show_lines=False | ||
| ) |
There was a problem hiding this comment.
If the mesh has no faces, the min and max operations on empty dictionaries will raise a ValueError. Consider adding a check to ensure scalars is not empty before computing smin and smax, or handle the case where the mesh might be empty.
- visualize_slicer now accepts mesh_colorfield param for vertex/face attribute coloring - auto-detect vertex vs face attributes, use viridis colormap - fix HeatGeodesicSolver to accept (V, F) tuple - update examples to use scalar_field/overhang coloring closes compas-dev#179
jf---
force-pushed
the
jf/scalarfields
branch
2 times, most recently
from
2c574e1 to
a19eff4
Compare
|
hi @tomvanmele, there's not too many contributors overwatching this repo, the 4 eyes principle is perhaps a bit much? |
|
i have enabled the "merge without waiting for ..." option for you. |
|
would in any case be good to try to get your tests to pass before merging though, in my opinion... |
|
Hi @tomvanmele tha ks for sharing yeah for sure regressions on test passing would be the bloody limit. |
3 participants
Summary
closes #179
Note: This PR depends on #182 (CI fixes) being merged first.