Update sprite's scale(render & collision) implement #979
Update sprite's scale(render & collision) implement #979JiepengTan wants to merge 1 commit intogoplus:devfrom
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 refactors the sprite transformation system to provide more explicit control over scaling and horizontal flipping. It introduces a dedicated boolean flag for horizontal flipping, separating it from the general scale factor, and updates the underlying engine and FFI layers to support this new approach. The changes aim to improve clarity and control over how sprites are rendered and manipulated. 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
|
JiepengTan
changed the title
![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 updates the sprite scale implementation by introducing a flipH parameter to handle horizontal flipping, and adjusts the scale calculation to ensure correct scaling behavior. The changes involve modifications across several files to accommodate the new parameter and logic.
gdspx.go
Outdated
| rot, flipH := calcRenderRotation(sprite) | ||
| scale := sprite.scale | ||
| sprite.syncSprite.UpdateTransform(x, y, rot, scale, flipH, offsetX, offsetY, isSync) |
There was a problem hiding this comment.
The scale variable is being assigned the value of sprite.scale on line 166, but it's not clear if this is the correct scale to be used in all cases. It might be more appropriate to calculate the scale based on the costume's bitmap resolution, similar to how it's done in getCostumeBoundByAlpha. This ensures consistency in scaling across different parts of the code.
Consider calculating the scale based on costume properties to ensure consistency.
|
|
||
| // scale 现在是正数，表示实际缩放大小 | ||
| scale := float64(scale64) | ||
| if scale < 0 { | ||
| scale = -scale | ||
| } |
There was a problem hiding this comment.
The code adjusts the scale to be positive if it's negative. It would be helpful to add a comment explaining why the scale can be negative in the first place and why it needs to be adjusted here. This will help future developers understand the purpose of this adjustment.
| // scale 现在是正数，表示实际缩放大小 | |
| scale := float64(scale64) | |
| if scale < 0 { | |
| scale = -scale | |
| } | |
| posOffset := Vec2{X: float64(offsetX), Y: float64(offsetY)} | |
| // scale can be negative, indicating a flip. We want the absolute value for scaling. | |
| scale := float64(scale64) | |
| if scale < 0 { | |
| scale = -scale | |
| } | |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
internal/engine/sprite.go
Outdated
| // scale 现在是正数，表示实际缩放大小 | ||
| scale := float64(scale64) | ||
| if scale < 0 { | ||
| scale = -scale | ||
| } |
There was a problem hiding this comment.
CRITICAL: Missing Input Validation
The scale parameter should be validated to prevent division-by-zero and special float values:
| // scale 现在是正数，表示实际缩放大小 | |
| scale := float64(scale64) | |
| if scale < 0 { | |
| scale = -scale | |
| } | |
| // Scale should always be positive and valid | |
| scale := float64(scale64) | |
| if scale < 0 { | |
| scale = -scale | |
| } | |
| if scale == 0 || math.IsNaN(scale) || math.IsInf(scale, 0) { | |
| scale = 1.0 // Default to safe value | |
| } |
This prevents rendering crashes and physics engine instability when scale = 0, NaN, or Infinity.
| } | ||
|
|
||
| func (pself *Sprite) UpdateTransform(x, y float64, rot float64, scale64 float64, offsetX, offsetY float64, isSync bool) { | ||
| func (pself *Sprite) UpdateTransform(x, y float64, rot float64, scale64 float64, flipH bool, offsetX, offsetY float64, isSync bool) { |
There was a problem hiding this comment.
Missing Function Documentation
This function needs comprehensive documentation explaining the new behavior:
| func (pself *Sprite) UpdateTransform(x, y float64, rot float64, scale64 float64, flipH bool, offsetX, offsetY float64, isSync bool) { | |
| // UpdateTransform updates the sprite's position, rotation, scale, and flip state. | |
| // Parameters: | |
| // - x, y: Position coordinates | |
| // - rot: Rotation in degrees | |
| // - scale64: Scale factor (always positive; horizontal flipping is controlled separately via flipH) | |
| // - flipH: Whether to flip the sprite horizontally | |
| // - offsetX, offsetY: Pivot offset coordinates | |
| // - isSync: Whether to apply changes synchronously | |
| // | |
| // Note: Scale is now always positive; horizontal flipping is controlled via flipH. | |
| func (pself *Sprite) UpdateTransform(x, y float64, rot float64, scale64 float64, flipH bool, offsetX, offsetY float64, isSync bool) { |
The signature change is breaking and requires clear documentation.
| } | ||
|
|
||
| func calcRenderRotation(p *SpriteImpl) (float64, float64) { | ||
| func calcRenderRotation(p *SpriteImpl) (float64, bool) { |
There was a problem hiding this comment.
Missing Function Documentation
Add documentation explaining return values and behavior:
| func calcRenderRotation(p *SpriteImpl) (float64, bool) { | |
| // calcRenderRotation calculates the rotation angle and horizontal flip state for rendering a sprite. | |
| // Returns: | |
| // - float64: The rotation angle in degrees | |
| // - bool: Whether the sprite should be horizontally flipped | |
| // | |
| // Behavior by rotation style: | |
| // - None: Returns 0 degrees with no flipping | |
| // - LeftRight: Returns 0 degrees with flipH set based on direction | |
| func calcRenderRotation(p *SpriteImpl) (float64, bool) { |
|
Excellent refactoring! The separation of scale and flip is a significant improvement in code clarity. Key strengths: ✅ Better semantic clarity - explicit Critical issue: Missing input validation for zero/NaN/Infinity scale values could cause rendering crashes. Please address inline comments. |
JiepengTan
force-pushed
the
pr_update_sprite_scale_impl
branch
3 times, most recently
from
997d4d6 to
b86c495
Compare
Code Review SummaryOverall, this PR successfully refactors sprite scale/flip handling with cleaner separation of concerns. The new approach (separate Critical Issue: Missing API methods in pure engine wrapper will cause compilation failures. Performance: No significant concerns. The additional FFI call overhead is negligible given low call frequency. Security: No vulnerabilities identified. Consider adding validation to prevent zero scale values (quality issue, not security). See inline comments for specific issues. |
| scale := float64(scale64) | ||
| if scale < 0 { | ||
| scale = -scale | ||
| } |
There was a problem hiding this comment.
Defensive abs() check - Consider adding validation at source
This defensive check ensures scale is positive, but suggests incomplete migration or potential bugs upstream. If getCostumeRenderScale() always returns positive values, this check is unnecessary. If negative scale can occur via ChangeSize(-value), consider validating at the source instead:
func (p *SpriteImpl) SetSize(size float64) {
if size <= 0 {
size = 0.01 // or log warning
}
// ... rest
}Additionally, zero scale could cause division-by-zero issues in pivot calculations. Consider adding a minimum threshold.
| } | ||
|
|
||
| func calcRenderRotation(p *SpriteImpl) (float64, float64) { | ||
| func calcRenderRotation(p *SpriteImpl) (float64, bool) { |
There was a problem hiding this comment.
Good improvement: Clear separation of rotation and flip state. The new signature (rotation float64, flipH bool) is more explicit and type-safe than the previous (rotation, hScale float64) approach.
| func (pself *spriteMgrImpl) SetRenderScale(obj gdx.Object, scale Vec2) {} | ||
| func (pself *spriteMgrImpl) GetRenderScale(obj gdx.Object) Vec2 { | ||
| var _ret1 Vec2 | ||
| func (pself *spriteMgrImpl) SetFlipH(obj gdx.Object, flip bool) {} |
There was a problem hiding this comment.
CRITICAL: Missing SetFlipH/IsFlipH methods in pure wrapper
The ISpriteMgr interface (pkg/gdspx/pkg/engine/interface.gen.go:206-207) requires SetFlipH and IsFlipH methods, but this pure wrapper implementation is missing them. This will cause compilation failures when building with -tags pure_engine.
Additionally, the old SetRenderScale/GetRenderScale methods still exist in this file and should be removed.
Action required: Regenerate this file or manually add:
func (pself *spriteMgrImpl) SetFlipH(obj gdx.Object, flip bool) {}
func (pself *spriteMgrImpl) IsFlipH(obj gdx.Object) bool {
var _ret1 bool
return _ret1
}And remove the SetRenderScale/GetRenderScale methods.
| pself.SetPosition(pos) | ||
| pself.SetRotation(rad) | ||
| pself.SetScale(NewVec2(scale, 1)) | ||
| pself.SetScale(NewVec2(scale, scale)) |
There was a problem hiding this comment.
Performance note: Uniform scaling (scale, scale) is better than the previous non-uniform approach. GPU rasterizers and physics engines can optimize uniform scaling more effectively.
JiepengTan
force-pushed
the
pr_update_sprite_scale_impl
branch
from
b86c495 to
3ec2f74
Compare
1 participant
related pr: goplus/godot#186