Conversation
Was less scarier than I thought
- Use standard math library that's now cross platform to remove PI hard-codes and standardise other features - Use std::ranges - Added "concepts" - Added templates Honestly, this was me experimenting with C++ after many years so I went ham
There was a problem hiding this comment.
Pull request overview
This PR updates the C++ codebase to use the C++20 standard and modernizes the build system, replacing hardcoded PI constants with std::numbers::pi_v<float> and refactoring the growth information system from pointer-based polymorphism to std::variant.
Changes:
- Updated CMake minimum version to 3.15 and C++ standard to C++20 across all build configurations
- Replaced hardcoded PI constants with C++20's
std::numbers::pi_v<float> - Refactored
GrowthInfofrom a pointer-based inheritance hierarchy to astd::variant<std::monostate, BranchGrowthInfo, BioNodeInfo>, eliminating dynamic allocations
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Version bump to 5.3.1 |
| m_tree/tests/main.cpp | Enhanced test coverage for BranchFunction and GrowthFunction |
| m_tree/tests/CMakeLists.txt | Updated to CMake 3.15-3.31 and C++20 |
| m_tree/source/utilities/GeometryUtilities.hpp | Removed hardcoded PI definition, added <numbers> include |
| m_tree/source/utilities/GeometryUtilities.cpp | Replaced M_PI with std::numbers::pi_v<float>, added std prefix to abs() |
| m_tree/source/tree_functions/base_types/TreeFunction.hpp | Added C++20 concept TreeFunctionType |
| m_tree/source/tree_functions/base_types/Property.hpp | Added C++20 concept PropertyFunction and renamed parameters for clarity |
| m_tree/source/tree_functions/GrowthFunction.hpp | Removed BioNodeInfo class definition (moved to GrowthInfo.hpp) |
| m_tree/source/tree_functions/GrowthFunction.cpp | Removed #pragma once, refactored to use std::variant with std::get |
| m_tree/source/tree_functions/CrownShape.hpp | Replaced hardcoded PI constants with std::numbers::pi_v<float> |
| m_tree/source/tree_functions/BranchFunction.hpp | Removed BranchGrowthInfo class definition (moved to GrowthInfo.hpp) |
| m_tree/source/tree_functions/BranchFunction.cpp | Refactored to use std::variant, added designated initializers, replaced PI constants |
| m_tree/source/tree/Node.hpp | Changed growthInfo from std::unique_ptr<GrowthInfo> to GrowthInfo (variant) |
| m_tree/source/tree/GrowthInfo.hpp | Restructured to use std::variant with BranchGrowthInfo and BioNodeInfo structs |
| m_tree/source/meshers/manifold_mesher/ManifoldMesher.hpp | Made attribute name strings const, added <string> include |
| m_tree/source/meshers/manifold_mesher/ManifoldMesher.cpp | Replaced PI constants, added designated initializers |
| m_tree/source/meshers/base_types/TreeMesher.hpp | Added C++20 concept Mesher |
| m_tree/source/CMakeLists.txt | Updated to CMake 3.15-3.31 and C++20 |
| m_tree/python_bindings/CMakeLists.txt | Updated to CMake 3.15-3.31 and C++20 |
| m_tree/pyproject.toml | Version bump to 5.3.1 |
| m_tree/install.py | Removed deprecated CMake policy argument |
| m_tree/dependencies/pybind11 | Updated pybind11 submodule to newer commit |
| m_tree/CMakeLists.txt | Updated to CMake 3.15-3.31 and C++20 |
| blender_manifest.toml | Version bump to 5.3.1 |
| VERSION | Version bump to 5_3_1 |
Comments suppressed due to low confidence (1)
m_tree/source/tree_functions/BranchFunction.cpp:1
- Missing space before multiplication operator. The expression
pi_v<float>)*child_radial_nshould bepi_v<float>) * child_radial_nfor better readability.
#include <iostream>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
m_tree/source/tree_functions/BranchFunction.cpp:1
- Missing space around the
*operator. Should bestd::numbers::pi_v<float>) * child_radial_nfor consistency with code style.
#include <iostream>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Still debugging but working my way through
Description
Generated Description
This pull request introduces several important updates to the codebase, focusing primarily on modernizing C++ standards, improving type safety and maintainability, updating dependencies, and making minor version bumps across the project. The most significant changes are grouped below:
C++ Modernization and Type Safety
CMakeLists.txtfiles, enabling modern language features and better tooling support. [1] [2] [3]Mesherconcept inTreeMesher.hpp, improving compile-time interface checks for meshers.GrowthInfobase class with astd::variant-basedGrowthInfotype, and updated all usages to usestd::get/std::get_if, removing dynamic casts and heap allocations for better type safety and performance. [1] [2] [3] [4] [5] [6]Dependency and Build System Updates
pybind11submodule to a newer commit, ensuring compatibility with the latest Python bindings and C++ standards.-DCMAKE_POLICY_VERSION_MINIMUM=3.5flag from the build script, as the minimum CMake version is now 3.15.Code Quality and Consistency
M_PIwith C++20'sstd::numbers::pi_v<float>, and updated related calculations throughoutManifoldMesher.cppandBranchFunction.cppfor improved portability and clarity. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]ManifoldMesher::AttributeNamesconst, improving immutability and clarity.Version Bumps
5.3.1in all relevant files (VERSION,blender_manifest.toml,pyproject.toml) to reflect these changes. [1] [2] [3]These changes collectively modernize the codebase, improve safety and maintainability, and update dependencies for a smoother development experience.