Conversation
…ng it thread safe
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a cross-platform GitHub Actions workflow and refactors several modules for MSVC compatibility and improved portability.
- Add CMake-based CI matrix for Linux and Windows.
- Implement cross-platform time handling and use
std::movein key places. - Replace templated
Trianglewith concrete classes and update numeric types todouble.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/build-and-test.yml | New CI workflow targeting Ubuntu and Windows with CMake presets |
| tests/src/TransformationsTest.cpp | Include <cmath>, define M_PI, update Triangle instantiation |
| tests/src/MatrixTest.cpp | Include <math.h>, define M_PI |
| src/main.cpp | Replaced move() with std::move() for unique pointers |
| libs/Prism/src/triangle.cpp | Added new non-template Triangle and MeshTriangle implementations |
| libs/Prism/src/sphere.cpp | Removed commented code around rec.p and face‐normal setters |
| libs/Prism/src/scene.cpp | Cross-platform localtime_s/localtime_r, improved generate_filename() |
| libs/Prism/src/plane.cpp | Changed world-space hit point computation in Plane::hit |
| libs/Prism/src/mesh.cpp | Switched from Triangle<std::shared_ptr<Point3>> to MeshTriangle |
| libs/Prism/src/matrix.cpp | Cast dimension to size_t in translation/scaling loops |
| libs/Prism/src/color.cpp | Changed Color constructors to use double |
| libs/Prism/include/Prism/triangle.hpp | Removed templated Triangle definition, added forward declarations |
| libs/Prism/include/Prism/scene.hpp | Moved convert_color/operator<< to source, added move semantics |
| libs/Prism/include/Prism/mesh.hpp | Updated mesh storage to use MeshTriangle |
| libs/Prism/include/Prism/matrix.hpp | Marked nested MatrixRow and ConstMatrixRow with PRISM_EXPORT |
| libs/Prism/include/Prism/color.hpp | Updated member types to double, added integer constructor |
| libs/Prism/include/Prism/ObjReader.hpp | Fixed constructor initializer list for ObjReader |
Comments suppressed due to low confidence (3)
libs/Prism/src/plane.cpp:32
- This change bypasses the object's transform when computing the world-space intersection point. Consider restoring
transform * transformed_ray.at(t)(or equivalent) to correctly account for any object transformation.
rec.p = ray.at(t); // Ponto de volta para o espaço global
libs/Prism/src/scene.cpp:4
- The implementation uses
std::put_time, which requires<iomanip>. Please add#include <iomanip>to avoid compilation errors on some toolchains.
#include <sstream>
libs/Prism/include/Prism/ObjReader.hpp:27
- The constructor no longer initializes any members in an initializer list; ensure that all class members (e.g.,
cmap) are properly initialized or given default values to avoid uninitialized-member bugs.
ObjReader(const std::string& filename) {
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.