Support full tilemap export and import in SPX v2. #1109
Conversation
JiepengTan
changed the title
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 upgrades the tilemap system in SPX v2 by enabling full export and import functionality, primarily through the integration of a new C++-based tilemap parser. This change allows for a more advanced and structured tilemap format, separating decorator data into its own JSON file for better organization. The update touches various layers of the codebase, from core tilemap management logic to code generation and FFI bindings, ensuring seamless interaction between Go and the underlying C++ engine for enhanced game development capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 introduces support for a new tilemap format, which involves adding a TilemapparserMgr and updating various generated files and export configurations. The core logic changes are in tilemap.go to handle the new format and in the codegen scripts to correctly resolve manager names.
My review focuses on the non-generated code. I've identified an opportunity to refactor duplicated code in the codegen scripts to improve maintainability and fix a potential panic. I also found a recurring typo in a struct definition within internal/tilemap/tilemap.go that should be corrected for clarity and correctness. Overall, the changes look good and move towards a more robust tilemap handling system.
| // getManagerNameForFunc extracts the manager name from the function name. | ||
| func getManagerNameForFunc(funcName string, knownManagers []string) string { | ||
| prefix := "GDExtensionSpx" | ||
| str := funcName[len(prefix):] | ||
| lowerStr := strings.ToLower(str) | ||
|
|
||
| // Prefer matching against known manager names (sorted by length descending, prioritizing longer names) | ||
| if len(knownManagers) > 0 { | ||
| // Create a copy sorted by length in descending order | ||
| sortedNames := make([]string, len(knownManagers)) | ||
| copy(sortedNames, knownManagers) | ||
| sort.Slice(sortedNames, func(i, j int) bool { | ||
| return len(sortedNames[i]) > len(sortedNames[j]) | ||
| }) | ||
|
|
||
| for _, mgr := range sortedNames { | ||
| if strings.HasPrefix(lowerStr, mgr) { | ||
| return mgr | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fall back to the original logic (stop at uppercase letter) | ||
| chs := []rune{} | ||
| chs = append(chs, rune(str[0]), rune(str[1])) | ||
| for _, ch := range str[2:] { | ||
| if ch >= 'A' && ch <= 'Z' { | ||
| break | ||
| } | ||
| chs = append(chs, ch) | ||
| } | ||
| return strings.ToLower(string(chs)) | ||
| } |
There was a problem hiding this comment.
This function getManagerNameForFunc duplicates logic from GetManagerName in pkg/gdspx/cmd/codegen/generate/common/funcs.go. This can lead to maintenance issues. For example, this implementation uses an ASCII-only check for uppercase letters (ch >= 'A' && ch <= 'Z'), while the other uses the more robust unicode.IsUpper.
Additionally, the fallback logic in both functions is unsafe and will panic if the input string str has fewer than 2 characters, as it accesses str[0] and str[1] without a length check.
I recommend consolidating this logic into a single, robust function. Since common already imports clang, you could export this function from the clang package (e.g., as GetManagerNameForFunc) and reuse it in the common package.
Here is a suggested improved implementation that is safe and uses Unicode-aware checks. Please note you will need to add import "unicode" to use it.
func getManagerNameForFunc(funcName string, knownManagers []string) string {
prefix := "GDExtensionSpx"
str := funcName[len(prefix):]
lowerStr := strings.ToLower(str)
// Prefer matching against known manager names (sorted by length descending, prioritizing longer names)
if len(knownManagers) > 0 {
// Create a copy sorted by length in descending order
sortedNames := make([]string, len(knownManagers))
copy(sortedNames, knownManagers)
sort.Slice(sortedNames, func(i, j int) bool {
return len(sortedNames[i]) > len(sortedNames[j])
})
for _, mgr := range sortedNames {
if strings.HasPrefix(lowerStr, mgr) {
return mgr
}
}
}
// Fall back to the original logic (stop at uppercase letter)
if len(str) < 2 {
return strings.ToLower(str)
}
for i, ch := range str[2:] {
if unicode.IsUpper(ch) {
return strings.ToLower(str[:i+2])
}
}
return strings.ToLower(str)
}| Scale vec2 `json:"scale,omitempty"` | ||
| Position Vec2 `json:"position"` | ||
| Scale Vec2 `json:"scale,omitempty"` | ||
| Ratation float64 `json:"rotation,omitempty"` |
There was a problem hiding this comment.
There's a typo in the field name Ratation. It should be Rotation. This also affects the JSON tag. Correcting this would improve code clarity and correctness. A similar typo exists in the spriteNode struct on line 108.
| Ratation float64 `json:"rotation,omitempty"` | |
| Rotation float64 `json:"rotation,omitempty"` |
| Scale vec2 `json:"scale,omitempty"` | ||
| Position Vec2 `json:"position"` | ||
| Scale Vec2 `json:"scale,omitempty"` | ||
| Ratation float64 `json:"rotation,omitempty"` |
There was a problem hiding this comment.
There's a typo in the field name Ratation. It should be Rotation. This also affects the JSON tag.
| Ratation float64 `json:"rotation,omitempty"` | |
| Rotation float64 `json:"rotation,omitempty"` |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| Scale vec2 `json:"scale,omitempty"` | ||
| Position Vec2 `json:"position"` | ||
| Scale Vec2 `json:"scale,omitempty"` | ||
| Ratation float64 `json:"rotation,omitempty"` |
There was a problem hiding this comment.
Critical: Typo in field name
Ratation should be Rotation. While the JSON tag is correct (json:"rotation"), the Go field name is misspelled, which affects code readability and the public API since DecoratorNode is exported.
This typo also appears in:
spriteNodestruct (line 108)- Usage in
tilemap.go:125
Recommendation: Rename to Rotation throughout the codebase.
tilemap.go
Outdated
|
|
||
| // isNewFormat checks if the tilemap JSON is in the new format (version >= 1) | ||
| // New format uses C++ TileMapParser with Base64 encoded tile_map_data | ||
| func (p *gameTilemapMgr) isNewFormat(fs spxfs.Dir, path string) bool { |
There was a problem hiding this comment.
Performance: Redundant JSON parsing
The isNewFormat method performs a full JSON parse just to read the version field. This is immediately followed by another complete parse (line 72 for old format, or C++ loader for new format).
For large tilemap files (>500KB), this doubles the parsing overhead.
Recommendations:
- Parse once and inspect the version field from the parsed data
- Or use a streaming JSON parser to peek at the version field
- Cache the parsed result to avoid re-reading
| position := item.Position.ToVec2() | ||
| pivot := item.Pivot.ToVec2() | ||
| assetPath := engine.ToAssetPath("tilemaps/" + item.Path) | ||
| relativePath := path.Join(tilemapDir, item.Path) |
There was a problem hiding this comment.
Security: Potential path traversal
User-controlled item.Path from decorator.json is joined with tilemapDir without validation. An attacker could use path traversal sequences like "../../sensitive/file.png" to access assets outside the intended directory.
While filepath.Clean() is eventually called in ToAssetPath(), it happens after path concatenation.
Recommendation:
// Validate decorator path
cleanPath := filepath.Clean(item.Path)
if strings.Contains(cleanPath, "..") {
fmt.Printf("Warning: Skipping decorator with invalid path: %s\n", item.Path)
continue
}
relativePath := path.Join(tilemapDir, item.Path)
| var data DecoratorJSON | ||
| err := loadJson(&data, fs, decoratorPath) | ||
| if err != nil { | ||
| fmt.Printf("[TILEMAP] No decorator.json found at %s (this is OK if no decorators)\n", decoratorPath) |
There was a problem hiding this comment.
Code Quality: Error handling could be more specific
The message "No decorator.json found" appears for any error, not just missing files. JSON parsing errors are silently treated as "OK".
Recommendation:
if err != nil {
if os.IsNotExist(err) {
// OK - decorator.json is optional
return
}
fmt.Printf("[TILEMAP] Warning: Failed to parse decorator.json: %v\n", err)
return
}
There was a problem hiding this comment.
原来的AI-Town工程需要重新用插件导出，兼容吗？
tilemap.go
Outdated
| } | ||
|
|
||
| // isNewFormat checks if the tilemap JSON is in the new format (version >= 1) | ||
| // New format uses C++ TileMapParser with Base64 encoded tile_map_data |
There was a problem hiding this comment.
Documentation: Potentially misleading comment
The comment mentions "Base64 encoded tile_map_data" but there's no evidence in the Go code of Base64 decoding. The C++ TileMapParser receives a file path, not decoded data.
Please clarify if Base64 encoding is handled internally by the C++ parser, or remove the reference if inaccurate.
Code Review SummaryThis PR successfully adds dual-format tilemap support with good backward compatibility. However, several critical issues need attention before merge. Critical Issues
Security Concerns
Code Quality
The architecture is sound and the dual-loader approach is well-structured. Please address the critical issues, particularly the typo and the ConvertData bug, as these affect functionality and the public API. |
JiepengTan
force-pushed
the
pr_tilemap_full_support
branch
from
33bda90 to
47580b0
Compare
JiepengTan
changed the title
| position := item.Position.ToVec2() | ||
| pivot := item.Pivot.ToVec2() | ||
| assetPath := engine.ToAssetPath("tilemaps/" + item.Path) | ||
| relativePath := path.Join(tilemapDir, item.Path) |
There was a problem hiding this comment.
Security: Path Traversal Vulnerability
The item.Path from decorator JSON is joined without validation, allowing potential path traversal attacks. An attacker could craft a malicious JSON with paths like "../../../etc/passwd".
| relativePath := path.Join(tilemapDir, item.Path) | |
| // Validate path to prevent traversal attacks | |
| if !filepath.IsLocal(item.Path) { | |
| panic(fmt.Sprintf("Invalid decorator path: %s", item.Path)) | |
| } | |
| relativePath := path.Join(tilemapDir, item.Path) |
Reference: CWE-22
tilemap.go
Outdated
| // Old format: use existing Go loader | ||
| var data tm.TscnMapData | ||
| err := loadJson(&data, fs, path) | ||
| err := loadJson(&data, fs, tilemapPath) |
There was a problem hiding this comment.
Performance: Redundant JSON Parsing
The tilemap JSON is parsed twice - once in isNewFormat() to check the version, then again here. For large tilemap files, this doubles I/O and parsing overhead (30-50% slowdown).
Consider parsing once and checking the version field in memory instead.
tilemap.go
Outdated
| // New format: use C++ TileMapParser for loading tilemap | ||
| enginePath := engine.ToAssetPath(tilemapPath) | ||
| fmt.Printf("[TILEMAP] Using C++ TileMapParser for: %s\n", enginePath) | ||
| tilemapparserMgr.LoadTilemap(enginePath) |
There was a problem hiding this comment.
Code Quality: Inconsistent Error Handling
The old format panics on load failure (good), but the new format silently continues with no error checking from LoadTilemap(). This makes debugging new format failures difficult.
Consider adding error checking or at minimum logging the operation's success/failure.
| Scale vec2 `json:"scale,omitempty"` | ||
| Position Vec2 `json:"position"` | ||
| Scale Vec2 `json:"scale,omitempty"` | ||
| Ratation float64 `json:"rotation,omitempty"` |
There was a problem hiding this comment.
Code Quality: Typo in Field Name
The field is named Ratation but should be Rotation. The JSON tag is correct, but the Go field name is misspelled. This appears in both DecoratorNode and spriteNode.
If this format is already in production, add a comment acknowledging the typo. Otherwise, fix it before release.
Code Review SummaryThe tilemap export/import implementation is architecturally sound with clear separation between old and new formats. However, several critical issues need attention: Security (High Priority):
Performance:
Code Quality:
See inline comments for details and suggested fixes. |
tilemap.go
Outdated
| // New format uses C++ TileMapParser with Base64 encoded tile_map_data | ||
| func (p *gameTilemapMgr) isNewFormat(fs spxfs.Dir, path string) bool { | ||
| var versionCheck struct { | ||
| Version int `json:"version"` |
There was a problem hiding this comment.
Documentation: Missing Explanation
The asymmetric adjustments (minY - 1 vs maxX + 1) need better explanation. Why subtract 1 for minY but not minX? This suggests a coordinate system assumption that should be documented in the function comment.
| func (p *gameTilemapMgr) parseTilemap() { | ||
| // Handle new format: load decorators from separate JSON file | ||
| if p.useNewLoader { | ||
| p.loadDecoratorsFromJSON() |
There was a problem hiding this comment.
Code Quality: Missing World Size Calculation
World size is only calculated for old format (calcWorldSize() at line 174). Is this intentional for new format? If the C++ parser handles it, add a comment explaining why it's skipped.
tilemap.go
Outdated
| var versionCheck struct { | ||
| Version int `json:"version"` | ||
| } | ||
| if err := loadJson(&versionCheck, fs, path); err != nil { |
There was a problem hiding this comment.
Code Quality: Error Handling Silently Fails
When isNewFormat() encounters a JSON parsing error, it returns false, treating corrupted files as old format. This masks real issues and makes debugging harder.
Consider distinguishing between "file doesn't exist", "malformed JSON", and "old format without version field".
| ) | ||
|
|
||
| // DecoratorJSON represents the structure of decorator.json file (new format) | ||
| type DecoratorJSON struct { |
There was a problem hiding this comment.
Documentation: Version Field Semantics Missing
The Version field lacks documentation explaining:
- Valid version numbers (currently >= 1 is new format)
- What version 0 or missing means (old format)
- Future version compatibility
| type DecoratorJSON struct { | |
| // DecoratorJSON represents the structure of decorator.json file (new format) | |
| type DecoratorJSON struct { | |
| // Version indicates decorator JSON format version. | |
| // Version >= 1: new format with separate decorator.json | |
| // Version 0 or missing: old format (combined with tilemap in TscnMapData) | |
| Version int `json:"version"` | |
| Decorators []tm.DecoratorNode `json:"decorators"` | |
| } |
JiepengTan
force-pushed
the
pr_tilemap_full_support
branch
from
28f4421 to
1667962
Compare
JiepengTan
force-pushed
the
pr_tilemap_full_support
branch
from
1667962 to
7d1df08
Compare
2 participants
test project: 99-01Platform.zip
related pr: goplus/godot#232
how to export tilemap data : goplus/godot#232
MutilTilemap.mp4