Enhance ball friction handling and update macOS CI/CD configuration#193
Open
mahi97 wants to merge 12 commits intoRoboCup-SSL:masterfrom
Open
Enhance ball friction handling and update macOS CI/CD configuration#193mahi97 wants to merge 12 commits intoRoboCup-SSL:masterfrom
mahi97 wants to merge 12 commits intoRoboCup-SSL:masterfrom
Conversation
Enhance robot management and configuration features
Fixes two bugs: - Mismatch 9: Ground friction was applied in all 3 axes (including Z), but ground friction should only act in the XY plane. - Mismatch 10: Ground friction was applied unconditionally even when the ball was airborne. Now checks ball Z position against ground level before applying friction. Agent-Logs-Url: https://github.com/mahi97/grSim/sessions/bd648558-4597-4716-8949-9375cb72d37e Co-authored-by: mahi97 <7570279+mahi97@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Replaces duplicated cfg->BallRadius() * 1.2 in the friction ground check and ball replacement positioning with a single named constant defined at the top of SSLWorld::step(). Agent-Logs-Url: https://github.com/mahi97/grSim/sessions/45d9654e-0950-4e11-a136-cc2003d2c4cb Co-authored-by: mahi97 <7570279+mahi97@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… macos-13 runner - clients/qt/CMakeLists.txt: VERSION 2.8 → 3.10 (CMake 4.x removed < 3.5 compat) - CMakeLists.txt: VERSION 3.5 → 3.10 (pre-empt upcoming removal of < 3.10 compat) - build.yaml: replace deprecated macos-13 with macos-15, clean up Homebrew paths Agent-Logs-Url: https://github.com/mahi97/grSim/sessions/63b6bfd3-c1e9-4877-b17a-5491aba569f2 Co-authored-by: mahi97 <7570279+mahi97@users.noreply.github.com>
Move the duplicated `cfg->BallRadius() * 1.2` threshold into a single private `SSLWorld::ballGroundThreshold()` const method, used by both step() and recvActions(). Agent-Logs-Url: https://github.com/mahi97/grSim/sessions/ed027e95-34cf-4b51-a993-74ad8ed20436 Co-authored-by: mahi97 <7570279+mahi97@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ake 4.x compat Protobuf 3.6.1, ODE, and vartypes have old cmake_minimum_required that CMake 4.x (on macOS runners) rejects. Pass CMAKE_POLICY_VERSION_MINIMUM=3.5 to allow these external projects to configure successfully. Agent-Logs-Url: https://github.com/mahi97/grSim/sessions/838d00c6-d230-4382-801b-9e2b0aa8ae78 Co-authored-by: mahi97 <7570279+mahi97@users.noreply.github.com>
Fix ball ground friction: skip when airborne, restrict to XY plane
ODE 0.16.4 demo_cylvssphere.cpp has double-to-float narrowing conversions that fail with the stricter Clang on macOS 15. Since we only need the ODE library (not demos or tests), disable them with ODE_WITH_DEMOS=OFF and ODE_WITH_TESTS=OFF. Agent-Logs-Url: https://github.com/mahi97/grSim/sessions/c1d5edc7-457e-40ea-a2f3-8cf678736c23 Co-authored-by: mahi97 <7570279+mahi97@users.noreply.github.com>
Fix macOS CI: update cmake_minimum_required for CMake 4.x, replace deprecated macos-13 runner
There was a problem hiding this comment.
Pull request overview
This PR modernizes the build/CI configuration (notably for newer macOS runners) and refines ball–ground interaction in the simulator by applying ground friction only when the ball is considered on the ground and only in the XY plane.
Changes:
- Introduce a centralized
SSLWorld::ballGroundThreshold()and use it for ball “on ground” detection and replacement Z positioning. - Update ground friction application to only affect XY motion when the ball is on the ground.
- Modernize build configuration: raise CMake minimum versions, adjust external-project CMake args/policies, disable ODE demos/tests, and update macOS CI matrix/env.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/sslworld.cpp |
Adds ground-threshold helper; updates friction application logic and ball replacement Z positioning. |
include/sslworld.h |
Declares the new ballGroundThreshold() helper. |
CMakeLists.txt |
Raises minimum CMake version and tweaks external VarTypes CMake args. |
cmake/modules/BuildProtobuf.cmake |
Passes additional CMake policy argument to the protobuf external build. |
cmake/modules/BuildODE.cmake |
Passes policy arg and disables ODE demos/tests for macOS compatibility. |
clients/qt/CMakeLists.txt |
Raises minimum CMake version for the Qt client build. |
.github/workflows/build.yaml |
Updates macOS build matrix and environment variables for Qt5 discovery on Homebrew ARM paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+446
to
+449
| dReal ballx, bally, ballz; | ||
| ball->getBodyPosition(ballx, bally, ballz); | ||
| (void)ballx; | ||
| (void)bally; |
There was a problem hiding this comment.
ball->getBodyPosition(ballx, bally, ballz); is only used to read ballz, and the (void)ballx/(void)bally casts are just to silence unused-variable warnings. Consider reading Z directly (e.g., via dBodyGetPosition(ball->body)[2]) or adding a helper that returns only Z to avoid the extra locals and casts.
Suggested change
| dReal ballx, bally, ballz; | |
| ball->getBodyPosition(ballx, bally, ballz); | |
| (void)ballx; | |
| (void)bally; | |
| const dReal ballz = dBodyGetPosition(ball->body)[2]; |
| # demos are not needed and fail to compile on macOS 15+ due to narrowing errors | ||
| -DODE_WITH_DEMOS=OFF | ||
| -DODE_WITH_TESTS=OFF | ||
| -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR> |
There was a problem hiding this comment.
-DCMAKE_INSTALL_PREFIX is passed twice in this ExternalProject_Add call (once as -DCMAKE_INSTALL_PREFIX:PATH=<INSTALL_DIR> and again here). Dropping the duplicate avoids ambiguity and makes it clearer which value/type is intended.
Suggested change
| -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR> |
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.
This pull request introduces several improvements to the build system and simulation physics, focusing on modernizing CMake requirements, updating macOS build support, and refining ball-ground interaction. The most significant changes are grouped below.
Build System Modernization and Compatibility:
CMakeLists.txtandclients/qt/CMakeLists.txtto ensure compatibility with newer CMake features. [1] [2].github/workflows/build.yamlto target macOS 14 and 15, and removed legacy macOS 13 build steps for a cleaner, more maintainable workflow.-DCMAKE_POLICY_VERSION_MINIMUM=3.5to external project CMake arguments forVarTypes,ODE, andprotobufto improve compatibility and suppress policy warnings. [1] [2] [3]-DODE_WITH_DEMOS=OFFand-DODE_WITH_TESTS=OFFto avoid build failures on macOS 15+.Simulation Physics Improvements:
SSLWorld:ballGroundThreshold()to consistently determine when the ball is considered "on the ground". [1] [2]