Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63f5246330
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int ncomp = 4; | ||
| sppm::Write(ppm_path, gm.Width(), gm.Height(), ncomp, (unsigned char*)pixels, yflip); | ||
| const char* npy_path = getenv("NPY_PATH"); | ||
| NP::Write(npy_path, (unsigned char*)pixels, gm.Height(), gm.Width(), 4); |
There was a problem hiding this comment.
When SOPTIX_Scene_test is used to generate NPY_PATH, this writes the downloaded OptiX buffer in raw launch order, whereas the removed PPM path passed yflip = true and the new SOPTIX_Pixels::save_npy helper still performs that same flip before NP::Write. This makes the standalone SOPTIX dump vertically inverted relative to both the previous output and the encapsulated render_npy path, so comparisons or downstream conversions from this test produce the wrong image orientation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR removes the legacy SysRap PPM write/read helpers and migrates the remaining SOPTIX pixel-dump output to the existing .npy persistence flow via NP::Write, reducing duplicate output code and API surface.
Changes:
- Removed
SPPM.hh/.ccandsppm.halong with their unit tests and CMake entries. - Replaced
SOPTIX::render_ppm/SOPTIX_Pixels::save_ppmwithrender_npy/save_npy. - Updated SOPTIX scene tests to emit
.npyarrays (viaNPY_PATH) instead of.ppmfiles (viaPPM_PATH).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sysrap/tests/SPPMTest.cc | Removes legacy SPPM test (deleted). |
| sysrap/tests/sppm_test.cc | Removes legacy sppm test (deleted). |
| sysrap/tests/SOPTIX_Scene_test.cc | Switches pixel dump output from PPM to NPY. |
| sysrap/tests/SOPTIX_Scene_Encapsulated_test.cc | Updates encapsulated render call to render_npy. |
| sysrap/tests/CMakeLists.txt | Drops SPPM test from test build sources. |
| sysrap/SPPM.hh | Removes legacy SPPM API (deleted). |
| sysrap/sppm.h | Removes legacy sppm helper API (deleted). |
| sysrap/SPPM.cc | Removes legacy SPPM implementation (deleted). |
| sysrap/SOPTIX.h | Renames render output entrypoint to render_npy and routes to save_npy. |
| sysrap/SOPTIX_Pixels.h | Replaces PPM write path with NPY write path (with vertical flip). |
| sysrap/CMakeLists.txt | Removes SPPM sources/headers from SysRap library build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const char* npy_path = getenv("NPY_PATH"); | ||
| NP::Write(npy_path, (unsigned char*)pixels, gm.Height(), gm.Width(), 4); | ||
|
|
| inline void SOPTIX_Pixels::save_npy(const char* path) | ||
| { | ||
| const int width = gm.Width(); | ||
| const int height = gm.Height(); | ||
| const int ncomp = 4; |
| inline void SOPTIX::render_npy(const char* _path) | ||
| { | ||
| const char* path = spath::Resolve(_path); | ||
|
|
||
| std::cout << "SOPTIX::render_ppm [" << ( path ? path : "-" ) << "]\n" ; | ||
| std::cout << "SOPTIX::render_npy [" << (path ? path : "-") << "]\n"; | ||
|
|
||
| if(!pix) pix = new SOPTIX_Pixels(gm) ; | ||
| render(pix->d_pixels); |
Removes the legacy SysRap PPM helpers and migrates the remaining SOPTIX pixel-dump path to the existing
.npyarraypersistence flow.
Motivation
The current render/snapshot codebase already uses
.npyarray output throughNP::Write, while the PPM helpers werelegacy image-writing utilities with little remaining use. Removing them reduces duplicate output code and shrinks the
installed SysRap API surface.
Breaking Change
SPPM.hhandsppm.hare removed.SOPTIX::render_ppm(...)should useSOPTIX::render_npy(...).SOPTIX_Pixels::save_ppm(...)should useSOPTIX_Pixels::save_npy(...).PPM_PATHshould move toNPY_PATH..ppmimage files should convert from the emitted.npypixel arrays downstream.