-
Notifications
You must be signed in to change notification settings - Fork 2
Move coord convert to go invert Y #184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: spx4.4.1
Are you sure you want to change the base?
Move coord convert to go invert Y #184
Conversation
4c02cd6 to
f5dd327
Compare
Summary of ChangesHello @JiepengTan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on migrating coordinate transformation logic from C++ to Go, aiming for a unified implementation across the SPX project. The changes involve inverting the Y-axis in various managers to ensure consistency in coordinate handling. This refactoring impacts sprite management, physics, scene management, tile drawing, debugging, input handling, and pathfinding, leading to a more maintainable and coherent codebase. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively moves the Y-coordinate inversion logic from C++ to Go, simplifying the C++ side of the FFI. The changes are consistently applied across multiple manager classes, including sprite, physics, scene, and input management, by removing all instances of y-coordinate flipping. This aligns perfectly with the stated goal of unifying the coordinate transformation approach. Additionally, a new comprehensive guide for the AI assistant, CLAUDE.md, is introduced. My review includes a suggestion to improve the reusability of this new documentation file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Summary
Clean refactoring that successfully removes Y-axis coordinate conversions from C++ layer. The changes are consistent and maintain code correctness. However, critical security and performance issues require attention before merging.
🔴 Critical Issues
1. Integer Overflow in Float-to-Int Conversion
- Location:
modules/spx/spx_utils.h:9-11(used throughout collision/physics code) - Issue:
spx_float_to_int()can overflow when coordinate values exceed ±922,337,203,685 - Impact: Undefined behavior, incorrect calculations, potential exploits
- CWE-190: Integer Overflow or Wraparound
- Fix: Add bounds checking before multiplication
2. Missing Null Pointer Validation
- Location:
modules/spx/spx_camera_mgr.cpp:78-86 - Issue:
get_global_camera_rect()doesn't validate viewport pointer before dereferencing - Impact: Crash/DoS during scene transitions
- CWE-476: NULL Pointer Dereference
- Fix: Add
ERR_FAIL_NULL_V(vp, Rect2())check
⚠️ Performance Concerns
3. Redundant Coordinate Conversions in Nested Loops
- Location:
modules/spx/spx_sprite_mgr.cpp:920-921, 997-998 - Issue:
_to_image_coord()called twice per pixel in collision detection (O(n²) calls) - Impact: For 100×100 overlap = 20,000 function calls with allocations
- Fix: Pre-compute transform parameters and use incremental arithmetic
4. Unnecessary Vector2 Allocations
- Location:
modules/spx/spx_sprite_mgr.cpp:870-872 - Issue: 3 Vector2 allocations per
_to_image_coord()call - Impact: 6N allocations for pixel collision checks
- Fix: Reuse
xposand modify in-place
📝 Documentation Issues
5. Missing Architectural Documentation
- Issue: No comments explaining that coordinate conversion moved from C++ to Go
- Impact: Future maintainers won't understand the design change
- Fix: Add comments to key functions like
get_position(),set_velocity(), etc.
6. Undocumented FFI Boundary
- Location:
platform/web/js/engine/gdspx.util.js - Issue: No documentation on coordinate system conventions at Go/JS/C++ boundaries
- Fix: Add JSDoc comments explaining Y-axis handling
✅ Positive Aspects
- Consistent removal of Y-axis inversions across all modules
- Clean separation of concerns (conversion logic now in Go layer)
- No breaking changes to internal C++ logic
- Good use of object pooling patterns
Recommendations
|
#187 需要在合并后 修改,里面涉及到了 y 的变动 |
The goal is to migrate the coordinate transformation from C++ to Go and unify the implementation approach (Invert Y)
1. spx_sprite_mgr.cpp: 9 conversions
set/getmethods2. spx_physic_mgr.cpp: 6 conversions
3. spx_scene_mgr.cpp: 5 conversions
4. spx_draw_tiles.cpp/h: 8 conversions
flip_y()function5. spx_debug_mgr.cpp: 4 conversions
6. spx_input_mgr.cpp: 1 conversion
7. spx_path_finder.cpp: 2 conversions
Conversion Patterns
pos.y = -pos.yorGdVec2(pos.x, -pos.y)return GdVec2(val.x, -val.y)flip_y()(used inspx_draw_tiles)