Conversation
…e y TikZ Se añade un nuevo trabajo en CI para MemorySanitizer con clang, validando el estado por defecto de Regular_Polygon. Se corrigen e inicializan todos los miembros de Regular_Polygon en el constructor por defecto, se valida que lado y ángulo sean finitos, y se corrige get_last_segment(). QuadTree ahora rechaza capacidad cero y bounds inválidos, mejora la eliminación con colapso de ancestros y maneja puntos fuera de rango. TikZ: detección automática de librerías (patterns), escalado para Beamer, y compilación de documentos LaTeX generados en pruebas. point_utils se envuelve en namespace Aleph para mantener compatibilidad. Segment y Ellipse: intersección trata regiones rellenas cerradas. RangeTree2D y AABBTree: debug_snapshot produce índices válidos. Breaking changes: ninguna.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughSe actualizan intersecciones geométricas, polígonos regulares, QuadTree y snapshots. También se amplía la exportación TikZ/LaTeX, se añade MemorySanitizer a CMake y CI, y se actualiza la versión a 5.10.0. ChangesCorrecciones geométricas y estructuras espaciales
Exportación TikZ y LaTeX
MemorySanitizer y automatización CI
Metadatos de versión
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Tikz_Scene
participant Tikz_Plane
participant LaTeX_Output
Tikz_Scene->>Tikz_Plane: recopila librerías requeridas
Tikz_Plane-->>Tikz_Scene: devuelve patterns si corresponde
Tikz_Scene->>LaTeX_Output: genera preámbulo y escena escalada
LaTeX_Output-->>Tikz_Scene: documento standalone, beamer u overlay
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: a978b5de18
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends Aleph-w’s geometry tooling and CI coverage: it adds a targeted MemorySanitizer probe, improves TikZ/Beamer export robustness (including dependency detection and scaling), and tightens several geometry/data-structure edge cases with new regression tests.
Changes:
- Add a new MemorySanitizer CI job + CMake option and a dedicated
Regular_Polygondefault-state probe. - Improve TikZ exports by auto-detecting required TikZ libraries (e.g.,
patterns), adding Beamer picture scaling, and optionally compiling generated LaTeX in tests/examples. - Fix/clarify geometry behavior (Regular_Polygon initialization/validation, Segment–Triangle/Ellipse filled-region clipping) and QuadTree validation/collapse behavior; harden debug snapshots for RangeTree2D and AABBTree.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| VERSION.txt | Bumps library version to 5.10.0. |
| CHANGELOG.md | Adds a 5.10.0 release section. |
| .release-please-manifest.json | Updates release-please manifest version to 5.10.0. |
| CMakeLists.txt | Adds ALEPH_USE_MEMORY_SANITIZER and MSan compiler/linker flags. |
| .github/workflows/ci.yml | Introduces a memory-sanitizer CI job and wires it into required checks. |
| docs/ci_sanitizer_policy.md | Documents the new MemorySanitizer job policy and scope. |
| README.md | Documents the new CMake option for MemorySanitizer. |
| Tests/CMakeLists.txt | Adds a targeted MSan probe target and optional LaTeX toolchain detection for tests. |
| polygon.H | Fully initializes Regular_Polygon default state; validates inputs; fixes closing segment logic and closure semantics. |
| Tests/polygon_test.cc | Extends Regular_Polygon tests for default state, invalid inputs, and get_last_segment(). |
| quadtree.H | Adds capacity/bounds validation, duplicate-point handling, safer operations on uninitialized roots, and ancestor collapsing on removal. |
| quadnode.H | Enforces non-empty regions on construction/region set; improves point removal state updates. |
| Tests/quadtree_test.cc | Adds coverage for invalid bounds/capacity, out-of-bounds removals, duplicate-point behavior, and ancestor collapsing. |
| point.H | Updates Segment–Triangle/Ellipse intersection semantics to treat shapes as closed filled regions; fixes rotated ellipse extremal points. |
| Tests/point_test.cc | Adds regression tests for the new filled-region clipping behavior. |
| geom_algorithms.H | Fixes debug snapshot construction to avoid invalidated references; reserves correctly and stores valid child indices. |
| Tests/geom_algorithms_test_decomp_rangetree_visibility_shortestpath_arrangement.cc | Adds a large RangeTree2D debug snapshot validity test + rotated ellipse extremal-point test. |
| Tests/geom_algorithms_test_boolean_3d_serializer_aabb_misc.cc | Adds a large AABBTree debug snapshot validity test. |
| point_utils.H | Wraps legacy helpers in namespace Aleph and re-exports to preserve source compatibility. |
| tikzgeom.H | Adds required TikZ library detection (patterns) and uses rotated ellipse extremal points for bbox. |
| Tests/tikzgeom_test.cc | Asserts that pattern fill correctly reports patterns dependency. |
| tikzgeom_scene.H | Adds dependency unioning across overlay steps, Beamer scaling via \\scalebox, and includes required packages/libraries. |
| Tests/tikzgeom_algorithms_test.cc | Adds tests for dependency emission and optional LaTeX compilation smoke tests when toolchain is available. |
| Examples/CMakeLists.txt | Optionally compiles maintained TikZ example documents when a LaTeX toolchain is detected. |
Contributor
Cellular-automata perf gatebaseline CPU
Lower wall time is better; Δ is current vs. baseline. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
quadtree.H (1)
150-161: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimizar
contains_onlymediante un retorno temprano.El método actual evalúa secuencialmente todos los puntos almacenados en el nodo hoja al invocar la función de orden superior
for_each_point, incluso si la discrepancia ocurre en el primer elemento. Dado que esta comprobación se ejecuta frecuentemente durante las inserciones en los límites de capacidad, iterar sobre la lista entera puede causar degradación asintótica (llegando a comportamientos de$O(N^2)$ al insertar múltiples elementos en escenarios con capacidades ajustadas dinámicamente o alta cantidad de duplicados).Se recomienda utilizar directamente el iterador interno para poder abortar la ejecución inmediatamente al detectar una discrepancia y retornar
falseen$O(1)$ .♻️ Propuesta de refactorización
- /** `@brief` Return true when every point stored in a leaf equals `@p` p. */ - [[nodiscard]] static bool contains_only(Node * node, const Point & p) - { - assert(node != nullptr and node->is_leaf()); - bool all_equal = true; - node->for_each_point([&](const Point & point) - { - if (point != p) - all_equal = false; - }); - return all_equal; - } + /** `@brief` Return true when every point stored in a leaf equals `@p` p. */ + [[nodiscard]] static bool contains_only(Node * node, const Point & p) + { + assert(node != nullptr and node->is_leaf()); + for (DynList<Point>::iterator it(node->get_points_set()); it.has_curr(); it.next_ne()) + { + if (it.get_curr_ne() != p) + return false; + } + return true; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@quadtree.H` around lines 150 - 161, Update the static contains_only method to iterate through the leaf node’s points using its internal iterator rather than for_each_point, returning false immediately when a point differs from p and true only after all points match. Preserve the existing node validity assertion and equality semantics..github/workflows/ci.yml (2)
902-905: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFijar
persist-credentials: falseen el checkout.zizmor señala que este nuevo job no desactiva la persistencia de credenciales en el checkout, dejando el token de git disponible en
.git/configdurante toda la ejecución del job (patrón "artipacked").🔒️ Fix propuesto
- name: Checkout uses: actions/checkout@v7 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 902 - 905, Update the actions/checkout step in the new job to set persist-credentials to false, ensuring the checkout token is not retained in the repository’s Git configuration while the job runs.Source: Linters/SAST tools
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiscrepancia entre la supresión real de cppcheck y lo documentado.
--suppress=nullPointer:polygon.Hsuprime el diagnósticonullPointerpara todo el archivopolygon.H, pero la política documenta esta supresión como específica dePolygon::remove_vertex. Con el alcance actual, cualquier futuronullPointergenuino en otra parte depolygon.Hquedaría silenciado sin que la documentación lo refleje.
.github/workflows/ci.yml#L916-925: usar una supresión más estrecha, p. ej.--suppress=nullPointer:polygon.H:<línea>o un comentario// cppcheck-suppress nullPointerjusto sobre la línea afectada enPolygon::remove_vertex, en lugar de suprimir el archivo completo.docs/ci_sanitizer_policy.md#L70-82: si se mantiene la supresión a nivel de archivo, actualizar el texto para no implicar una granularidad por función que el comando no ofrece.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 1, Reduce the cppcheck suppression used by the CI configuration around the Polygon::remove_vertex exception so it applies only to the affected line or statement, using a line-specific suppression or an inline cppcheck-suppress comment instead of suppressing all of polygon.H. Keep docs/ci_sanitizer_policy.md aligned with the resulting suppression scope; only describe function-specific behavior if the implementation actually provides it.tikzgeom_scene.H (1)
98-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLógica de deduplicación de librerías TikZ duplicada entre
tikzgeom.Hytikzgeom_scene.H. Ambas clases implementan de forma independiente el mismo patrón "añadir a unArray<std::string>si no está ya presente".
tikzgeom_scene.H#L98-L114:append_unique_librariesreimplementa la deduplicación elemento-por-elemento sobre unArrayde origen.tikzgeom.H#L405-L413:append_unique_libraryimplementa el mismo patrón para un único string.Se podría extraer una función libre compartida (p. ej. en un header de utilidades comunes) que ambas clases reutilicen, evitando mantener dos implementaciones equivalentes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tikzgeom_scene.H` around lines 98 - 114, Extrae la lógica común de deduplicación de bibliotecas a una función libre compartida en un header de utilidades, y reutilízala desde tikzgeom_scene.H:98-114 mediante append_unique_libraries y desde tikzgeom.H:405-413 mediante append_unique_library; elimina ambas implementaciones duplicadas, preservando el comportamiento para fuentes completas y elementos individuales.point.H (1)
1929-1950: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLambda
coveredduplicada entreintersects_with(Triangle&)eintersection_with(Triangle&).La misma lógica (punto cubierto por el triángulo o alguna de sus aristas) se redefine idéntica en ambas funciones. Si una se modifica (p. ej. para manejar un nuevo caso límite) y la otra no, ambas dejarán de estar sincronizadas.
♻️ Extraer helper compartido
+ private: + static bool triangle_covers_point(const Triangle & t, + const DynList<Segment> & edges, + const Point & p) + { + if (t.contains(p)) + return true; + for (const Segment & edge : edges) + if (edge.contains(p)) + return true; + return false; + } + inline bool Segment::intersects_with(const Triangle & t) const { const auto edges = t.edges(); - const auto covered = [&t, &edges](const Point & p) - { - if (t.contains(p)) - return true; - for (const Segment & edge : edges) - if (edge.contains(p)) - return true; - return false; - }; + const auto covered = [&t, &edges](const Point & p) + { return triangle_covers_point(t, edges, p); }; if (covered(src_) or covered(tgt_)) return true;(aplicar el mismo cambio en
intersection_with(const Triangle&))Also applies to: 1991-2007
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@point.H` around lines 1929 - 1950, Extrae la lógica duplicada de la lambda covered en Segment::intersects_with(const Triangle&) y Segment::intersection_with(const Triangle&) a un helper compartido que determine si un Point está cubierto por el triángulo o cualquiera de sus aristas. Reutiliza ese helper en ambas funciones y conserva exactamente el comportamiento actual para los vértices, puntos interiores y puntos sobre las aristas.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Examples/CMakeLists.txt`:
- Around line 84-116: Agregar el script Examples/verify_latex_example.cmake
referenciado por _VERIFY_LATEX_EXAMPLE y ejecutado por
add_tikz_latex_example_test(). El script debe validar el ejemplo indicado por
EXAMPLE_PATH usando PDFLATEX_PATH, OUTPUT_DIR y MODE, y devolver un fallo claro
cuando la compilación o la verificación del PDF no sean correctas, para que las
pruebas TikzExamples.* funcionen al activar el toolchain LaTeX.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 902-905: Update the actions/checkout step in the new job to set
persist-credentials to false, ensuring the checkout token is not retained in the
repository’s Git configuration while the job runs.
- Line 1: Reduce the cppcheck suppression used by the CI configuration around
the Polygon::remove_vertex exception so it applies only to the affected line or
statement, using a line-specific suppression or an inline cppcheck-suppress
comment instead of suppressing all of polygon.H. Keep
docs/ci_sanitizer_policy.md aligned with the resulting suppression scope; only
describe function-specific behavior if the implementation actually provides it.
In `@point.H`:
- Around line 1929-1950: Extrae la lógica duplicada de la lambda covered en
Segment::intersects_with(const Triangle&) y Segment::intersection_with(const
Triangle&) a un helper compartido que determine si un Point está cubierto por el
triángulo o cualquiera de sus aristas. Reutiliza ese helper en ambas funciones y
conserva exactamente el comportamiento actual para los vértices, puntos
interiores y puntos sobre las aristas.
In `@quadtree.H`:
- Around line 150-161: Update the static contains_only method to iterate through
the leaf node’s points using its internal iterator rather than for_each_point,
returning false immediately when a point differs from p and true only after all
points match. Preserve the existing node validity assertion and equality
semantics.
In `@tikzgeom_scene.H`:
- Around line 98-114: Extrae la lógica común de deduplicación de bibliotecas a
una función libre compartida en un header de utilidades, y reutilízala desde
tikzgeom_scene.H:98-114 mediante append_unique_libraries y desde
tikzgeom.H:405-413 mediante append_unique_library; elimina ambas
implementaciones duplicadas, preservando el comportamiento para fuentes
completas y elementos individuales.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4bdcb9d0-75a8-4530-a42c-fa7417cdaaf7
📒 Files selected for processing (24)
.github/workflows/ci.yml.release-please-manifest.jsonCHANGELOG.mdCMakeLists.txtExamples/CMakeLists.txtREADME.mdTests/CMakeLists.txtTests/geom_algorithms_test_boolean_3d_serializer_aabb_misc.ccTests/geom_algorithms_test_decomp_rangetree_visibility_shortestpath_arrangement.ccTests/point_test.ccTests/polygon_test.ccTests/quadtree_test.ccTests/tikzgeom_algorithms_test.ccTests/tikzgeom_test.ccVERSION.txtdocs/ci_sanitizer_policy.mdgeom_algorithms.Hpoint.Hpoint_utils.Hpolygon.Hquadnode.Hquadtree.Htikzgeom.Htikzgeom_scene.H
…e y TikZ Se añade un nuevo trabajo en CI para MemorySanitizer con clang, validando el estado por defecto de Regular_Polygon. Se corrigen e inicializan todos los miembros de Regular_Polygon en el constructor por defecto, se valida que lado y ángulo sean finitos, y se corrige get_last_segment(). QuadTree ahora rechaza capacidad cero y bounds inválidos, mejora la eliminación con colapso de ancestros y maneja puntos fuera de rango. TikZ: detección automática de librerías (patterns), escalado para Beamer, y compilación de documentos LaTeX generados en pruebas. point_utils se envuelve en namespace Aleph para mantener compatibilidad. Segment y Ellipse: intersección trata regiones rellenas cerradas. RangeTree2D y AABBTree: debug_snapshot produce índices válidos. Breaking changes: ninguna.
…ests Tests/regular_polygon_msan_probe.cc and Examples/verify_latex_example.cmake were referenced by CMakeLists.txt in d9d3016 but never committed, so the memory-sanitizer CI job failed with "Cannot find source file". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e y TikZ Se añade un nuevo trabajo en CI para MemorySanitizer con clang, validando el estado por defecto de Regular_Polygon. Se corrigen e inicializan todos los miembros de Regular_Polygon en el constructor por defecto, se valida que lado y ángulo sean finitos, y se corrige get_last_segment(). QuadTree ahora rechaza capacidad cero y bounds inválidos, mejora la eliminación con colapso de ancestros y maneja puntos fuera de rango. TikZ: detección automática de librerías (patterns), escalado para Beamer, y compilación de documentos LaTeX generados en pruebas. point_utils se envuelve en namespace Aleph para mantener compatibilidad. Segment y Ellipse: intersección trata regiones rellenas cerradas. RangeTree2D y AABBTree: debug_snapshot produce índices válidos. Breaking changes: ninguna.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
polygon.H:1207
Regular_Polygon::is_closed()changed from astaticmethod that always returned true to an instance method returningnum_vertex_ >= 3. This is a public API/behavior change: any downstream call sites usingRegular_Polygon::is_closed()(static) will stop compiling, and code relying on the old “always closed” semantics will change behavior. This also conflicts with the PR description’s claim of “Breaking changes: ninguna” unless explicitly documented as intentional.
/** @brief Check if the polygon is a valid closed regular polygon.
* @return True for constructed polygons and false for the default empty state.
* @note This is an instance query because closure depends on whether the
* object represents a constructed polygon or the default empty state.
*/
[[nodiscard]] bool is_closed() const noexcept { return num_vertex_ >= 3; }
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.
…e y TikZ
Se añade un nuevo trabajo en CI para MemorySanitizer con clang, validando el estado por defecto de Regular_Polygon. Se corrigen e inicializan todos los miembros de Regular_Polygon en el constructor por defecto, se valida que lado y ángulo sean finitos, y se corrige get_last_segment(). QuadTree ahora rechaza capacidad cero y bounds inválidos, mejora la eliminación con colapso de ancestros y maneja puntos fuera de rango. TikZ: detección automática de librerías (patterns), escalado para Beamer, y compilación de documentos LaTeX generados en pruebas. point_utils se envuelve en namespace Aleph para mantener compatibilidad. Segment y Ellipse: intersección trata regiones rellenas cerradas. RangeTree2D y AABBTree: debug_snapshot produce índices válidos.
Breaking changes: ninguna.
Summary by CodeRabbit
Nuevas funcionalidades
Correcciones
Documentación y calidad