Headless renderer #52
Conversation
This class offers the option to render without a gui. Works analogously to the Visualizer class. Currently only a static camera perspective is implemented that can be defined with the view_matrix.
from Headless_Visualizer to Headless_Renderer
SamTov
left a comment
SamTov
left a comment
There was a problem hiding this comment.
I would like to have the camera options included in this PR if possible. That way it all works from day one. We also need a test.
…ng process.
Following Camera Options are now available
Can be used as a static camera. Needs either a view matrix or the three vectors eye, center and up.
↑ (up)
|
| camera looks from the eye to the center
|
●------>●
eye center
- eye: The position of the camera
- center: The position at which the camera is looking.
- up: This vector defines the orientation of the camera.
Can be used as
```
base_camera = znvis.Camera(center, eye, up)
```
Also provides methods that allow
- ```get_view_matrix_from_particle_positions(first_frame_positions)``` focusing on the center of the simulation box
- ```get_view_matrix_from_box_size(box_size)``` focusing on the center of all of particles in the simulation.
- ```get_minimal_view_matrix(box_size, rendering_resolution)``` experimental method that should find the camera setting that minimizes dead zones in your rendered image, where nothing happens.
Follows a particle during a full simulation shifted from the particle via a defined ```camera_particle_vector```.
Can be instantiated as for example
```
vis.ParticleFollowingCamera(particle_positions=positions_of_particle_1,
camera_particle_vector=np.array([0,0,50]),
camera_up_vector=np.array([0,1,0]))
```
By passing a keyframe camera to the visualizer, you can manually stick camera positions to a certain frame.
The keyframe camera will then be able to interpolate a smooth camera trajectory between all your chosen frames.
This is exported to your output path from where you can pass it to the headless visualizer that will then render your desired trajectory.
At the moment you will need to use two scripts or this workaround
```
import subprocess
print("Starting interactive visualization...")
p1 = subprocess.Popen(["python", "vis1.py")
p1.wait()
print("Starting rendering process...")
subprocess.run(["python", "vis2.py")
```
vis1.py should include:
```
keyframe_camera = vis.KeyframeCamera()
visualizer = vis.Visualizer(particles=particle_list,
output_folder=out_folder,
keyframe_camera=keyframe_camera)
visualizer.run_visualization()
```
vis2.py should include :
```
keyframe_camera = vis.KeyframeCamera(view_matrices_path='./output_folder/interpolated_view_matrices.npy')
visualizer2 = vis.Headless_Visualizer(particles=particle_list,
output_folder=out_folder,
camera=keyframe_camera,)
visualizer2.render_visualization()
```
Allows defining a subclass ```trajectory``` object, which provides a trajectory the camera should follow through a scene.
Can be used as
```
trajectory_camera = vis.TrajectoryCamera(total_frames=number_of_frames,
trajectory=trajectory_object)
```
Provides methods that return center, eye and up vector depending on the frame index depending on the total number of frames to create smooth trajectories the camera can follow.
At the moment the following trajectories are implemented
- Circular Trajectory
Creates a circular trajectory with ```radius```around the ```axis```-axis focusing on the ```center```. Optionally, the trajectory can be looped, ping-ponged or limited on a certain angle range. The ```frames_per_rotation``` variable defines the speed of the rotation.
An example would be:
```
circular_trajectory = vis.CircularTrajectory(total_frames=number_of_frames,
center=np.array([Lx/2, Ly/2, 0]),
radius=200,
frames_per_rotation=20,
axis='y',
clockwise=True,
loop=False,
angle_range=(-np.pi/4, np.pi/4),
ping_pong=True)
```
- Zooming Trajectory
Creates a zooming in or zooming out trajectory from ```center``` and ```initial_eye``` based on the sign of ```zoom_distance```. The ```frames_per_zoom``` argument defines the ratio during which zooming happens.
An example would be:
```
zoom_trajectory = vis.ZoomingTrajectory(total_frames=number_of_frames,
center=np.array([Lx/2, Ly/2, 0]),
initial_eye=np.array([Lx/2, Ly/2, 200]),
zoom_distance = 150,
frames_per_zoom=1.0)
```
Introduced new Camera ClassThis update enhances control options of the camera during the rendering process. Following Camera Options are now available BaseCameraCan be used as a static camera. Needs either a view matrix or the three vectors eye, center and up.
Can be used as Also provides methods that allow
Particle Following CameraFollows a particle during a full simulation shifted from the particle via a defined Keyframe CameraBy passing a keyframe camera to the visualizer, you can manually stick camera positions to a certain frame. At the moment you will need to use two scripts or this workaround vis1.py should include: vis2.py should include : Trajectory CameraAllows defining a subclass TrajectoriesProvides methods that return center, eye and up vector depending on the frame index depending on the total number of frames to create smooth trajectories the camera can follow.
|
|
I forgot to adapt the tests to some changes I made in the keyframe_camera.py... Oops. |
|
Can we clean up the UI. Why do we need two scripts? |
SamTov
left a comment
SamTov
left a comment
There was a problem hiding this comment.
Please take a look at the comments.
- Changed the init structure - Changed strings in inits to cls.__names__ - Reorganized the BaseCamera class and the StaticCamera - Moved `self.view_matrices_dict` initialization into the init of the keyframe camera - Expanded the docstring about the interpolation method - Clarified the `ParticleFollowingCamera` class
Also added the related tests.
WalkthroughAdds a cameras subsystem (camera classes and trajectories), base and headless visualizers, a VideoManager, per-frame particle mesh handling, many unit tests, small .gitignore tweaks, and expands package exports to expose cameras and Headless_Visualizer. All changes are additions and test coverage increases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Visualizer as Visualizer (GUI/Headless)
participant Camera as Camera (Static/Trajectory/Keyframe/ParticleFollowing)
participant Trajectory as Trajectory (Circular/Zooming)
participant Renderer as Renderer
rect #E8F4FF
User->>Visualizer: configure(particles, camera, output)
Visualizer->>Camera: get_view_matrix(0)
alt Trajectory-backed Camera
Camera->>Trajectory: get_center_eye_up(0)
Trajectory-->>Camera: center, eye, up
Camera->>Camera: look_at(center, eye, up)
else Static / ParticleFollowing
Camera->>Camera: compute/return view_matrix
end
end
rect #F7FFF0
loop per frame i
Visualizer->>Camera: get_view_matrix(i)
alt Trajectory-backed
Camera->>Trajectory: get_center_eye_up(i)
Trajectory-->>Camera: center, eye, up
Camera->>Camera: look_at(...)
end
Visualizer->>Renderer: render(frame i, view_matrix, meshes)
Renderer-->>Visualizer: frame saved
end
end
sequenceDiagram
autonumber
participant User
participant Visualizer as Visualizer (GUI)
participant KeyCam as KeyframeCamera
participant Disk as Disk
User->>Visualizer: Add/Remove/Interpolate keyframes
Visualizer->>KeyCam: add_view_matrix(idx, M) / remove_view_matrix(idx)
Visualizer->>KeyCam: interpolate_and_export_view_matrices()
KeyCam->>KeyCam: sort + interpolate (+ optional SVD smoothing)
KeyCam->>Disk: save interpolated .npy
Disk-->>KeyCam: confirm
Visualizer->>KeyCam: get_view_matrix(i)
KeyCam-->>Visualizer: interpolated M[i]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
znvis/cameras/trajectories/__init__.py (1)
24-32: Re-exports look good.Full import paths and all via cls.name are fine here.
🧹 Nitpick comments (27)
.gitignore (2)
142-143: Good addition; consider common OS artifacts tooAdding .DS_Store is good. Optionally also ignore Windows and AppleDouble artifacts.
# macOS system files .DS_Store +Thumbs.db +._*
140-141: No .mp4 files currently tracked; blanket ignore is safeNo tracked
.mp4files are present, so keeping the blanket*.mp4ignore is fine. If you later add curated videos (e.g., indocs/orexamples/), consider an allowlist:- *.mp4 + # Generated video outputs + *.mp4 + # Uncomment to allow curated samples + # !docs/**/media/**/*.mp4 + # !examples/**/assets/**/*.mp4znvis/particle/particle.py (2)
124-133: Use warnings and single-string exceptions; avoid multi-arg ValueError (TRY003)
- Replace print-based warnings with warnings.warn.
- Combine multi-string ValueError messages into a single string (Ruff TRY003).
As per coding guidelines and Ruff TRY003, apply:
@@ - if self.position.size == 0: - raise ValueError( - "The provided position array is empty.", - "Please provide a valid position array.", - ) + if self.position.size == 0: + raise ValueError( + "The provided position array is empty. Please provide a valid position array." + ) @@ - print( - "-------\nWARNING: The provided director array is empty.", - "Setting to None.\n-------", - ) + warnings.warn( + "The provided director array is empty. Setting to None.", + stacklevel=2, + ) @@ - print( - "-------\nWARNING: The provided position array has an ", - "unexpected shape. Using the first entry as the static array.", - "\n-------", - ) + warnings.warn( + "The provided position array has an unexpected shape. Using the first entry as the static array.", + stacklevel=2, + ) @@ - print( - "-------\nWARNING: The provided director array has an ", - "unexpected shape. Using the first entry as the static ", - "array.\n-------", - ) + warnings.warn( + "The provided director array has an unexpected shape. Using the first entry as the static array.", + stacklevel=2, + ) @@ - raise ValueError( - "The provided director data contains NaN ", - f"values at time step {i}.", - ) + raise ValueError( + f"The provided director data contains NaN values at time step {i}." + )Add import:
# top of file import warningsAlso applies to: 143-151, 155-159, 175-178
75-75: Type of mesh_list mismatches stored objectsmesh_list contains open3d meshes, not Mesh wrappers. Adjust the annotation to avoid confusion.
- mesh_list: typing.List[Mesh] = None + mesh_list: typing.List[object] = None # stores o3d.geometry.TriangleMeshCI/unit_tests/particle/test_particle.py (3)
95-105: Use unittest asserts instead of bare assertSwitch to unittest assertions for clearer failures.
- self.configuration_warning_particle.construct_mesh_list() - assert self.configuration_warning_particle.position[0].shape == (2, 3) - assert self.configuration_warning_particle.director[0].shape == (2, 3) + self.configuration_warning_particle.construct_mesh_list() + self.assertEqual(self.configuration_warning_particle.position[0].shape, (2, 3)) + self.assertEqual(self.configuration_warning_particle.director[0].shape, (2, 3))
117-119: Prefer simple equality for shape checksUsing numpy assert on tuples is unnecessary.
- np.testing.assert_array_equal(self.particle.position[0].shape, (2, 3)) + self.assertEqual(self.particle.position[0].shape, (2, 3))
158-162: Clarify static post-construction expectationsAfter construct_mesh_list, static positions become a single-frame list.
- self.assertEqual(np.asarray(self.static_particle.position)[0].shape, (2, 3)) + self.assertEqual(len(self.static_particle.position), 1) + self.assertEqual(self.static_particle.position[0].shape, (2, 3))CI/unit_tests/camera/test_base_trajectory_camera.py (1)
38-43: Assert something about get_view_matrix resultAdd a basic shape assertion to ensure output validity.
- # Test the get_view_matrix method with a valid frame index - self.rotating_camera.get_view_matrix(0) + # Test the get_view_matrix method with a valid frame index + vm = self.rotating_camera.get_view_matrix(0) + self.assertEqual(vm.shape, (4, 4))CI/unit_tests/camera/camera_trajectories/test_zooming_trajectory.py (2)
16-18: Fix misleading docstring in setUpClass.This tests ZoomingTrajectory, not an "InterpolationCamera".
66-73: Use unittest assertions instead of bare asserts.Bare asserts give poorer failure messages and can be optimized away.
- assert np.allclose( - self.camera_trajectory.get_center_eye_up(0), - self.camera_trajectory.get_center_eye_up(-1), - ) - assert np.allclose( - self.camera_trajectory.get_center_eye_up(20), - self.camera_trajectory.get_center_eye_up(10), - ) + self.assertTrue( + np.allclose( + self.camera_trajectory.get_center_eye_up(0), + self.camera_trajectory.get_center_eye_up(-1), + ) + ) + self.assertTrue( + np.allclose( + self.camera_trajectory.get_center_eye_up(20), + self.camera_trajectory.get_center_eye_up(10), + ) + )CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py (1)
17-18: Correct class names in docstrings.References to InterpolationCamera/KeyframeCamera should be BaseTrajectory.
Also applies to: 23-24
znvis/cameras/static_camera.py (1)
24-26: Type hint Optional[int] and silence unused arg.Align signature with Optional[int] and rename unused param; keeps interface while satisfying linter.
import numpy as np +from typing import Optional from znvis.cameras.base_camera import BaseCamera @@ - def get_view_matrix(self, frame_index: int = None) -> np.ndarray: + def get_view_matrix(self, _frame_index: Optional[int] = None) -> np.ndarray: """ Get the current view matrix of the camera. @@ - return self.view_matrix + return self.view_matrixOptional: shorten the ValueError message to appease TRY003. As per static analysis hints.
Also applies to: 73-87
CI/unit_tests/camera/test_keyframe_camera.py (2)
68-71: Prefer unittest assertions over bare asserts.Improves failure output and avoids -O optimizations skipping them.
- assert len(view_matrices_dict) == 3 - assert len(interpolated_view_matrices) == 200 - assert np.allclose(view_matrices_dict[0], interpolated_view_matrices[0]) + self.assertEqual(len(view_matrices_dict), 3) + self.assertEqual(len(interpolated_view_matrices), 200) + self.assertTrue(np.allclose(view_matrices_dict[0], interpolated_view_matrices[0]))
84-84: Remove stray print in test.Avoids noisy test output.
- print(self.keyframe_camera.get_view_matrix(0))znvis/cameras/trajectory_camera.py (1)
24-28: Use Optional[int] for frame_index.Match optional default with type hints and keep compatibility.
import numpy as np +from typing import Optional from znvis.cameras.base_camera import BaseCamera from znvis.cameras.trajectories.base_trajectory import BaseTrajectory @@ - def get_view_matrix(self, frame_index: int = None) -> np.ndarray: + def get_view_matrix(self, frame_index: Optional[int] = None) -> np.ndarray: center, eye, up = self.trajectory.get_center_eye_up(frame_index) self.view_matrix = self.look_at(center, eye, up) return self.view_matrixAs per static analysis hints.
Also applies to: 49-52
znvis/cameras/trajectories/base_trajectory.py (1)
24-28: Annotate optional frame_index properly.Use Optional[int] for default None and keep interface consistent across trajectory classes.
import numpy as np +from typing import Optional @@ - def get_center_eye_up(self, frame_index: int = None) -> tuple: + def get_center_eye_up(self, frame_index: Optional[int] = None) -> tuple: """ Provides the view matrix for the given frame index. """ raise NotImplementedError("This method should be overridden by subclasses.")As per static analysis hints.
Also applies to: 39-44
CI/unit_tests/camera/camera_trajectories/test_circular_trajectory.py (1)
193-205: Remove debug prints from testsUnnecessary stdout noise; tests should be quiet.
- print(i, frame_index) @@ - print(center, eye, up)CI/unit_tests/camera/test_static_camera.py (1)
163-169: Prefix unused variable to satisfy linters
centerisn’t used. Prefix with underscore.- center, eye, up = self.look_at_camera.get_center_eye_up_from_view_matrix( + _center, eye, up = self.look_at_camera.get_center_eye_up_from_view_matrix( view_matrix )znvis/visualizer/visualizer.py (2)
65-76: Use Optional typing for nullable paramsAlign hints with actual None defaults.
- vector_field: typing.List[znvis.VectorField] = None, + vector_field: typing.Optional[typing.List[znvis.VectorField]] = None, @@ - keyframe_camera: KeyframeCamera = None, + keyframe_camera: typing.Optional[KeyframeCamera] = None,
391-392: Be explicit about path type sent to renderer (optional)
os.path.joinaccepts PathLike, but passingstr(self.output_folder)avoids surprises.- save_dir=self.output_folder, + save_dir=str(self.output_folder),znvis/cameras/particle_following_camera.py (1)
47-49: Avoid numpy calls in default args; use None defaults and set inside.Prevents shared mutable defaults and satisfies Ruff B008.
Apply:
- particle_directions: np.ndarray = None, - camera_particle_vector: np.ndarray = np.array([0, 0, 20]), - camera_up_vector: np.ndarray = np.array([0, 1, 0]), + particle_directions: np.ndarray = None, + camera_particle_vector: np.ndarray = None, + camera_up_vector: np.ndarray = None,And initialize inside init (add below the docstring, before assignments):
+ if camera_particle_vector is None: + camera_particle_vector = np.array([0.0, 0.0, 20.0]) + if camera_up_vector is None: + camera_up_vector = np.array([0.0, 1.0, 0.0])Based on static analysis hints
znvis/cameras/keyframe_camera.py (1)
208-214: Interpolation duplicates the first keyframe twice.
interpolated_view_matrices.append(view_matrices[0])plusj=0in the loop adds the first matrix twice. This matches the current assertion but may be unintended. If you prefer single inclusion, startjfrom 1 or drop the explicit append.znvis/cameras/base_camera.py (2)
116-118: Avoid numpy calls in default args; set default inside.Prevents shared mutable defaults and satisfies Ruff B008.
Apply:
- def get_minimal_view_matrix( - box_size: np.ndarray, renderer_resolution: np.ndarray = np.array([4, 3]) - ) -> np.ndarray: + def get_minimal_view_matrix( + box_size: np.ndarray, renderer_resolution: np.ndarray = None + ) -> np.ndarray:Then initialize inside the function:
- aspect_ratio = renderer_resolution[0] / renderer_resolution[1] + if renderer_resolution is None: + renderer_resolution = np.array([4, 3]) + aspect_ratio = renderer_resolution[0] / renderer_resolution[1]Based on static analysis hints
190-204: Type hint and unused param polish.
frame_indexis unused here but part of the interface. ConsiderOptional[int]for clarity (RUF013) and documenting that subclasses may use it.znvis/visualizer/headless_visualizer.py (3)
68-71: Avoid mutable default and callable in defaults (B006/B008). Initialize inside.
samples_per_pixelis fine; changerenderer_resolutionandrenderer.Apply:
- renderer_resolution: list = [4096, 2160], + renderer_resolution: typing.Optional[list] = None, renderer_spp: int = 64, - renderer: Mitsuba = Mitsuba(), + renderer: typing.Optional[Mitsuba] = None,Initialize in init (add near other attribute setup):
+ if renderer_resolution is None: + renderer_resolution = [4096, 2160] + if renderer is None: + renderer = Mitsuba()Based on static analysis hints
132-141: Graceful fallback when no frames exist before movie creation.If rendering produced zero frames, indexing
images[0]will crash. Add an early return.Apply:
images = [f.as_posix() for f in self.frame_folder.glob("*.png")] # Sort images by number images = sorted(images, key=lambda s: int(re.search(r"\d+", s).group())) - single_frame = cv2.imread(images[0]) + if not images: + print("No frames found; skipping video creation.") + return + single_frame = cv2.imread(images[0])
109-111: BoundingBox instantiation may mis-handle instances.
self.bounding_box = bounding_box() if bounding_box else Nonetreats the argument as a class. If an instance is passed, this will raise. Consider accepting either:Apply:
- self.bounding_box = bounding_box() if bounding_box else None + if bounding_box is None: + self.bounding_box = None + else: + self.bounding_box = bounding_box() if isinstance(bounding_box, type) else bounding_box
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.gitignore(1 hunks)CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py(1 hunks)CI/unit_tests/camera/camera_trajectories/test_circular_trajectory.py(1 hunks)CI/unit_tests/camera/camera_trajectories/test_zooming_trajectory.py(1 hunks)CI/unit_tests/camera/test_base_trajectory_camera.py(1 hunks)CI/unit_tests/camera/test_keyframe_camera.py(1 hunks)CI/unit_tests/camera/test_particle_following_camera.py(1 hunks)CI/unit_tests/camera/test_static_camera.py(1 hunks)CI/unit_tests/particle/test_particle.py(2 hunks)znvis/__init__.py(3 hunks)znvis/cameras/__init__.py(1 hunks)znvis/cameras/base_camera.py(1 hunks)znvis/cameras/keyframe_camera.py(1 hunks)znvis/cameras/particle_following_camera.py(1 hunks)znvis/cameras/static_camera.py(1 hunks)znvis/cameras/trajectories/__init__.py(1 hunks)znvis/cameras/trajectories/base_trajectory.py(1 hunks)znvis/cameras/trajectories/circular_trajectory.py(1 hunks)znvis/cameras/trajectories/zooming_trajectory.py(1 hunks)znvis/cameras/trajectory_camera.py(1 hunks)znvis/particle/particle.py(3 hunks)znvis/visualizer/headless_visualizer.py(1 hunks)znvis/visualizer/visualizer.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (22)
CI/unit_tests/camera/test_keyframe_camera.py (1)
znvis/cameras/keyframe_camera.py (2)
interpolate_view_matrices(174-221)get_view_matrix(239-255)
CI/unit_tests/camera/test_particle_following_camera.py (1)
znvis/cameras/particle_following_camera.py (2)
ParticleFollowingCamera(29-119)get_view_matrix(88-119)
znvis/cameras/static_camera.py (1)
znvis/cameras/base_camera.py (3)
BaseCamera(27-256)look_at(152-188)get_view_matrix(190-204)
znvis/__init__.py (1)
znvis/visualizer/headless_visualizer.py (1)
Headless_Visualizer(45-324)
znvis/cameras/trajectory_camera.py (2)
znvis/cameras/base_camera.py (2)
get_view_matrix(190-204)look_at(152-188)znvis/cameras/trajectories/base_trajectory.py (2)
BaseTrajectory(27-77)get_center_eye_up(39-43)
CI/unit_tests/camera/camera_trajectories/test_circular_trajectory.py (1)
znvis/cameras/trajectories/circular_trajectory.py (3)
CircularTrajectory(29-190)get_loop_and_ping_pong_frame_index(99-140)get_center_eye_up(142-190)
znvis/cameras/trajectories/zooming_trajectory.py (2)
znvis/cameras/trajectories/base_trajectory.py (2)
BaseTrajectory(27-77)get_center_eye_up(39-43)znvis/cameras/trajectories/circular_trajectory.py (1)
get_center_eye_up(142-190)
CI/unit_tests/camera/camera_trajectories/test_zooming_trajectory.py (1)
znvis/cameras/trajectories/zooming_trajectory.py (2)
ZoomingTrajectory(29-114)get_center_eye_up(91-114)
znvis/cameras/particle_following_camera.py (2)
znvis/cameras/base_camera.py (2)
get_view_matrix(190-204)look_at(152-188)znvis/cameras/trajectory_camera.py (1)
get_view_matrix(49-52)
CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py (1)
znvis/cameras/trajectories/base_trajectory.py (3)
BaseTrajectory(27-77)get_center_eye_up(39-43)get_center_eye_up_from_view_matrix(46-77)
znvis/particle/particle.py (1)
znvis/particle/vector_field.py (1)
_create_mesh(68-96)
znvis/cameras/__init__.py (5)
znvis/cameras/base_camera.py (1)
BaseCamera(27-256)znvis/cameras/keyframe_camera.py (1)
KeyframeCamera(31-281)znvis/cameras/particle_following_camera.py (1)
ParticleFollowingCamera(29-119)znvis/cameras/static_camera.py (1)
StaticCamera(29-87)znvis/cameras/trajectory_camera.py (1)
TrajectoryCamera(30-52)
znvis/cameras/trajectories/__init__.py (3)
znvis/cameras/trajectories/base_trajectory.py (1)
BaseTrajectory(27-77)znvis/cameras/trajectories/circular_trajectory.py (1)
CircularTrajectory(29-190)znvis/cameras/trajectories/zooming_trajectory.py (1)
ZoomingTrajectory(29-114)
znvis/cameras/keyframe_camera.py (3)
znvis/cameras/base_camera.py (2)
BaseCamera(27-256)get_view_matrix(190-204)znvis/cameras/particle_following_camera.py (1)
get_view_matrix(88-119)znvis/cameras/trajectory_camera.py (1)
get_view_matrix(49-52)
znvis/visualizer/visualizer.py (2)
znvis/cameras/keyframe_camera.py (6)
KeyframeCamera(31-281)add_view_matrix(61-68)get_view_matrix(239-255)remove_view_matrix(79-91)interpolate_and_export_view_matrices(124-154)reset_view_matrix_progress(70-77)znvis/rendering/mitsuba.py (1)
render_mesh_objects(113-187)
CI/unit_tests/camera/test_base_trajectory_camera.py (2)
znvis/cameras/trajectory_camera.py (2)
TrajectoryCamera(30-52)get_view_matrix(49-52)znvis/cameras/trajectories/circular_trajectory.py (1)
CircularTrajectory(29-190)
CI/unit_tests/particle/test_particle.py (3)
znvis/material/material.py (1)
Material(31-61)znvis/particle/particle.py (2)
Particle(34-201)construct_mesh_list(108-201)znvis/mesh/sphere.py (1)
Sphere(32-51)
znvis/visualizer/headless_visualizer.py (6)
znvis/rendering/mitsuba.py (2)
Mitsuba(60-187)render_mesh_objects(113-187)znvis/particle/particle.py (1)
Particle(34-201)znvis/particle/vector_field.py (1)
VectorField(34-142)znvis/cameras/base_camera.py (1)
get_view_matrix(190-204)znvis/cameras/keyframe_camera.py (2)
load_view_matrices(156-172)get_view_matrix(239-255)znvis/visualizer/visualizer.py (9)
_create_movie(267-297)_initialize_particles(401-412)_initialize_vector_field(414-417)_record_trajectory(504-584)update_callable(511-516)update_callable(593-598)_update_particles(662-695)save_callable(518-563)save_callable(600-619)
znvis/cameras/trajectories/base_trajectory.py (2)
znvis/cameras/trajectories/circular_trajectory.py (1)
get_center_eye_up(142-190)znvis/cameras/trajectories/zooming_trajectory.py (1)
get_center_eye_up(91-114)
znvis/cameras/trajectories/circular_trajectory.py (2)
znvis/cameras/trajectories/base_trajectory.py (2)
BaseTrajectory(27-77)get_center_eye_up(39-43)znvis/cameras/trajectories/zooming_trajectory.py (1)
get_center_eye_up(91-114)
znvis/cameras/base_camera.py (4)
znvis/cameras/keyframe_camera.py (1)
get_view_matrix(239-255)znvis/cameras/particle_following_camera.py (1)
get_view_matrix(88-119)znvis/cameras/static_camera.py (1)
get_view_matrix(73-87)znvis/cameras/trajectory_camera.py (1)
get_view_matrix(49-52)
CI/unit_tests/camera/test_static_camera.py (2)
znvis/cameras/static_camera.py (2)
StaticCamera(29-87)get_view_matrix(73-87)znvis/cameras/base_camera.py (7)
get_view_matrix_from_particle_positions(64-89)look_at(152-188)get_view_matrix_from_box_size(92-113)get_minimal_view_matrix(116-149)get_view_matrix(190-204)set_view_matrix(206-219)get_center_eye_up_from_view_matrix(222-256)
🪛 Ruff (0.13.3)
znvis/cameras/static_camera.py
69-71: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Unused method argument: frame_index
(ARG002)
73-73: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
znvis/cameras/trajectory_camera.py
49-49: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
znvis/cameras/trajectories/zooming_trajectory.py
41-41: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
80-82: Avoid specifying long messages outside the exception class
(TRY003)
znvis/cameras/particle_following_camera.py
47-47: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
48-48: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
znvis/particle/particle.py
124-127: Avoid specifying long messages outside the exception class
(TRY003)
169-171: Avoid specifying long messages outside the exception class
(TRY003)
175-178: Avoid specifying long messages outside the exception class
(TRY003)
znvis/cameras/keyframe_camera.py
47-47: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
199-199: Loop control variable i not used within loop body
(B007)
239-239: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
znvis/visualizer/headless_visualizer.py
61-61: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
64-64: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
68-68: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
70-70: Do not perform function call Mitsuba in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
178-178: Unpacked variable layers is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
291-291: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
znvis/cameras/trajectories/base_trajectory.py
39-39: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
znvis/cameras/trajectories/circular_trajectory.py
95-95: Avoid specifying long messages outside the exception class
(TRY003)
97-97: Avoid specifying long messages outside the exception class
(TRY003)
142-142: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
znvis/cameras/base_camera.py
117-117: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
190-190: Unused method argument: frame_index
(ARG002)
190-190: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
CI/unit_tests/camera/test_static_camera.py
163-163: Unpacked variable center is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (3)
znvis/particle/particle.py (1)
44-55: API expansion to per-frame lists looks goodUnion[np.ndarray, List[np.ndarray]] for trajectory-like attributes is a sensible extension. No issues found.
Also applies to: 71-74
znvis/__init__.py (1)
41-41: Name consistency: Headless_Visualizer vs “Headless_Renderer” in PR descriptionDocs/examples mention Headless_Renderer; code exports Headless_Visualizer. Please align naming or update docs to avoid confusion.
znvis/visualizer/headless_visualizer.py (1)
177-179: Drop unused variable to satisfy lint (RUF059).[ suggest_recommended_refactor ]
Apply:- single_frame = cv2.imread(images[0]) - height, width, layers = single_frame.shape + single_frame = cv2.imread(images[0]) + height, width, _ = single_frame.shape
CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py
Outdated
Show resolved
Hide resolved
Adapted tests to new code.
Seems like with an open3d version newer than 0.17.0 several instances of the headless renderer and the visualizer can be run from the same script. Adapted the functionalities to that. Fixed several issues highlighted by the coderabbitai Reworked the interface of the keyframe camera to make usage more intuitive.
Also normalized director treatment when position is a list.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
znvis/visualizer/visualizer.py (1)
110-125: self.number_of_steps not set when number_of_steps is provided.If number_of_steps is not None, attribute remains unset, breaking later uses (e.g., keyframe_camera.number_of_frames assignment). Set it in the else path.
if number_of_steps is None: @@ else: self.number_of_steps = min(len_list) + else: + self.number_of_steps = int(number_of_steps)
♻️ Duplicate comments (2)
znvis/visualizer/headless_visualizer.py (1)
126-138: Thanks for addressing number_of_steps initialization.Branch now sets self.number_of_steps when a value is provided; resolves earlier runtime bug.
znvis/cameras/particle_following_camera.py (1)
103-116: Per-frame up normalization bug (uses whole array norm).up_norm is computed from self.camera_up_vector, not up_vec. This breaks per-frame handling and can skew normalization.
Apply:
- up_norm = np.linalg.norm(self.camera_up_vector) - if up_norm == 0.0: + up_norm = float(np.linalg.norm(up_vec)) + if up_norm == 0.0: raise ValueError( - f'"camera_up_vector" must be non-zero. ' - f"Error occured for frame index {frame_index}." + '"camera_up_vector" must be non-zero. ' + f"Error occurred for frame index {frame_index}." ) up = up_vec / up_norm
🧹 Nitpick comments (13)
znvis/particle/particle.py (2)
27-29: Use warnings.warn instead of print for runtime warnings.Prefer the warnings module for visibility and testability; keep stdout clean. Also import warnings.
Apply:
@@ -import numpy as np +import numpy as np +import warnings @@ - print( - "-------\nWARNING: The provided director array is empty." - "Setting to None.\n-------", - ) + warnings.warn( + "The provided director array is empty. Setting to None.", + RuntimeWarning, + ) @@ - print( - "-------\nWARNING: The provided position array has an ", - "unexpected shape. Using the first entry as the static array." - "\n-------", - ) + warnings.warn( + "The provided position array has an unexpected shape. " + "Using the first entry as the static array.", + RuntimeWarning, + ) @@ - print( - "-------\nWARNING: The provided director array has an " - "unexpected shape. Using the first entry as the static " - "array.\n-------", - ) + warnings.warn( + "The provided director array has an unexpected shape. " + "Using the first entry as the static array.", + RuntimeWarning, + )Based on static analysis hints
Also applies to: 131-133, 143-147, 156-159
200-204: Avoid shadowing built-in name “dir”.Rename local variable for clarity and to avoid masking built-in dir().
Apply:
- for particle_index in range(n_particles): - pos = frame_pos[particle_index] - dir = frame_dir[particle_index] if frame_dir is not None else None - mesh = self._create_mesh(pos, dir, frame_index, particle_index) + for particle_index in range(n_particles): + pos = frame_pos[particle_index] + director_vec = ( + frame_dir[particle_index] if frame_dir is not None else None + ) + mesh = self._create_mesh(pos, director_vec, frame_index, particle_index)CI/unit_tests/particle/test_particle.py (1)
118-119: Prefer assertEqual for shapes (clearer failure output).Use unittest’s assert for tuple shape checks.
Apply:
- np.testing.assert_array_equal(self.particle.position[0].shape, (2, 3)) + self.assertEqual(self.particle.position[0].shape, (2, 3))znvis/visualizer/headless_visualizer.py (1)
277-299: Type hints: avoid implicit Optional in method signature.Annotate Optional for parameters defaulting to None (RUF013).
Apply:
- def _update_particles(self, visualizer=None, step: int = None): + def _update_particles(self, visualizer: typing.Any | None = None, step: int | None = None):Based on static analysis hints
CI/unit_tests/camera/test_particle_following_camera.py (1)
17-18: Fix docstrings to reference ParticleFollowingCamera.Update misleading camera names in docstrings for clarity.
Apply:
- Prepare an instance of the InterpolationCamera class for testing + Prepare an instance of the ParticleFollowingCamera class for testing @@ - Test the initialization of the KeyframeCamera class. + Test the initialization of the ParticleFollowingCamera class.Also applies to: 29-30
CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py (1)
17-18: Fix docstrings to reference BaseTrajectory.Align docstrings with the class under test.
Apply:
- Prepare an instance of the InterpolationCamera class for testing + Prepare an instance of the BaseTrajectory class for testing @@ - Test the initialization of the KeyframeCamera class. + Test the initialization of the BaseTrajectory class.Also applies to: 23-24
CI/unit_tests/camera/test_static_camera.py (1)
148-172: Prefix unused variable with underscore.The test unpacks
centerbut only validateseyeandup. Since the docstring notes that center extraction is approximate and not the focus of this test, consider prefixing the variable with an underscore to indicate it's intentionally unused.Apply this diff:
- center, eye, up = self.look_at_camera.get_center_eye_up_from_view_matrix( + _center, eye, up = self.look_at_camera.get_center_eye_up_from_view_matrix( view_matrix )znvis/cameras/particle_following_camera.py (1)
88-101: Doc/signature mismatch for frame_index.Doc says “optional, default None” but signature requires int. Align either doc or code. Suggest making it optional with default 0 for convenience.
- def get_view_matrix(self, frame_index: int): + def get_view_matrix(self, frame_index: int = 0): @@ - frame_index : int, optional - The index of the frame for which to get the view matrix, by default None. + frame_index : int, optional + The index of the frame for which to get the view matrix. Default = 0.znvis/visualizer/visualizer.py (2)
806-811: Silence unused parameter warning._open3d passes an arg to actions; accept it but ignore explicitly.
- def _output_current_counter(self, visualizer=None): + def _output_current_counter(self, _=None): """ Output the current counter value. """ print(self.counter)
72-76: Avoid callable default for renderer.Instantiating Mitsuba() in the signature creates a shared mutable default. Prefer None and init inside.
- renderer: Mitsuba = Mitsuba(), + renderer: Mitsuba = None, @@ - self.renderer = renderer + self.renderer = renderer or Mitsuba()znvis/cameras/base_camera.py (1)
190-205: Unused parameter frame_index.Base method ignores frame_index; consider prefixing with underscore or documenting intentionally unused to silence linters.
- def get_view_matrix(self, frame_index: int) -> np.ndarray: + def get_view_matrix(self, frame_index: int) -> np.ndarray: # noqa: ARG002znvis/cameras/keyframe_camera.py (2)
236-239: Remove stray print; enforce minimal validation.Printing length on load is noisy in libraries. Keep the length check only.
- self.interpolated_view_matrices = np.load(path, allow_pickle=True) - print(len(self.interpolated_view_matrices)) + self.interpolated_view_matrices = np.load(path, allow_pickle=True)
262-263: zip without strict parameter (B905).Be explicit to catch length mismatches in future refactors.
- frame_indexes, view_matrices = zip(*view_matrices_dictionary.items()) + frame_indexes, view_matrices = zip(*view_matrices_dictionary.items(), strict=False)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py(1 hunks)CI/unit_tests/camera/camera_trajectories/test_circular_trajectory.py(1 hunks)CI/unit_tests/camera/camera_trajectories/test_zooming_trajectory.py(1 hunks)CI/unit_tests/camera/test_base_trajectory_camera.py(1 hunks)CI/unit_tests/camera/test_keyframe_camera.py(1 hunks)CI/unit_tests/camera/test_particle_following_camera.py(1 hunks)CI/unit_tests/camera/test_static_camera.py(1 hunks)CI/unit_tests/particle/test_particle.py(2 hunks)znvis/__init__.py(3 hunks)znvis/cameras/__init__.py(1 hunks)znvis/cameras/base_camera.py(1 hunks)znvis/cameras/keyframe_camera.py(1 hunks)znvis/cameras/particle_following_camera.py(1 hunks)znvis/cameras/static_camera.py(1 hunks)znvis/cameras/trajectories/base_trajectory.py(1 hunks)znvis/cameras/trajectories/circular_trajectory.py(1 hunks)znvis/cameras/trajectories/zooming_trajectory.py(1 hunks)znvis/cameras/trajectory_camera.py(1 hunks)znvis/particle/particle.py(3 hunks)znvis/visualizer/headless_visualizer.py(1 hunks)znvis/visualizer/visualizer.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- CI/unit_tests/camera/test_keyframe_camera.py
- CI/unit_tests/camera/camera_trajectories/test_zooming_trajectory.py
- CI/unit_tests/camera/test_base_trajectory_camera.py
- CI/unit_tests/camera/camera_trajectories/test_circular_trajectory.py
🧰 Additional context used
🧬 Code graph analysis (17)
znvis/cameras/__init__.py (5)
znvis/cameras/base_camera.py (1)
BaseCamera(27-278)znvis/cameras/keyframe_camera.py (1)
KeyframeCamera(32-381)znvis/cameras/particle_following_camera.py (1)
ParticleFollowingCamera(29-143)znvis/cameras/static_camera.py (1)
StaticCamera(29-87)znvis/cameras/trajectory_camera.py (1)
TrajectoryCamera(30-51)
znvis/cameras/trajectories/base_trajectory.py (3)
znvis/cameras/trajectories/circular_trajectory.py (1)
get_center_eye_up(147-191)znvis/cameras/trajectories/zooming_trajectory.py (1)
get_center_eye_up(94-117)znvis/cameras/base_camera.py (1)
get_center_eye_up_from_view_matrix(222-256)
znvis/cameras/trajectory_camera.py (2)
znvis/cameras/base_camera.py (1)
look_at(152-188)znvis/cameras/trajectories/base_trajectory.py (2)
BaseTrajectory(27-77)get_center_eye_up(39-43)
znvis/cameras/base_camera.py (2)
znvis/cameras/trajectory_camera.py (1)
get_view_matrix(48-51)znvis/cameras/particle_following_camera.py (1)
get_view_matrix(88-143)
CI/unit_tests/camera/test_particle_following_camera.py (1)
znvis/cameras/particle_following_camera.py (2)
ParticleFollowingCamera(29-143)get_view_matrix(88-143)
znvis/__init__.py (1)
znvis/visualizer/headless_visualizer.py (1)
Headless_Visualizer(45-310)
znvis/visualizer/headless_visualizer.py (4)
znvis/rendering/mitsuba.py (2)
Mitsuba(60-187)render_mesh_objects(113-187)znvis/particle/particle.py (1)
Particle(34-213)znvis/particle/vector_field.py (1)
VectorField(34-142)znvis/cameras/base_camera.py (3)
BaseCamera(27-278)verify_camera_setup_for_rendering(258-278)get_view_matrix(190-204)
znvis/cameras/particle_following_camera.py (2)
znvis/cameras/base_camera.py (2)
get_view_matrix(190-204)look_at(152-188)znvis/cameras/trajectory_camera.py (1)
get_view_matrix(48-51)
znvis/cameras/static_camera.py (1)
znvis/cameras/base_camera.py (3)
BaseCamera(27-278)look_at(152-188)get_view_matrix(190-204)
CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py (1)
znvis/cameras/trajectories/base_trajectory.py (3)
BaseTrajectory(27-77)get_center_eye_up(39-43)get_center_eye_up_from_view_matrix(46-77)
znvis/visualizer/visualizer.py (2)
znvis/cameras/keyframe_camera.py (6)
KeyframeCamera(32-381)add_view_matrix(104-116)get_view_matrix(319-335)remove_view_matrix(127-146)interpolate_and_export_view_matrices(179-212)reset_view_matrix_progress(118-125)znvis/rendering/mitsuba.py (1)
render_mesh_objects(113-187)
znvis/particle/particle.py (1)
znvis/particle/vector_field.py (1)
_create_mesh(68-96)
znvis/cameras/keyframe_camera.py (4)
znvis/cameras/base_camera.py (3)
BaseCamera(27-278)get_view_matrix(190-204)verify_camera_setup_for_rendering(258-278)znvis/cameras/trajectory_camera.py (1)
get_view_matrix(48-51)znvis/cameras/particle_following_camera.py (1)
get_view_matrix(88-143)znvis/cameras/static_camera.py (1)
get_view_matrix(73-87)
CI/unit_tests/particle/test_particle.py (3)
znvis/material/material.py (1)
Material(31-61)znvis/particle/particle.py (2)
Particle(34-213)construct_mesh_list(108-213)znvis/mesh/sphere.py (1)
Sphere(32-51)
CI/unit_tests/camera/test_static_camera.py (2)
znvis/cameras/static_camera.py (2)
StaticCamera(29-87)get_view_matrix(73-87)znvis/cameras/base_camera.py (7)
get_view_matrix_from_particle_positions(64-89)look_at(152-188)get_view_matrix_from_box_size(92-113)get_minimal_view_matrix(116-149)get_view_matrix(190-204)set_view_matrix(206-219)get_center_eye_up_from_view_matrix(222-256)
znvis/cameras/trajectories/circular_trajectory.py (2)
znvis/cameras/trajectories/base_trajectory.py (2)
BaseTrajectory(27-77)get_center_eye_up(39-43)znvis/cameras/trajectories/zooming_trajectory.py (1)
get_center_eye_up(94-117)
znvis/cameras/trajectories/zooming_trajectory.py (2)
znvis/cameras/trajectories/base_trajectory.py (2)
BaseTrajectory(27-77)get_center_eye_up(39-43)znvis/cameras/trajectories/circular_trajectory.py (1)
get_center_eye_up(147-191)
🪛 Ruff (0.14.0)
znvis/cameras/base_camera.py
117-117: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
190-190: Unused method argument: frame_index
(ARG002)
znvis/visualizer/headless_visualizer.py
61-61: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
64-64: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
68-68: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
70-70: Do not perform function call Mitsuba in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
164-164: Unpacked variable layers is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
277-277: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
znvis/cameras/particle_following_camera.py
47-47: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
48-48: Do not perform function call np.array in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
111-114: Avoid specifying long messages outside the exception class
(TRY003)
125-127: Avoid specifying long messages outside the exception class
(TRY003)
znvis/cameras/static_camera.py
69-71: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Unused method argument: frame_index
(ARG002)
znvis/visualizer/visualizer.py
138-138: Avoid specifying long messages outside the exception class
(TRY003)
806-806: Unused method argument: visualizer
(ARG002)
znvis/particle/particle.py
124-127: Avoid specifying long messages outside the exception class
(TRY003)
172-174: Avoid specifying long messages outside the exception class
(TRY003)
179-182: Avoid specifying long messages outside the exception class
(TRY003)
186-189: Avoid specifying long messages outside the exception class
(TRY003)
208-208: Avoid specifying long messages outside the exception class
(TRY003)
znvis/cameras/keyframe_camera.py
50-50: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
84-88: Avoid specifying long messages outside the exception class
(TRY003)
94-96: Avoid specifying long messages outside the exception class
(TRY003)
198-201: Avoid specifying long messages outside the exception class
(TRY003)
232-235: Avoid specifying long messages outside the exception class
(TRY003)
239-242: Avoid specifying long messages outside the exception class
(TRY003)
262-262: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
269-269: Loop control variable i not used within loop body
(B007)
372-377: Avoid specifying long messages outside the exception class
(TRY003)
CI/unit_tests/camera/test_static_camera.py
165-165: Unpacked variable center is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
znvis/cameras/trajectories/circular_trajectory.py
100-100: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Avoid specifying long messages outside the exception class
(TRY003)
znvis/cameras/trajectories/zooming_trajectory.py
73-73: Avoid specifying long messages outside the exception class
(TRY003)
82-84: Avoid specifying long messages outside the exception class
(TRY003)
92-92: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (28)
znvis/visualizer/headless_visualizer.py (1)
45-45: Public API name mismatch with PR description (Headless_Renderer vs Headless_Visualizer).Docs and examples mention Headless_Renderer; class here is Headless_Visualizer. Align naming or provide an alias/export.
Optionally add:
+ # Backwards-compat/export alias + Headless_Renderer = Headless_VisualizerCI/unit_tests/camera/test_particle_following_camera.py (1)
31-50: LGTM on assertions.Using unittest/NumPy assertions ensures failures are caught.
CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py (1)
33-45: LGTM on BaseTrajectory assertions.Tests correctly assert eye/up/center extraction and NotImplemented.
znvis/cameras/trajectory_camera.py (1)
48-51: LGTM!The
get_view_matriximplementation correctly delegates to the trajectory'sget_center_eye_upmethod and computes the view matrix using the inheritedlook_atutility.znvis/cameras/static_camera.py (2)
62-71: LGTM!The constructor properly validates that either a
view_matrixor all three vectors (center, eye, up) are provided, with clear error messaging when neither is supplied.
73-87: LGTM!For a static camera, the
frame_indexparameter is unused by design since the view matrix remains constant. This conforms to theBaseCamerainterface while correctly implementing static behavior.znvis/__init__.py (3)
25-25: LGTM!The module imports for
camerasandrenderingare correctly structured.
41-41: LGTM!The import of
Headless_Visualizerproperly exposes the new headless rendering capability at the package level.
57-57: LGTM!The
__all__list correctly includesHeadless_Visualizerand uses string literals for the module names "rendering" and "cameras", which is the proper approach for exporting modules.Also applies to: 62-63
znvis/cameras/__init__.py (2)
24-29: LGTM!The imports correctly expose all camera classes and the trajectories sub-package for the cameras module.
31-38: LGTM!The
__all__list correctly exports camera classes using their__name__attributes and the trajectories module as a string literal, which properly exposes the public API.znvis/cameras/trajectories/base_trajectory.py (3)
28-37: LGTM!The constructor properly initializes the base class with the total number of frames.
39-43: LGTM!The abstract
get_center_eye_upmethod correctly requires subclasses to implement the trajectory-specific logic.
45-77: LGTM!The static method correctly extracts center, eye, and up vectors from a view matrix using rotation-translation inversion. The documentation appropriately notes that the center is an approximation due to the normalized forward vector.
znvis/cameras/trajectories/zooming_trajectory.py (2)
35-93: LGTM!The constructor properly handles all edge cases identified in previous reviews:
- Defaults
upvector to[0, 1, 0]whenNone- Validates that
centerandinitial_eyeare not identical (preventing zero-norm direction)- Uses
max(1, number_of_zoom_frames - 1)denominator to avoid off-by-one errors- Validates
step_sizeis non-zero when multiple frames are used
94-117: LGTM!The
get_center_eye_upmethod correctly clamps the frame index and computes the camera position along the zoom trajectory.znvis/cameras/trajectories/circular_trajectory.py (3)
36-103: LGTM!The constructor properly initializes all trajectory parameters and includes appropriate validation for
rotation_axis(must be 'x', 'y', or 'z') andframes_per_rotation(must be at least 1).
104-145: LGTM!The
get_loop_and_ping_pong_frame_indexmethod correctly handles all four combinations of loop and ping_pong settings, with appropriate frame index wrapping and clamping behavior.
147-191: LGTM!The
get_center_eye_upmethod correctly:
- Adjusts frame index using loop/ping-pong logic
- Applies optional smoothing to progress via cosine interpolation
- Computes theta based on progress (smoothed or linear)
- Correctly includes radius in all three axis calculations (x, y, z)
- Returns appropriate center, eye, and up vectors
CI/unit_tests/camera/test_static_camera.py (9)
14-34: LGTM!The test setup properly creates fixtures for both initialization methods (center/eye/up and direct view_matrix), enabling comprehensive testing of the
StaticCameraclass.
36-50: LGTM!The test correctly validates that the
look_atmethod produces the expected view matrix for the given center, eye, and up vectors.
52-58: LGTM!The test correctly validates that a camera initialized with a view_matrix stores and returns that matrix.
60-67: LGTM!The test correctly validates that
StaticCameraraisesValueErrorwhen initialized with allNoneparameters.
69-83: LGTM!The test correctly validates the
get_view_matrix_from_particle_positionsstatic method by comparing against an expected matrix computed from the particle positions.
85-94: LGTM!The test correctly validates the
get_view_matrix_from_box_sizestatic method against expected translation values.
96-119: LGTM!The test correctly validates the
get_minimal_view_matrixstatic method, which computes a camera position that minimizes dead zones based on FOV and aspect ratio.
121-136: LGTM!The test correctly validates that
StaticCamera.get_view_matrixreturns the same view matrix regardless of frame index, which is the expected behavior for a static camera.
138-146: LGTM!The test correctly validates that the
set_view_matrixmethod updates the camera's view matrix.
This lets video creation be handled similar for the headless_renderer and the visualizer.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
znvis/visualizer/visualizer.py (1)
760-764: Bug: using identity comparison instead of assignment.self.do_rewind is False does nothing; should assign False.
- if self.do_rewind is True: - self.do_rewind is False + if self.do_rewind is True: + self.do_rewind = False self.play_speed = 1
♻️ Duplicate comments (5)
znvis/particle/particle.py (1)
176-191: Add explicit length check for position/director consistency.While the NaN validation is thorough, there's no explicit check that
len(self.director) == len(self.position)before iterating. If they mismatch, the code will fail during frame access at lines 197-198 with an IndexError rather than a clear error message.Add an explicit length check after line 178:
# Check data for consistency if self.position is None: raise ValueError("Position data must be not None.") +if self.director is not None and len(self.director) != len(self.position): + raise ValueError( + f"Director and position must have same number of frames: " + f"{len(self.director)} != {len(self.position)}" + ) for i, position in enumerate(self.position):CI/unit_tests/camera/test_particle_following_camera.py (1)
27-41: LGTM: assertions cover vectors, shapes, and view matrixThe tests now assert vector initializations and the computed view matrix; this addresses prior non-asserting np.allclose calls.
Also applies to: 42-50
CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py (1)
29-45: LGTM: proper assertions for center/eye/up extractionAssertions ensure failures are caught; test intent is clear.
znvis/visualizer/headless_visualizer.py (1)
62-62: Avoid callable defaults and implicit Optional; make types explicit.Use T | None and instantiate in-body to satisfy Ruff (RUF013, B008) and avoid side effects. Also make step Optional. As per static analysis hints
- number_of_steps: int = None, + number_of_steps: int | None = None, @@ - renderer: Mitsuba | None = Mitsuba(), + renderer: Mitsuba | None = None, @@ - camera: znvis.cameras.BaseCamera = None, + camera: znvis.cameras.BaseCamera | None = None, @@ - def _update_particles(self, visualizer=None, step: int = None): + def _update_particles(self, visualizer=None, step: int | None = None):Also applies to: 69-71, 277-277
znvis/cameras/keyframe_camera.py (1)
203-213: Fix UnboundLocalError when no keyframes; return consistently.Currently returns interpolated_view_matrices even if never defined. Return early (or raise) when empty, and return the stored attribute otherwise.
- if len(self.view_matrices_dictionary) == 0: - print("No view matrices found. Please add view matrices first.") - else: + if len(self.view_matrices_dictionary) == 0: + print("No view matrices found. Please add view matrices first.") + return None + else: sorted_view_matrices_dictionary = self._sort_dictionary() interpolated_view_matrices = self._interpolate_view_matrices( sorted_view_matrices_dictionary ) self.interpolated_view_matrices = interpolated_view_matrices self._export_interpolated_view_matrices() - return interpolated_view_matrices + return self.interpolated_view_matrices
🧹 Nitpick comments (21)
znvis/particle/particle.py (2)
121-161: Prefer the warnings module over print() for runtime warnings.The normalization logic correctly handles empty arrays and shape mismatches. However, the code uses
print()for warnings (lines 130-133, 143-147, 155-159), which makes it harder for users to control warning visibility or capture them programmatically.Consider using the
warningsmodule for better control:+import warnings + # Convert ndarrays into lists if isinstance(self.position, np.ndarray): if self.position.size == 0: raise ValueError( "The provided position array is empty." "Please provide a valid position array." ) if self.director is not None and self.director.size == 0: self.director = None - print( - "-------\nWARNING: The provided director array is empty." - "Setting to None.\n-------", - ) + warnings.warn( + "The provided director array is empty. Setting to None.", + UserWarning, + stacklevel=2, + )Apply similar changes to the other print() statements at lines 143-147 and 155-159.
193-215: Reconsider empty frame handling strategy.The code raises a ValueError when a frame has zero particles (line 210). While this prevents IndexError on
meshes[0], it might be overly strict. Some simulations could legitimately have frames where all particles have been removed or haven't been created yet.Consider whether empty frames should be:
- Rejected (current behavior) - appropriate if zero particles is always a data error
- Allowed - skip mesh construction or append a placeholder (None/empty mesh)
If empty frames are valid, apply:
# Combine all meshes into one if not meshes: - raise ValueError(f"No particles found at time step {frame_index}.") + # Allow empty frames - append None or continue + self.mesh_list.append(None) + continue combined_mesh = meshes[0] for m in meshes[1:]: combined_mesh += mPlease clarify the expected behavior for frames with zero particles. If they're always invalid, the current implementation is correct.
znvis/video/video_manager.py (3)
222-224: Narrow exception scope and chain the errorAvoid catching bare Exception; chain the original error for diagnostics.
Apply this diff:
- except Exception as e: - raise RuntimeError(f"Error while writing video frames: {e}") + except (cv2.error, OSError, ValueError) as e: + raise RuntimeError("Error while writing video frames") from e
24-31: Annotate CODEC_MAP as ClassVar and document codec availability caveatStatic analyzers flag mutable class attr; also note that availability depends on the OpenCV build (FFmpeg/encoders).
Apply this diff:
-from typing import List, Union +from typing import ClassVar, Dict, List, Union @@ - CODEC_MAP = { + CODEC_MAP: ClassVar[Dict[str, str]] = { "mp4": "mp4v", # MPEG-4 Part 2 (widely compatible) "avi": "XVID", # Xvid codec for AVI "mov": "mp4v", # QuickTime format with MPEG-4 "mkv": "XVID", # Matroska with Xvid "wmv": "WMV2", # Windows Media Video "webm": "VP80", # WebM with VP8 "flv": "FLV1", # Flash Video "mpg": "MPG1", # MPEG-1 "mpeg": "MPG1", # MPEG-1 "m4v": "mp4v", # MPEG-4 Video "ogv": "THEO", # Ogg Video (Theora) "3gp": "H263", # 3GPP format "h264": "H264", # H.264 codec "hevc": "HEVC", # H.265/HEVC codec "apng": "apng", # Animated PNG (not widely supported) }Consider restricting public “supported” list to formats verified in CI on your target platforms, and rely on the
isOpened()check (above) to fail fast when an encoder isn’t available. [Based on learnings]Also applies to: 41-58
115-137: Minor: prefer try/else for validate_video_formatStyle/readability tweak; functional behavior unchanged.
Apply this diff:
- try: - self.get_video_codec(video_format) - return video_format - except ValueError: + try: + self.get_video_codec(video_format) + except ValueError: print( f"Warning: Invalid video format '{video_format}'. Defaulting to 'mp4'." ) return "mp4" + else: + return video_formatznvis/video/__init__.py (1)
23-27: Remove duplicate all declarationDefine all once to avoid confusion.
Apply this diff:
-__all__ = ["VideoManager"] - -from .video_manager import VideoManager - -__all__ = ["VideoManager"] +from .video_manager import VideoManager +__all__ = ["VideoManager"]znvis/cameras/trajectory_camera.py (1)
35-42: Fix constructor docstring typeParameter type should be BaseTrajectory, not BaseCameraTrajectory.
Apply this diff:
- trajectory : BaseCameraTrajectory + trajectory : BaseTrajectory The camera trajectory object.znvis/cameras/static_camera.py (1)
73-87: Silence unused parameter (ARG002) to satisfy linters.The frame_index is unused by design for a static camera. Rename to underscore to make intent clear.
- def get_view_matrix(self, frame_index: int) -> np.ndarray: + def get_view_matrix(self, _frame_index: int) -> np.ndarray: """ Get the current view matrix of the camera. @@ - return self.view_matrix + return self.view_matrixAs per static analysis hints.
CI/unit_tests/video/test_video_manager.py (2)
81-82: Relax FPS equality; OpenCV often reports non-integer fps.Use a tolerance to avoid platform/codec-dependent flakiness.
- assert info["fps"] == self.frame_rate + assert abs(info["fps"] - self.frame_rate) < 0.5OpenCV wheels can yield slight fps drift across platforms. Based on learnings.
59-116: Prefer unittest assertions for clearer failure messages.Replace bare asserts with unittest asserts (e.g., assertEqual, assertTrue) for consistency and richer diffs.
CI/unit_tests/camera/test_static_camera.py (1)
16-18: Docstring accuracy nit.“This Camera class” → “StaticCamera class” for clarity.
- Prepare an instance of the Camera class for testing. + Prepare an instance of the StaticCamera class for testing.znvis/visualizer/visualizer.py (4)
71-75: Avoid mutable default and default-callable in signature (B006, B008).
- renderer_resolution should default to None.
- renderer should not be instantiated in the signature.
- renderer_resolution: list = [4096, 2160], + renderer_resolution: list | None = None, @@ - renderer: Mitsuba = Mitsuba(), + renderer: Mitsuba | None = None, @@ - self.renderer_resolution = renderer_resolution + self.renderer_resolution = renderer_resolution or [4096, 2160] @@ - self.renderer = renderer + self.renderer = renderer or Mitsuba()As per static analysis hints.
805-810: Silence unused callback arg (ARG002).Rename parameter to underscore; preserves callback signature.
- def _output_current_counter(self, visualizer=None): + def _output_current_counter(self, _visualizer=None):As per static analysis hints.
231-239: Docstring mismatch.Docstring says “Set self.interrupt = 1” but code sets it to 0. Update docstring.
- Set self.interrupt = 1 + Set self.interrupt = 0
306-318: Docstring typo/noise.“Parametersor texture in ...” looks like leftover text. Clean to a standard Parameters section.
- Parametersor texture in ("albedo", "normal", "ao", "metallic", "roughness"): - ---------- + Parameters + ----------znvis/cameras/particle_following_camera.py (3)
78-90: Validate input shapes and normalize particle_directions upfront.Add basic checks and normalization to avoid index/scale errors when arrays are (n_frames, 3) and ensure directions are unit-length with zero-norm guards.
self.particle_positions = particle_positions + # Basic shape checks + if self.particle_positions.ndim != 2 or self.particle_positions.shape[1] != 3: + raise ValueError("particle_positions must have shape (n_frames, 3)") if camera_particle_vector is None: camera_particle_vector = np.array([0, 0, 20]) if camera_up_vector is None: camera_up_vector = np.array([0, 1, 0]) if particle_directions is not None: - self.particle_directions = particle_directions + particle_directions = np.asarray(particle_directions) + if ( + particle_directions.ndim != 2 + or particle_directions.shape != self.particle_positions.shape + ): + raise ValueError( + "particle_directions must match particle_positions shape (n_frames, 3)" + ) + # Normalize per-frame with zero-norm guard + norms = np.linalg.norm(particle_directions, axis=1, keepdims=True) + norms[norms == 0.0] = 1.0 + self.particle_directions = particle_directions / norms else: self.particle_directions = None
92-106: Add return type hint.Annotate get_view_matrix return type for consistency.
- def get_view_matrix(self, frame_index: int): + def get_view_matrix(self, frame_index: int) -> np.ndarray:
115-131: Shorten exception messages (TRY003).Consider briefer messages to satisfy linters; context already clear from the raised site.
As per static analysis hints.
znvis/cameras/keyframe_camera.py (3)
48-53: Make view_matrices_path explicitly Optional (RUF013).Align with typing in this module.
- view_matrices_path: pathlib.Path = None, + view_matrices_path: pathlib.Path | None = None,
214-227: Docstring/signature mismatch: load_view_matrices doesn’t return dict.Function sets self.interpolated_view_matrices but returns nothing. Adjust signature and docstring.
- def load_view_matrices(self) -> dict: + def load_view_matrices(self) -> None: @@ - Returns - ------- - view_matrices_dictionary : dict - A dictionary containing the view matrices of the manually - picked + Returns + ------- + None
270-271: Nit: avoid unused loop variable (B007).Use list extend to prefill start frames.
- for i in range(start_frame): - interpolated_view_matrices.append(start_matrix) + interpolated_view_matrices.extend([start_matrix] * start_frame)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py(1 hunks)CI/unit_tests/camera/test_particle_following_camera.py(1 hunks)CI/unit_tests/camera/test_static_camera.py(1 hunks)CI/unit_tests/particle/test_particle.py(2 hunks)CI/unit_tests/video/test_video_manager.py(1 hunks)znvis/cameras/base_camera.py(1 hunks)znvis/cameras/keyframe_camera.py(1 hunks)znvis/cameras/particle_following_camera.py(1 hunks)znvis/cameras/static_camera.py(1 hunks)znvis/cameras/trajectory_camera.py(1 hunks)znvis/particle/particle.py(3 hunks)znvis/video/__init__.py(1 hunks)znvis/video/video_manager.py(1 hunks)znvis/visualizer/headless_visualizer.py(1 hunks)znvis/visualizer/visualizer.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
CI/unit_tests/camera/test_particle_following_camera.py (2)
znvis/cameras/particle_following_camera.py (2)
ParticleFollowingCamera(29-147)get_view_matrix(92-147)znvis/cameras/base_camera.py (1)
get_view_matrix(194-208)
CI/unit_tests/particle/test_particle.py (3)
znvis/material/material.py (1)
Material(31-61)znvis/particle/particle.py (2)
Particle(34-215)construct_mesh_list(108-215)znvis/mesh/sphere.py (1)
Sphere(32-51)
znvis/cameras/base_camera.py (5)
znvis/cameras/keyframe_camera.py (2)
get_view_matrix(320-336)verify_camera_setup_for_rendering(364-382)znvis/cameras/particle_following_camera.py (1)
get_view_matrix(92-147)znvis/cameras/static_camera.py (1)
get_view_matrix(73-87)znvis/cameras/trajectory_camera.py (1)
get_view_matrix(46-49)znvis/cameras/trajectories/base_trajectory.py (1)
get_center_eye_up_from_view_matrix(46-77)
CI/unit_tests/video/test_video_manager.py (1)
znvis/video/video_manager.py (5)
get_video_codec(86-113)validate_video_format(115-136)create_video_from_frames(138-235)get_video_info(237-270)get_supported_formats(75-84)
znvis/cameras/static_camera.py (1)
znvis/cameras/base_camera.py (2)
look_at(156-192)get_view_matrix(194-208)
znvis/cameras/keyframe_camera.py (1)
znvis/cameras/base_camera.py (3)
BaseCamera(27-282)get_view_matrix(194-208)verify_camera_setup_for_rendering(262-282)
znvis/video/__init__.py (1)
znvis/video/video_manager.py (1)
VideoManager(33-270)
znvis/cameras/trajectory_camera.py (2)
znvis/cameras/base_camera.py (2)
get_view_matrix(194-208)look_at(156-192)znvis/cameras/trajectories/base_trajectory.py (2)
BaseTrajectory(27-77)get_center_eye_up(39-43)
znvis/cameras/particle_following_camera.py (1)
znvis/cameras/base_camera.py (2)
get_view_matrix(194-208)look_at(156-192)
znvis/visualizer/headless_visualizer.py (5)
znvis/rendering/mitsuba.py (2)
Mitsuba(60-187)render_mesh_objects(113-187)znvis/video/video_manager.py (2)
validate_video_format(115-136)create_video_from_frames(138-235)znvis/particle/particle.py (1)
construct_mesh_list(108-215)znvis/bounding_objects/bounding_box.py (1)
BoundingBox(31-67)znvis/cameras/base_camera.py (2)
verify_camera_setup_for_rendering(262-282)get_view_matrix(194-208)
CI/unit_tests/camera/test_static_camera.py (2)
znvis/cameras/static_camera.py (2)
StaticCamera(29-87)get_view_matrix(73-87)znvis/cameras/base_camera.py (7)
get_view_matrix_from_particle_positions(64-89)look_at(156-192)get_view_matrix_from_box_size(92-113)get_minimal_view_matrix(116-153)get_view_matrix(194-208)set_view_matrix(210-223)get_center_eye_up_from_view_matrix(226-260)
znvis/visualizer/visualizer.py (3)
znvis/cameras/keyframe_camera.py (6)
KeyframeCamera(32-382)add_view_matrix(104-116)get_view_matrix(320-336)remove_view_matrix(127-146)interpolate_and_export_view_matrices(179-212)reset_view_matrix_progress(118-125)znvis/rendering/mitsuba.py (2)
Mitsuba(60-187)render_mesh_objects(113-187)znvis/video/video_manager.py (3)
VideoManager(33-270)validate_video_format(115-136)create_video_from_frames(138-235)
znvis/particle/particle.py (1)
znvis/particle/vector_field.py (1)
_create_mesh(68-96)
CI/unit_tests/camera/camera_trajectories/test_base_trajectory.py (1)
znvis/cameras/trajectories/base_trajectory.py (3)
BaseTrajectory(27-77)get_center_eye_up(39-43)get_center_eye_up_from_view_matrix(46-77)
🪛 Ruff (0.14.0)
znvis/cameras/base_camera.py
194-194: Unused method argument: frame_index
(ARG002)
znvis/cameras/static_camera.py
69-71: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Unused method argument: frame_index
(ARG002)
znvis/cameras/keyframe_camera.py
50-50: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
84-88: Avoid specifying long messages outside the exception class
(TRY003)
94-96: Avoid specifying long messages outside the exception class
(TRY003)
198-201: Avoid specifying long messages outside the exception class
(TRY003)
232-235: Avoid specifying long messages outside the exception class
(TRY003)
238-241: Avoid specifying long messages outside the exception class
(TRY003)
270-270: Loop control variable i not used within loop body
(B007)
373-378: Avoid specifying long messages outside the exception class
(TRY003)
znvis/cameras/particle_following_camera.py
115-118: Avoid specifying long messages outside the exception class
(TRY003)
129-131: Avoid specifying long messages outside the exception class
(TRY003)
znvis/visualizer/headless_visualizer.py
62-62: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
69-69: Do not perform function call Mitsuba in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
277-277: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
znvis/visualizer/visualizer.py
71-71: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
73-73: Do not perform function call Mitsuba in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
805-805: Unused method argument: visualizer
(ARG002)
znvis/particle/particle.py
124-127: Avoid specifying long messages outside the exception class
(TRY003)
172-174: Avoid specifying long messages outside the exception class
(TRY003)
178-178: Avoid specifying long messages outside the exception class
(TRY003)
181-184: Avoid specifying long messages outside the exception class
(TRY003)
188-191: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Avoid specifying long messages outside the exception class
(TRY003)
znvis/video/video_manager.py
42-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
108-111: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Consider moving this statement to an else block
(TRY300)
177-179: Avoid specifying long messages outside the exception class
(TRY003)
222-222: Do not catch blind exception: Exception
(BLE001)
223-223: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
223-223: Avoid specifying long messages outside the exception class
(TRY003)
254-254: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
znvis/particle/particle.py (2)
44-74: LGTM! Type annotations correctly support flexible input formats.The Union type annotations for position, velocity, force, and director properly reflect the refactored logic that accepts both numpy arrays and lists of per-frame arrays, aligning with the documented behavior in the docstring.
163-174: Director normalization correctly addresses previous feedback.The code properly normalizes director to list form when position is a list, handling both 3D and 2D cases. The shape validation ensures consistency before mesh construction.
Based on past review comments.
CI/unit_tests/particle/test_particle.py (4)
46-93: LGTM! Comprehensive test setup covering all edge cases.The expanded setup properly initializes particles for testing static meshes, empty arrays, NaN values, and configuration warnings. The Material and Sphere initialization correctly reflects the updated API.
95-106: LGTM! Test correctly verifies shape normalization for static meshes.The test validates that when a static particle receives a 3D position array (unexpected shape), the normalization logic correctly extracts the first frame and produces a shape of (2, 3).
107-132: LGTM! Tests correctly updated for list-based position handling.The updated assertions properly reflect the refactored data structure where position is treated as a list of frames, using
len(self.particle.position)instead of shape checks andposition[0].shapefor per-frame dimensions.
134-215: LGTM! Comprehensive test coverage for edge cases and error handling.The new tests thoroughly validate:
- Static particle initialization and mesh construction with shape handling
- Empty array detection with clear error messages
- NaN detection in both position and director with specific error messages
- Recovery paths (e.g., setting valid position after empty array error)
The test assertions correctly verify error messages and final states, ensuring robust error handling.
znvis/visualizer/headless_visualizer.py (1)
127-139: Good: number_of_steps now always defined.Branch assigns self.number_of_steps for both None and provided cases. Fix addresses earlier runtime risk.
Also fixed several bugs and best-practice suggestions
Test was incompatible with _frame_index
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
znvis/video/video_manager.py (1)
182-182: Sorting assumes all filenames contain digits.The sort key
lambda s: int(re.search(r"\d+", s.name).group())will crash withAttributeErrorif any filename lacks digits (whenre.searchreturns None).Apply this diff to handle filenames without digits:
- images = sorted(images, key=lambda s: int(re.search(r"\d+", s.name).group())) + # Natural sort by first integer in name; fallback to filename + def _num_key(p): + m = re.search(r"\d+", p.name) + return (m is None, int(m.group()) if m else 0, p.name) + + images = sorted(images, key=_num_key)Based on past review comments.
🧹 Nitpick comments (9)
znvis/cameras/particle_following_camera.py (1)
92-147: Consider optional bounds and shape validation.While numpy will naturally raise errors for invalid shapes or out-of-bounds indices, adding explicit validation could provide clearer error messages:
- Validate
frame_indexis within[0, len(particle_positions))at the start ofget_view_matrix- Optionally validate shapes in
__init__(e.g.,particle_positions.shape[1] == 3)This is a nice-to-have for improved user experience, not a critical issue.
znvis/video/video_manager.py (2)
41-57: Consider annotating CODEC_MAP withtyping.ClassVar.CODEC_MAP is a class-level constant. Adding a
ClassVarannotation improves type safety and clarifies that this is not an instance attribute.Apply this diff:
+from typing import ClassVar, List, Union + class VideoManager: """ Professional video creation and management for ZnVis. Handles codec selection, frame validation, and video export with proper error handling and resource management. """ # Codec mapping for different formats - CODEC_MAP = { + CODEC_MAP: ClassVar[dict[str, str]] = { "mp4": "mp4v", # MPEG-4 Part 2 (widely compatible) "avi": "XVID", # Xvid codec for AVI
228-229: Preserve exception chain when re-raising.Use
raise ... from eto maintain the original exception context for debugging.Apply this diff:
except Exception as e: - raise RuntimeError(f"Error while writing video frames: {e}") + raise RuntimeError(f"Error while writing video frames: {e}") from eznvis/visualizer/__init__.py (1)
28-28: Consider sorting__all__alphabetically (optional).Static analysis suggests sorting
__all__for consistency. However, the current logical ordering (BaseVisualizer → Visualizer → Headless_Visualizer) is also reasonable.If you prefer alphabetical ordering:
-__all__ = ["BaseVisualizer", "Visualizer", "Headless_Visualizer"] +__all__ = ["BaseVisualizer", "Headless_Visualizer", "Visualizer"]znvis/visualizer/base_visualizer.py (1)
214-234: Unusedcounterparameter can be removed.The
counterparameter is declared but never used. The method always usesself.counter. Consider removing it or documenting why it's kept for future use.Apply this diff if not needed:
- def get_mesh_dict(self, counter=None): + def get_mesh_dict(self): """ Creates the mesh dict for a given scene. """znvis/cameras/static_camera.py (2)
69-71: Consider extracting error message to a constant.The static analysis tool flagged this as TRY003 - long error messages are conventionally defined as class or module constants for reusability and maintainability.
If desired, you could refactor to:
+_INVALID_INIT_MSG = "Either a view_matrix or center, eye and up must be provided." + class StaticCamera(BaseCamera): def __init__( ... ) -> None: ... else: - raise ValueError( - "Either a view_matrix or center, eye and up must be provided." - ) + raise ValueError(_INVALID_INIT_MSG)
62-71: Initialization logic is correct; optional shape checks and message refactor
BaseCameraalready enforces a(4, 4)shape forview_matrix.- You may add early asserts that
center,eye, andupare 1D arrays of length 3 to fail fast.- (Optional) shorten or extract the
ValueErrortext on lines 68–71 to satisfy the TRY003 lint hint.znvis/cameras/trajectory_camera.py (1)
46-49: Avoid mutatingself.view_matrixin get_view_matrix
Return the computed view matrix directly viaself.look_at(center, eye, up)and move any caching logic to a separate method.znvis/cameras/keyframe_camera.py (1)
148-178: Guard against None in_sort_dictionary.Line 167 accesses
self.number_of_frameswhich could beNoneif the method is called beforenumber_of_framesis set. While the current caller (interpolate_and_export_view_matrices) guards this, the private method should be defensive.Apply this diff to add an early check:
def _sort_dictionary(self) -> None: """ Create the view matrix dictionary. This function is called in the visualizer.py to create the view matrix dictionary. """ + if self.number_of_frames is None: + raise ValueError( + "number_of_frames must be set before sorting the dictionary." + ) min_key = min(self.view_matrices_dictionary.keys())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CI/unit_tests/camera/test_static_camera.py(1 hunks)CI/unit_tests/video/test_video_manager.py(1 hunks)znvis/cameras/keyframe_camera.py(1 hunks)znvis/cameras/particle_following_camera.py(1 hunks)znvis/cameras/static_camera.py(1 hunks)znvis/cameras/trajectory_camera.py(1 hunks)znvis/video/__init__.py(1 hunks)znvis/video/video_manager.py(1 hunks)znvis/visualizer/__init__.py(1 hunks)znvis/visualizer/base_visualizer.py(1 hunks)znvis/visualizer/headless_visualizer.py(1 hunks)znvis/visualizer/visualizer.py(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- CI/unit_tests/camera/test_static_camera.py
- znvis/video/init.py
- znvis/visualizer/headless_visualizer.py
🧰 Additional context used
🧬 Code graph analysis (8)
znvis/cameras/trajectory_camera.py (2)
znvis/cameras/base_camera.py (2)
get_view_matrix(194-208)look_at(156-192)znvis/cameras/trajectories/base_trajectory.py (2)
BaseTrajectory(27-77)get_center_eye_up(39-43)
znvis/visualizer/base_visualizer.py (6)
znvis/rendering/mitsuba.py (1)
Mitsuba(60-187)znvis/video/video_manager.py (3)
VideoManager(32-288)validate_video_format(114-135)create_video_from_frames(137-250)znvis/particle/particle.py (1)
Particle(34-215)znvis/particle/vector_field.py (1)
VectorField(34-142)znvis/bounding_objects/bounding_box.py (1)
BoundingBox(31-67)znvis/visualizer/visualizer.py (3)
_initialize_particles(369-380)_initialize_vector_field(382-385)_update_particles(601-634)
znvis/cameras/keyframe_camera.py (1)
znvis/cameras/base_camera.py (3)
BaseCamera(27-282)get_view_matrix(194-208)verify_camera_setup_for_rendering(262-282)
CI/unit_tests/video/test_video_manager.py (1)
znvis/video/video_manager.py (6)
VideoManager(32-288)get_video_codec(85-112)validate_video_format(114-135)create_video_from_frames(137-250)get_video_info(252-288)get_supported_formats(74-83)
znvis/visualizer/visualizer.py (3)
znvis/cameras/keyframe_camera.py (6)
KeyframeCamera(32-386)add_view_matrix(104-116)get_view_matrix(324-340)remove_view_matrix(127-146)interpolate_and_export_view_matrices(179-213)reset_view_matrix_progress(118-125)znvis/rendering/mitsuba.py (2)
Mitsuba(60-187)render_mesh_objects(113-187)znvis/visualizer/base_visualizer.py (2)
BaseVisualizer(32-235)get_mesh_dict(214-235)
znvis/cameras/static_camera.py (1)
znvis/cameras/base_camera.py (3)
BaseCamera(27-282)look_at(156-192)get_view_matrix(194-208)
znvis/visualizer/__init__.py (3)
znvis/visualizer/base_visualizer.py (1)
BaseVisualizer(32-235)znvis/visualizer/headless_visualizer.py (1)
Headless_Visualizer(43-206)znvis/visualizer/visualizer.py (1)
Visualizer(43-758)
znvis/cameras/particle_following_camera.py (1)
znvis/cameras/base_camera.py (3)
BaseCamera(27-282)get_view_matrix(194-208)look_at(156-192)
🪛 Ruff (0.14.0)
znvis/visualizer/base_visualizer.py
192-192: Unused method argument: visualizer
(ARG002)
192-192: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
214-214: Unused method argument: counter
(ARG002)
znvis/cameras/keyframe_camera.py
84-88: Avoid specifying long messages outside the exception class
(TRY003)
94-96: Avoid specifying long messages outside the exception class
(TRY003)
198-201: Avoid specifying long messages outside the exception class
(TRY003)
232-235: Avoid specifying long messages outside the exception class
(TRY003)
238-241: Avoid specifying long messages outside the exception class
(TRY003)
299-302: Avoid specifying long messages outside the exception class
(TRY003)
377-382: Avoid specifying long messages outside the exception class
(TRY003)
znvis/visualizer/visualizer.py
138-138: Avoid specifying long messages outside the exception class
(TRY003)
738-738: Unused method argument: visualizer
(ARG002)
znvis/cameras/static_camera.py
69-71: Avoid specifying long messages outside the exception class
(TRY003)
znvis/visualizer/__init__.py
28-28: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
znvis/video/video_manager.py
41-57: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
107-110: Avoid specifying long messages outside the exception class
(TRY003)
130-130: Consider moving this statement to an else block
(TRY300)
176-178: Avoid specifying long messages outside the exception class
(TRY003)
188-188: Avoid specifying long messages outside the exception class
(TRY003)
213-216: Avoid specifying long messages outside the exception class
(TRY003)
228-228: Do not catch blind exception: Exception
(BLE001)
229-229: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
229-229: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Do not catch blind exception: Exception
(BLE001)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
znvis/cameras/particle_following_camera.py
115-118: Avoid specifying long messages outside the exception class
(TRY003)
129-131: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (17)
znvis/cameras/particle_following_camera.py (2)
43-90: LGTM! Default argument initialization properly addressed.The
__init__method now correctly initializescamera_particle_vectorandcamera_up_vectorinside the function body (lines 79-82) rather than usingnp.arraycalls in the default arguments, eliminating the B008 violations flagged in previous reviews.
92-147: LGTM! Per-frame handling and zero-norm guards correctly implemented.The
get_view_matrixmethod now properly:
- Supports per-frame
camera_up_vector(lines 108-112) by checkingndimand indexing appropriately- Guards against zero-norm up vectors (lines 113-119) with a clear error message
- Handles per-frame
camera_particle_vectorwhen computing distance (lines 133-137)- Guards against zero distance (lines 139-140) using a sensible epsilon
All critical concerns from previous reviews have been addressed.
znvis/video/video_manager.py (1)
271-287: LGTM!FPS validation and resource cleanup are properly handled. The guards against zero/invalid FPS prevent division errors, and the
finallyblock ensures the VideoCapture is released.znvis/visualizer/base_visualizer.py (2)
66-150: LGTM!The base class initialization is comprehensive and well-structured. Path setup, video management, renderer configuration, and automatic
number_of_stepsdetection are all properly handled.
152-170: LGTM!Video creation delegates properly to
VideoManagerwith appropriate error handling. The try-except block catches and re-raisesRuntimeErrorwith proper context.znvis/cameras/static_camera.py (1)
73-87: LGTM! Appropriate use of underscore for unused parameter.The method correctly returns the fixed view matrix, and using
_frame_indexclearly signals that the parameter is unused for static cameras, which aligns with Python conventions for abstract method signatures.znvis/cameras/trajectory_camera.py (1)
35-44: super().init() not required BaseCamera.init is abstract (raises NotImplementedError); subclasses must implement their own initializer without invoking super.Likely an incorrect or invalid review comment.
znvis/cameras/keyframe_camera.py (5)
48-103: LGTM! Robust initialization with proper validation.The constructor correctly validates the
import_view_matricesrequirement and handles both interactive and import modes. The conditional logic ensures paths exist when importing and provides clear guidance for interactive use.
203-214: Clean handling of empty dictionary.The method now explicitly returns
Nonewhen the dictionary is empty, avoiding theUnboundLocalErrorflagged in previous reviews. The else block properly defines and returnsinterpolated_view_matrices.
272-284: Interpolation logic is correct.The loop properly generates intermediate frames between keyframes. By iterating
jfrom 1 tointerpolation_steps(inclusive), each segment produces the correct number of interpolated matrices, with the endpoint of each segment matching the start of the next. The assertion on line 288 validates the final count.
228-236: Improved path handling.The method now handles both directory and file paths correctly. If
view_matrices_pathis a directory, it appends the expected filename; if it's already a.npyfile, it uses it directly. This resolves the path inconsistency flagged in previous reviews.
298-302: Proper guard against None path.The method now validates that
view_matrices_pathis set before attempting export, raising a clearValueErrorif it'sNone. This addresses the guard concern from previous reviews.znvis/visualizer/visualizer.py (5)
111-124: Clean integration with BaseVisualizer.The constructor properly delegates to the parent class with all necessary parameters, maintaining a clear separation of concerns. The visualizer-specific attributes are then set independently.
136-151: KeyframeCamera integration is well-structured.The conditional setup validates the camera type, synchronizes paths and frame counts, and activates the UI interface only when a KeyframeCamera is provided. The default path assignment (line 146) is a helpful convenience.
183-204: UI actions properly wire KeyframeCamera methods.The lambda functions correctly capture
self.counterand pass it along with the current view matrix to the camera's methods. The actions are conditionally added based onactivate_view_matrix_interface, ensuring they're only available when appropriate.
130-131: New folder organization improves output structure.The addition of
scene_folderandscreenshot_folderprovides better organization for exported assets, separating scenes, screenshots, and video frames into distinct directories.
738-743: Unused parameter is fine for callback signature.The
visualizerparameter is required by the callback interface but not used in this method. The static analysis warning (ARG002) can be safely ignored—consistency with other action callbacks is more important than removing an unused parameter.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
znvis/cameras/static_camera.py (1)
73-87: Consider prefixing unused parameter with underscore.The
frame_indexparameter is intentionally unused for static cameras (since the view matrix doesn't change), but this maintains interface consistency with other camera implementations that do use it.To clarify the intent and silence the linter warning, consider prefixing the parameter with an underscore:
Apply this diff:
- def get_view_matrix(self, frame_index: int) -> np.ndarray: + def get_view_matrix(self, _frame_index: int) -> np.ndarray: """ Get the current view matrix of the camera. Parameters ---------- - frame_index : int + _frame_index : int The frame index of the view matrix that should be returned. + Note: This parameter is unused for static cameras but maintained + for interface consistency. Returns ------- view_matrix : np.ndarray shape=(4, 4) The view matrix of the camera. """ return self.view_matrix
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
znvis/cameras/static_camera.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
znvis/cameras/static_camera.py (1)
znvis/cameras/base_camera.py (2)
look_at(156-192)get_view_matrix(194-208)
🪛 Ruff (0.14.0)
znvis/cameras/static_camera.py
69-71: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Unused method argument: frame_index
(ARG002)
🔇 Additional comments (1)
znvis/cameras/static_camera.py (1)
1-28: LGTM!The file header and imports are clean and properly structured.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
znvis/cameras/static_camera.py (1)
62-80: Previous validation suggestions have been implemented.The input validation requested in the earlier review has been successfully added. The implementation correctly:
- Prevents conflicting parameter combinations (line 62-65)
- Validates view_matrix shape (line 67-68)
- Validates center/eye/up as numpy arrays with correct shapes (line 72-74)
- Ensures at least one initialization method is provided (line 77-80)
Optional: Consider adding isinstance check for view_matrix.
For consistency with the center/eye/up validation, consider adding an isinstance check before the shape validation:
if view_matrix is not None: + if not isinstance(view_matrix, np.ndarray): + raise ValueError("view_matrix must be a numpy array.") if view_matrix.shape != (4, 4): raise ValueError("view_matrix must have shape (4, 4).") self.view_matrix = view_matrixThis provides a clearer error message if a non-array is passed and aligns with the validation pattern used for center/eye/up vectors.
CI/unit_tests/video/test_video_manager.py (4)
59-63: PreferassertEqualfor clarity.The assertions work correctly but
assertEqualis more idiomatic for equality checks and provides better failure messages.Apply this diff:
- self.assertTrue(self.manager.get_video_codec("mp4") == "mp4v") - self.assertTrue(self.manager.get_video_codec("avi") == "XVID") - self.assertTrue(self.manager.get_video_codec("webm") == "VP80") + self.assertEqual(self.manager.get_video_codec("mp4"), "mp4v") + self.assertEqual(self.manager.get_video_codec("avi"), "XVID") + self.assertEqual(self.manager.get_video_codec("webm"), "VP80")
65-67: PreferassertEqualfor clarity.Consistent with the previous test, use
assertEqualfor more idiomatic assertions.Apply this diff:
- self.assertTrue(self.manager.validate_video_format("mp4") == "mp4") - self.assertTrue(self.manager.validate_video_format("xyz") == "mp4") # fallback + self.assertEqual(self.manager.validate_video_format("mp4"), "mp4") + self.assertEqual(self.manager.validate_video_format("xyz"), "mp4") # fallback
79-80: PreferassertEqualfor clarity.Use
assertEqualfor more idiomatic assertions with better failure messages.Apply this diff:
- self.assertTrue(info["width"] == 200) - self.assertTrue(info["height"] == 100) + self.assertEqual(info["width"], 200) + self.assertEqual(info["height"], 100) self.assertTrue(abs(info["fps"] - self.frame_rate) < 0.5) - self.assertTrue(info["frame_count"] == 10) + self.assertEqual(info["frame_count"], 10)Also applies to: 82-82
119-119: Remove unnecessary parentheses.Single exception types don't need tuple wrapping.
Apply this diff:
- with self.assertRaises((RuntimeError)) as context: + with self.assertRaises(RuntimeError) as context:znvis/video/video_manager.py (2)
40-57: Annotate class attribute withClassVar.Mutable class attributes should be annotated with
typing.ClassVarto clearly indicate they're shared across instances.Apply this diff:
+ from typing import ClassVar + # Codec mapping for different formats - CODEC_MAP = { + CODEC_MAP: ClassVar[dict[str, str]] = { "mp4": "mp4v", # MPEG-4 Part 2 (widely compatible)Note: Import
ClassVarat the top of the file fromtyping.
228-229: Preserve exception chain withraise ... from.Use
raise ... from eto maintain the original exception context for better debugging.Apply this diff:
except Exception as e: - raise RuntimeError(f"Error while writing video frames: {e}") + raise RuntimeError(f"Error while writing video frames: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CI/unit_tests/video/test_video_manager.py(1 hunks)znvis/cameras/static_camera.py(1 hunks)znvis/video/video_manager.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
CI/unit_tests/video/test_video_manager.py (1)
znvis/video/video_manager.py (6)
VideoManager(32-288)get_video_codec(85-112)validate_video_format(114-135)create_video_from_frames(137-250)get_video_info(252-288)get_supported_formats(74-83)
znvis/cameras/static_camera.py (1)
znvis/cameras/base_camera.py (2)
look_at(156-192)get_view_matrix(194-208)
🪛 Ruff (0.14.0)
znvis/video/video_manager.py
41-57: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
107-110: Avoid specifying long messages outside the exception class
(TRY003)
130-130: Consider moving this statement to an else block
(TRY300)
176-178: Avoid specifying long messages outside the exception class
(TRY003)
188-188: Avoid specifying long messages outside the exception class
(TRY003)
213-216: Avoid specifying long messages outside the exception class
(TRY003)
228-228: Do not catch blind exception: Exception
(BLE001)
229-229: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
229-229: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Do not catch blind exception: Exception
(BLE001)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
znvis/cameras/static_camera.py
63-65: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
78-80: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Unused method argument: frame_index
(ARG002)
🔇 Additional comments (2)
znvis/cameras/static_camera.py (1)
82-96: The unusedframe_indexparameter is intentional.The static analysis tool flags
frame_indexas unused, but this is a false positive. The parameter is required for interface compatibility withBaseCamera.get_view_matrix()and other camera implementations that do use frame indices (e.g., KeyframeCamera, ParticleFollowingCamera). For a static camera, the view matrix correctly remains constant across all frames.CI/unit_tests/video/test_video_manager.py (1)
92-93: PreferassertEqualfor clarity.Use
assertEqualfor more idiomatic assertions.Apply this diff:
- self.assertTrue(video_path.exists()) - self.assertTrue(str(video_path).endswith(".mp4")) + self.assertTrue(video_path.exists()) + self.assertTrue(str(video_path).endswith(".mp4"))Actually,
assertTruewithexists()andendswith()is appropriate for boolean checks. Only the assertEqual suggestion applies when checking exact values. Let me reconsider.Line 92:
self.assertTrue(video_path.exists())- appropriate for boolean
Line 93:self.assertTrue(str(video_path).endswith(".mp4"))- appropriate for boolean substring checkNo issue here.
-->
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
CI/unit_tests/visualizer/test_headless_renderer.py (5)
44-162: Reduce code duplication in test setup.The setup method recreates nearly identical particle and vector field objects three times (lines 52-101, 116-132, 148-149). This duplication makes the test harder to maintain and increases the risk of inconsistencies.
Consider extracting common test data creation into helper methods:
@classmethod def _create_test_particles(cls): """Create standard test particles.""" position = np.random.uniform(-5, 5, (10, 2, 3)) particle1 = Particle( name="my_particle", position=position.copy(), mesh=Sphere() ) static_pos = np.random.uniform(-5, 5, (2, 3)) particle2 = Particle( name="static_particle", position=static_pos.copy(), mesh=Sphere(), static=True, ) return particle1, particle2, position, static_pos @classmethod def _create_test_vector_fields(cls, pos, static_directors, dynamic_pos, dynamic_directors): """Create standard test vector fields.""" static_vector_field = VectorField( name="static_field", position=pos, mesh=Arrow(scale=10, material=cls.material), direction=static_directors, static=True, ) dynamic_vector_field = VectorField( name="dynamic_field", position=dynamic_pos, mesh=Arrow(scale=10, material=cls.material), direction=dynamic_directors, ) return static_vector_field, dynamic_vector_fieldThen call these helpers in
setUpClassto reduce duplication.
203-207: Use shutil.rmtree for more robust cleanup.The current cleanup approach uses glob patterns with manual file deletion, which is fragile and won't handle nested directories or non-empty directories properly.
Apply this diff:
+ import shutil + # Delete all content of the output folder - for file in self.visualizer.output_folder.glob("video_frames/*"): - file.unlink() - for file in self.visualizer.output_folder.glob("video_frames"): - file.rmdir() + video_frames_dir = self.visualizer.output_folder / "video_frames" + if video_frames_dir.exists(): + shutil.rmtree(video_frames_dir)
209-228: Add cleanup and verify video creation.This test lacks cleanup after execution and doesn't verify that the video file was actually created (it only checks that frames were deleted).
Consider adding cleanup and video file verification:
def test_keep_frames(self): """ Test the keep frames option. """ self.visualizer_delete_frames.render_visualization() self.assertFalse( ( self.visualizer_delete_frames.output_folder / "video_frames" / "frame_000000.png" ).exists() ) self.assertFalse( ( self.visualizer_delete_frames.output_folder / "video_frames" / "frame_000009.png" ).exists() ) # Verify video was created video_file = self.visualizer_delete_frames.output_folder / f"ZnVis-Video.{self.visualizer_delete_frames.video_format}" self.assertTrue(video_file.exists(), "Video file should be created") # Cleanup import shutil if self.visualizer_delete_frames.output_folder.exists(): shutil.rmtree(self.visualizer_delete_frames.output_folder)
251-254: Use shutil.rmtree for cleanup consistency.Similar to the earlier cleanup issue, use a more robust approach.
Apply this diff:
+ import shutil + - for file in self.visualizer_empty.output_folder.glob("video_frames/*"): - file.unlink() - for file in self.visualizer_empty.output_folder.glob("video_frames"): - file.rmdir() + video_frames_dir = self.visualizer_empty.output_folder / "video_frames" + if video_frames_dir.exists(): + shutil.rmtree(video_frames_dir)
186-208: Consider verifying video file creation.The test checks that frame files exist but doesn't verify that the video file itself was created successfully, which is a key output of the headless visualizer.
Add an assertion to verify video creation:
def test_headless_rendering(self): """ Test the headless rendering process. """ self.visualizer.render_visualization() self.assertTrue( ( self.visualizer.output_folder / "video_frames" / "frame_000000.png" ).exists() ) self.assertTrue( ( self.visualizer.output_folder / "video_frames" / "frame_000009.png" ).exists() ) # Verify video was created video_file = self.visualizer.output_folder / f"ZnVis-Video.{self.visualizer.video_format}" self.assertTrue(video_file.exists(), "Video file should be created") # Delete all content of the output folder import shutil video_frames_dir = self.visualizer.output_folder / "video_frames" if video_frames_dir.exists(): shutil.rmtree(video_frames_dir) if video_file.exists(): video_file.unlink()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CI/integration_tests/test_different_shapes.py(1 hunks)CI/integration_tests/test_simple_vector_fields.py(1 hunks)CI/unit_tests/visualizer/test_headless_renderer.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CI/integration_tests/test_different_shapes.py
🧰 Additional context used
🧬 Code graph analysis (1)
CI/unit_tests/visualizer/test_headless_renderer.py (6)
znvis/material/material.py (1)
Material(31-61)znvis/mesh/arrow.py (1)
Arrow(34-84)znvis/mesh/sphere.py (1)
Sphere(32-51)znvis/particle/particle.py (1)
Particle(34-215)znvis/particle/vector_field.py (1)
VectorField(34-150)znvis/visualizer/headless_visualizer.py (2)
Headless_Visualizer(43-206)render_visualization(194-206)
🔇 Additional comments (1)
CI/integration_tests/test_simple_vector_fields.py (1)
42-43: LGTM! Docstring now accurately reflects the test.The updated docstring correctly describes the test as working with vector fields rather than spheres, aligning with the test class name and the actual VectorField objects created in the test.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
CI/unit_tests/visualizer/test_headless_renderer.py (3)
114-130: Use .copy() when reusing position arrays to avoid shared references.When creating the second and third visualizer instances, the particle position arrays are reused without calling
.copy(). This creates shared references that could lead to unexpected behavior if any test modifies these arrays.Apply this diff to add
.copy()calls:-particle1 = Particle(name="my_particle", position=position, mesh=Sphere()) +particle1 = Particle(name="my_particle", position=position.copy(), mesh=Sphere()) particle2 = Particle( - name="static_particle", position=static_pos, mesh=Sphere(), static=True + name="static_particle", position=static_pos.copy(), mesh=Sphere(), static=True )
201-205: Consider using addCleanup for more robust test cleanup.The manual cleanup code in this test method (and similarly in
test_empty_visualizerat lines 249-252) will be skipped if any assertion fails, potentially leaving test artifacts on disk.Consider using
self.addCleanup()to ensure cleanup always runs:def test_headless_rendering(self): """ Test the headless rendering process. """ + def cleanup_frames(): + """Remove generated frames.""" + frames_dir = self.visualizer.output_folder / "video_frames" + if frames_dir.exists(): + for file in frames_dir.glob("*"): + file.unlink() + frames_dir.rmdir() + + self.addCleanup(cleanup_frames) + self.visualizer.render_visualization() self.assertTrue( ( self.visualizer.output_folder / "video_frames" / "frame_000000.png" ).exists() ) self.assertTrue( ( self.visualizer.output_folder / "video_frames" / "frame_000009.png" ).exists() ) - - # Delete all content of the output folder - for file in self.visualizer.output_folder.glob("video_frames/*"): - file.unlink() - for file in self.visualizer.output_folder.glob("video_frames"): - file.rmdir()Apply the same pattern to
test_empty_visualizer.
207-226: Verify and clean up video file when do_create_video is True.This test verifies that frames are deleted when
keep_frames=False, but doesn't check whether the video file itself was created or clean it up afterward. Since the visualizer hasdo_create_video=Trueby default, a video file may be left behind.Consider adding assertions and cleanup for the video file:
def test_keep_frames(self): """ Test the keep frames option. """ self.visualizer_delete_frames.render_visualization() # Verify frames were deleted self.assertFalse( ( self.visualizer_delete_frames.output_folder / "video_frames" / "frame_000000.png" ).exists() ) self.assertFalse( ( self.visualizer_delete_frames.output_folder / "video_frames" / "frame_000009.png" ).exists() ) # Verify or clean up video file if created video_file = self.visualizer_delete_frames.output_folder / "ZnVis-Video.mp4" if video_file.exists(): video_file.unlink()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CI/unit_tests/visualizer/test_headless_renderer.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
CI/unit_tests/visualizer/test_headless_renderer.py (6)
znvis/material/material.py (1)
Material(31-61)znvis/mesh/arrow.py (1)
Arrow(34-84)znvis/mesh/sphere.py (1)
Sphere(32-51)znvis/particle/particle.py (1)
Particle(34-215)znvis/particle/vector_field.py (1)
VectorField(34-150)znvis/visualizer/headless_visualizer.py (2)
Headless_Visualizer(43-206)render_visualization(194-206)
🔇 Additional comments (1)
CI/unit_tests/visualizer/test_headless_renderer.py (1)
254-265: LGTM! tearDownClass implementation is robust.The
tearDownClassmethod properly cleans up the test output directory, addressing the previous review comment. The existence check before removal makes it safe to run even if tests fail early.
2 participants
Added a headless renderer class for rendering without a graphical interface.
For the best result consider to export an appropriate view matrix for your visualization before rendering.
This class can be used analogously to the Visualizer class. An example that renders a whole simulation with given particles, vector fields in their respective lists into an output folders using a given view matrix could look like this.
Summary by CodeRabbit
New Features
Tests
Chores
Refactor