Move coord convert from c++ => go (invert y) #973
Conversation
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 significantly refactors the engine's coordinate system management by migrating Y-axis inversion logic from C++ to Go. It establishes a dedicated adapter layer that transparently handles coordinate translations between the SPX (Y-up) and Godot (Y-down) systems across various core manager functionalities. This change centralizes a critical transformation, promoting consistency and simplifying future development efforts related to coordinate handling. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the coordinate system conversion logic by moving it from C++ to Go, which is a positive architectural change. The new coord_adapter.go file centralizes the Y-axis inversion needed to align SPX's coordinate system with Godot's. My review includes a critical fix for a potential panic in the new adapter code due to an unsafe type assertion. I've also included a few medium-severity suggestions to improve the clarity and consistency of comments related to these coordinate conversions.
| // This conversion should be handled at upper layer due to gdx.Array access limitations | ||
| func (pself *NavigationMgrImpl) FindPath(from Vec2, to Vec2, withJump bool) gdx.Array { | ||
| var arr = pself.navigationMgrImpl.FindPath(flipY(from), flipY(to), withJump) | ||
| result := arr.([]float32) | ||
| if result == nil { | ||
| return nil | ||
| } | ||
| // flipY to convert godot coord to spx coord | ||
| for i := 0; i+1 < len(result); i += 2 { | ||
| result[i+1] = -result[i+1] | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
The type assertion arr.([]float32) on line 159 is unsafe and will cause a panic if arr is nil or not of type []float32. You should use the "comma ok" idiom to safely perform the type assertion. Additionally, the comment on line 156 is now misleading since the conversion is handled within this function, and should be removed or updated.
| // This conversion should be handled at upper layer due to gdx.Array access limitations | |
| func (pself *NavigationMgrImpl) FindPath(from Vec2, to Vec2, withJump bool) gdx.Array { | |
| var arr = pself.navigationMgrImpl.FindPath(flipY(from), flipY(to), withJump) | |
| result := arr.([]float32) | |
| if result == nil { | |
| return nil | |
| } | |
| // flipY to convert godot coord to spx coord | |
| for i := 0; i+1 < len(result); i += 2 { | |
| result[i+1] = -result[i+1] | |
| } | |
| return result | |
| } | |
| // The returned path coordinates are converted to the SPX coordinate system. | |
| func (pself *NavigationMgrImpl) FindPath(from Vec2, to Vec2, withJump bool) gdx.Array { | |
| arr := pself.navigationMgrImpl.FindPath(flipY(from), flipY(to), withJump) | |
| if arr == nil { | |
| return nil | |
| } | |
| result, ok := arr.([]float32) | |
| if !ok { | |
| // Consider logging an error here as this is an unexpected type. | |
| return arr | |
| } | |
| // flipY to convert godot coord to spx coord | |
| for i := 0; i+1 < len(result); i += 2 { | |
| result[i+1] = -result[i+1] | |
| } | |
| return result | |
| } |
| ary[3] = -engine.ConvertToInt64(p.PosY) // here need flipY to convert spx coord to godot coord | ||
| ary[4] = engine.ConvertToInt64(p.NormalX) | ||
| ary[5] = engine.ConvertToInt64(p.NormalY) | ||
| ary[5] = -engine.ConvertToInt64(p.NormalY) |
There was a problem hiding this comment.
The comment on line 88 is a bit informal. Also, NormalY on line 90 is also flipped but lacks a comment. For better clarity and maintainability, it's good practice to use consistent and descriptive comments for coordinate system conversions.
| ary[3] = -engine.ConvertToInt64(p.PosY) // here need flipY to convert spx coord to godot coord | |
| ary[4] = engine.ConvertToInt64(p.NormalX) | |
| ary[5] = engine.ConvertToInt64(p.NormalY) | |
| ary[5] = -engine.ConvertToInt64(p.NormalY) | |
| ary[3] = -engine.ConvertToInt64(p.PosY) // Flip Y for Godot's coordinate system (Y-down). | |
| ary[4] = engine.ConvertToInt64(p.NormalX) | |
| ary[5] = -engine.ConvertToInt64(p.NormalY) // Flip Y for Godot's coordinate system (Y-down). |
| p.PosY = -engine.ConvertToFloat64(dataAry[3]) // here need flipY to convert to godot coord to spx coord | ||
| p.NormalX = engine.ConvertToFloat64(dataAry[4]) | ||
| p.NormalY = engine.ConvertToFloat64(dataAry[5]) | ||
| p.NormalY = -engine.ConvertToFloat64(dataAry[5]) |
There was a problem hiding this comment.
The comment on line 106 has a typo ("to godot coord to spx coord") and is a bit informal. The same conversion is done for NormalY on line 108. Using a clear and consistent comment style for these conversions will make the code easier to understand.
| p.PosY = -engine.ConvertToFloat64(dataAry[3]) // here need flipY to convert to godot coord to spx coord | |
| p.NormalX = engine.ConvertToFloat64(dataAry[4]) | |
| p.NormalY = engine.ConvertToFloat64(dataAry[5]) | |
| p.NormalY = -engine.ConvertToFloat64(dataAry[5]) | |
| p.PosY = -engine.ConvertToFloat64(dataAry[3]) // Flip Y to convert from Godot (Y-down) to SPX (Y-up). | |
| p.NormalX = engine.ConvertToFloat64(dataAry[4]) | |
| p.NormalY = -engine.ConvertToFloat64(dataAry[5]) // Flip Y to convert from Godot (Y-down) to SPX (Y-up). |
Code Review SummaryExcellent refactoring that centralizes coordinate conversion logic. The approach is architecturally sound with good separation of concerns. Critical issues:
Positives:
Performance impact is negligible. |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| // This conversion should be handled at upper layer due to gdx.Array access limitations | ||
| func (pself *NavigationMgrImpl) FindPath(from Vec2, to Vec2, withJump bool) gdx.Array { | ||
| var arr = pself.navigationMgrImpl.FindPath(flipY(from), flipY(to), withJump) | ||
| result := arr.([]float32) |
There was a problem hiding this comment.
<!-- codeagent-review-id: pr-973 -->
Critical: Type assertion panic risk
This unsafe type assertion will panic if arr is not []float32, creating a denial-of-service vulnerability.
| result := arr.([]float32) | |
| result, ok := arr.([]float32) | |
| if \!ok || result == nil { |
| // ============================================================================ | ||
|
|
||
| // FindPath returns array: [x0, y0, x1, y1, x2, y2, ...] | ||
| // This conversion should be handled at upper layer due to gdx.Array access limitations |
There was a problem hiding this comment.
Documentation inaccuracy
This comment states "conversion should be handled at upper layer" but the conversion IS done here (lines 163-165). Unlike RaycastWithDetails, this function actually converts the array elements.
| // This conversion should be handled at upper layer due to gdx.Array access limitations | |
| // FindPath returns array: [x0, y0, x1, y1, x2, y2, ...] | |
| // Input coordinates are converted via flipY, and output array Y values are also | |
| // converted here by negating all odd-indexed elements (y-coordinates). |
| return nil | ||
| } | ||
| // flipY to convert godot coord to spx coord | ||
| for i := 0; i+1 < len(result); i += 2 { |
There was a problem hiding this comment.
Inconsistent approach + minor optimization
This inline Y-negation is inconsistent with the flipY() helper used everywhere else.
Also, simplify the loop to start at index 1 to avoid the addition in the condition:
| for i := 0; i+1 < len(result); i += 2 { | |
| for i := 1; i < len(result); i += 2 { | |
| result[i] = -result[i] | |
| } |
| } | ||
|
|
||
| // RaycastWithDetails returns array: [collide, sprite_gid, pos.x, pos.y, normal.x, normal.y] | ||
| // This conversion should be handled at upper layer due to gdx.Array access limitations |
There was a problem hiding this comment.
Misleading documentation
The comment is confusing. Input coordinates ARE converted here via flipY(), but output array Y values are converted in physic.go:tryRaycastResult(). Clarify what's happening where:
| // This conversion should be handled at upper layer due to gdx.Array access limitations | |
| // RaycastWithDetails returns array: [collide, sprite_gid, pos.x, pos.y, normal.x, normal.y] | |
| // Input coordinates are converted here via flipY, but output array pos/normal Y values | |
| // must be converted at the upper layer (see physic.go:tryRaycastResult) due to gdx.Array | |
| // access limitations preventing direct element modification. |
| p.SpriteId = dataAry[1] | ||
| p.PosX = engine.ConvertToFloat64(dataAry[2]) | ||
| p.PosY = engine.ConvertToFloat64(dataAry[3]) | ||
| p.PosY = -engine.ConvertToFloat64(dataAry[3]) // here need flipY to convert to godot coord to spx coord |
There was a problem hiding this comment.
Grammar error in comment
| p.PosY = -engine.ConvertToFloat64(dataAry[3]) // here need flipY to convert to godot coord to spx coord | |
| p.PosY = -engine.ConvertToFloat64(dataAry[3]) // flipY to convert from Godot coord to SPX coord |
| p.PosY = -engine.ConvertToFloat64(dataAry[3]) // here need flipY to convert to godot coord to spx coord | ||
| p.NormalX = engine.ConvertToFloat64(dataAry[4]) | ||
| p.NormalY = engine.ConvertToFloat64(dataAry[5]) | ||
| p.NormalY = -engine.ConvertToFloat64(dataAry[5]) |
There was a problem hiding this comment.
Same grammar fix needed
| p.NormalY = -engine.ConvertToFloat64(dataAry[5]) | |
| p.NormalY = -engine.ConvertToFloat64(dataAry[5]) // flipY to convert from Godot coord to SPX coord |
JiepengTan
force-pushed
the
pr_move_coord_convert_to_go_invery
branch
from
08672e3 to
274d417
Compare
JiepengTan
force-pushed
the
pr_move_coord_convert_to_go_invery
branch
from
274d417 to
6e752e4
Compare
JiepengTan
force-pushed
the
pr_move_coord_convert_to_go_invery
branch
from
6358b96 to
a63b98e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively centralizes the coordinate system conversion logic by introducing an adapter layer that inverts the Y-axis. This is a good architectural improvement. The implementation is mostly solid, but I've found a critical issue in FindPath that could lead to a panic due to an unsafe type assertion. I've also provided a couple of medium-severity suggestions to improve code consistency and maintainability by refactoring RaycastWithDetails to be more consistent with FindPath and updating a misleading comment.
| var arr = pself.navigationMgrImpl.FindPath(flipY(from), flipY(to), withJump) | ||
| result := arr.([]float32) | ||
| if result == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The type assertion arr.([]float32) is unsafe and will cause a panic if arr is nil or has a different underlying type. This can lead to a crash. Please use the two-value type assertion to handle these cases gracefully.
arr := pself.navigationMgrImpl.FindPath(flipY(from), flipY(to), withJump)
if arr == nil {
return nil
}
result, ok := arr.([]float32)
if !ok || result == nil {
return nil
}| func (pself *PhysicMgrImpl) RaycastWithDetails(from Vec2, to Vec2, ignoreSprites gdx.Array, collisionMask int64, collideWithAreas bool, collideWithBodies bool) gdx.Array { | ||
| return pself.physicMgrImpl.RaycastWithDetails(flipY(from), flipY(to), ignoreSprites, collisionMask, collideWithAreas, collideWithBodies) | ||
| } |
There was a problem hiding this comment.
For consistency with FindPath and to better centralize the coordinate conversion logic, consider handling the Y-coordinate flipping for the returned array within this function. This would make the adapter layer fully responsible for coordinate translations. This change would also require removing the manual Y-flipping from the ToArray and tryRaycastResult functions in physic.go, and updating the comments on lines 187-188.
| func (pself *PhysicMgrImpl) RaycastWithDetails(from Vec2, to Vec2, ignoreSprites gdx.Array, collisionMask int64, collideWithAreas bool, collideWithBodies bool) gdx.Array { | |
| return pself.physicMgrImpl.RaycastWithDetails(flipY(from), flipY(to), ignoreSprites, collisionMask, collideWithAreas, collideWithBodies) | |
| } | |
| func (pself *PhysicMgrImpl) RaycastWithDetails(from Vec2, to Vec2, ignoreSprites gdx.Array, collisionMask int64, collideWithAreas bool, collideWithBodies bool) gdx.Array { | |
| arr := pself.physicMgrImpl.RaycastWithDetails(flipY(from), flipY(to), ignoreSprites, collisionMask, collideWithAreas, collideWithBodies) | |
| if arr == nil { | |
| return nil | |
| } | |
| result, ok := arr.([]int64) | |
| if !ok { | |
| // This case is unexpected, returning the original array. Consider logging an error. | |
| return arr | |
| } | |
| if len(result) >= 6 { | |
| // result[3] is pos.y, result[5] is normal.y | |
| // Convert from Godot's coordinate system (y-down) to SPX's (y-up). | |
| result[3] = -result[3] | |
| result[5] = -result[5] | |
| } | |
| return result | |
| } |
| // FindPath returns array: [x0, y0, x1, y1, x2, y2, ...] | ||
| // This conversion should be handled at upper layer due to gdx.Array access limitations |
There was a problem hiding this comment.
This comment is misleading. The coordinate conversion is handled within this function, not at an upper layer. Please update the comment to accurately describe the function's behavior.
| // FindPath returns array: [x0, y0, x1, y1, x2, y2, ...] | |
| // This conversion should be handled at upper layer due to gdx.Array access limitations | |
| // FindPath returns an array of path coordinates: [x0, y0, x1, y1, ...]. | |
| // The coordinates are converted from Godot's system (Y-down) to SPX's system (Y-up). |
| // This conversion should be handled at upper layer due to gdx.Array access limitations | ||
| func (pself *NavigationMgrImpl) FindPath(from Vec2, to Vec2, withJump bool) gdx.Array { | ||
| var arr = pself.navigationMgrImpl.FindPath(flipY(from), flipY(to), withJump) | ||
| result := arr.([]float32) |
There was a problem hiding this comment.
Security/Reliability Issue: Unchecked Type Assertion
This unchecked type assertion will panic if the underlying type is not []float32, causing a potential denial of service. Use the safe two-value form:
result, ok := arr.([]float32)
if !ok || result == nil {
return nil
}
| // ============================================================================ | ||
|
|
||
| // FindPath returns array: [x0, y0, x1, y1, x2, y2, ...] | ||
| // This conversion should be handled at upper layer due to gdx.Array access limitations |
There was a problem hiding this comment.
Documentation Issue: Misleading Comment
The comment states "This conversion should be handled at upper layer" but the function actually DOES perform output array conversion in lines 293-294. The comment from RaycastWithDetails was likely copied but doesn't apply here. Consider:
// FindPath returns array: [x0, y0, x1, y1, x2, y2, ...]
// Both input and output coordinates are converted here.
// Output Y-coordinates (at odd indices) are inverted to convert from Godot to SPX coordinate system.
| } | ||
|
|
||
| // RaycastWithDetails returns array: [collide, sprite_gid, pos.x, pos.y, normal.x, normal.y] | ||
| // This conversion should be handled at upper layer due to gdx.Array access limitations |
There was a problem hiding this comment.
Documentation Issue: Incomplete Comment
The comment is accurate that output conversion happens at the upper layer, but should clarify that only INPUT coordinates are converted here. The returned array's Y-coordinates (indices 3 and 5) must be handled by the caller:
// RaycastWithDetails returns array: [collide, sprite_gid, pos.x, pos.y, normal.x, normal.y]
// Note: Input coordinates (from, to) are converted here, but the returned array
// Y-coordinates (pos.y at index 3, normal.y at index 5) must be converted by the
// caller due to gdx.Array access limitations. See physic.go:tryRaycastResult().
| ) | ||
|
|
||
| // flipY flips the Y coordinate to convert between SPX and Godot coordinate systems | ||
| func flipY(pos Vec2) Vec2 { |
There was a problem hiding this comment.
Performance Concern: Frequent Allocations in Hot Path
The flipY function creates a new Vec2 struct on every call. Since this is called for every position/velocity operation (in rendering and physics updates), this creates significant memory pressure. At 60 FPS with 100 sprites, this could generate ~15,000-25,000 allocations/sec (~6-8 KB/frame).
Consider optimizing for hot paths. One approach is to modify the copy in-place:
func flipY(pos Vec2) Vec2 {
pos.Y = -pos.Y
return pos
}Or consider pointer-based APIs for frequently-called methods like GetPosition/SetPosition to avoid repeated allocations in Get-Modify-Set patterns (e.g., AddPos calls GetPosition then SetPosition).
| pself.spriteMgrImpl.AddImpulse(obj, flipY(impulse)) | ||
| } | ||
|
|
||
| func (pself *SpriteMgrImpl) GetGravity(obj gdx.Object) float64 { |
There was a problem hiding this comment.
Missing Documentation
Gravity negation requires explanation. Unlike vectors, gravity is a scalar, so the sign flip is conceptually different. Add a comment:
// GetGravity returns gravity with inverted sign to match SPX's upward Y-axis convention.
// In SPX, positive Y is up, so positive gravity means upward force. In Godot, positive
// Y is down, so positive gravity means downward force. The sign inversion maintains
// consistent physical behavior across both coordinate systems.
func (pself *SpriteMgrImpl) GetGravity(obj gdx.Object) float64 {
return -pself.spriteMgrImpl.GetGravity(obj)
}
Code Review SummaryExcellent architectural improvement moving coordinate conversions from C++ to Go! The adapter pattern provides clear separation of concerns. However, several noteworthy issues need attention: Critical:
Important:
Recommendations:
See inline comments for details. |
1 participant
related pr: goplus/godot#184