feat(spx_tilemap_exporter): add Python export script and documentation#243
feat(spx_tilemap_exporter): add Python export script and documentation#243JiepengTan wants to merge 1 commit intogoplus:spx4.4.1from
Conversation
- Add export.py for automated TileMap data export workflow - Add README.md and README.zh.md with complete usage documentation - Improve export_cli.gd with better CLI support - Support Godot path via --godot arg or GODOT_PATH env var
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 enhances the SPX TileMap Exporter by introducing a new Python-based automation script, making the export workflow more streamlined and user-friendly. It also improves the existing Godot CLI script to allow for dynamic scene selection and provides comprehensive, localized documentation to ensure developers can easily set up and utilize the tool. These changes aim to simplify the process of exporting game assets from Godot to the SPX runtime. 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
|
| set_godot_path_override(args.godot) | ||
|
|
||
| # Get the directory where this script is located (works regardless of where it's executed from) | ||
| script_dir = Path(__file__).resolve().parent.parent.parent |
There was a problem hiding this comment.
Path calculation mismatch: Lines 257-260 calculate script_dir by going up 3 parent levels from export.py, but the README instructs users to place this script at addons/spx_tilemap_exporter/export.py. This inconsistency means:
- Going up 3 levels from
addons/spx_tilemap_exporter/export.pyreaches an incorrect location - The
--copyfeature will fail for users following the Quick Start guide
The comment on line 259 says "script is under tutorial/AA-00Town" which contradicts the README documentation (line 15).
Suggestion: Either fix the path calculation to match the documented structure, or clarify in the README that this script is specifically for engine developers working in the tutorial/AA-00Town/ directory.
|
|
||
|
|
||
| # Global variable to store Godot path from command line argument | ||
| _godot_path_override: Path | None = None |
There was a problem hiding this comment.
Code simplification: The global variable pattern with _godot_path_override and set_godot_path_override() adds unnecessary complexity. Consider passing the override path directly as a parameter to get_godot_path() to eliminate global state and improve testability.
|
|
||
| # Priority 1: Command line argument | ||
| if _godot_path_override: | ||
| if not _godot_path_override.exists(): |
There was a problem hiding this comment.
Duplicate validation logic: The path existence check is duplicated for both _godot_path_override and GODOT_PATH environment variable. Consider extracting this into a helper function to reduce duplication.
|
|
||
| # Execute command | ||
| try: | ||
| result = subprocess.run(cmd, cwd=str(script_dir)) |
There was a problem hiding this comment.
Missing timeout: Consider adding a timeout parameter to prevent the process from hanging indefinitely on large projects or if Godot encounters an issue:
result = subprocess.run(cmd, cwd=str(script_dir), timeout=600)| print("") | ||
|
|
||
| # Execute command | ||
| try: |
There was a problem hiding this comment.
Duplicate subprocess pattern: Both import_project() and run_export() have nearly identical subprocess error handling (try/except with return code checking). Consider extracting a helper function like run_godot_command() to eliminate this duplication.
Code Review SummaryThis PR adds comprehensive Python export tooling and documentation for the SPX TileMap Exporter. The code is well-structured with good documentation coverage. Key findings:
The implementation is solid for a development tool, with no critical security issues found. |
There was a problem hiding this comment.
Code Review
This pull request introduces a Python script to automate the TileMap data export workflow, along with comprehensive documentation in both English and Chinese. The changes also improve the existing Godot CLI export script by adding support for specifying a scene via command-line arguments.
My review focuses on improving the clarity of the new documentation and code. I've pointed out some inconsistencies in the README examples that could confuse users. I've also suggested improvements to variable naming in the new Python script for better maintainability, and a fix in the GDScript to align its behavior with the documented intention for creating export paths. Overall, this is a great addition that will significantly improve the developer experience.
| **Method 1: Specify Godot path via command line argument** | ||
|
|
||
| ```bash | ||
| python export.py --godot /path/to/godot --scene levels/level1.tscn |
There was a problem hiding this comment.
The example command for running the export script is missing the full path to the script. Assuming the user is at the project root, the command should be python addons/spx_tilemap_exporter/export.py .... This is inconsistent with the "Quick Start" section and might confuse users. This applies to other examples in this file as well.
| python export.py --godot /path/to/godot --scene levels/level1.tscn | |
| python addons/spx_tilemap_exporter/export.py --godot /path/to/godot --scene levels/level1.tscn |
| **方式一:通过命令行参数指定 Godot 路径** | ||
|
|
||
| ```bash | ||
| python export.py --godot /path/to/godot --scene levels/level1.tscn |
There was a problem hiding this comment.
运行导出脚本的示例命令缺少脚本的完整路径。假设用户位于项目根目录,命令应该是 python addons/spx_tilemap_exporter/export.py ...。这与“快速开始”部分不一致,可能会让用户感到困惑。此问题也存在于文件中的其他示例。
| python export.py --godot /path/to/godot --scene levels/level1.tscn | |
| python addons/spx_tilemap_exporter/export.py --godot /path/to/godot --scene levels/level1.tscn |
| script_dir = Path(__file__).resolve().parent.parent.parent | ||
|
|
||
| # Calculate project root directory (script is under tutorial/AA-00Town, go up two levels) | ||
| project_root = script_dir.parent.parent |
There was a problem hiding this comment.
The variable script_dir is misleadingly named. Based on its calculation (Path(__file__).resolve().parent.parent.parent), it points to the Godot project's root directory, not the script's directory. The comment on line 257 is also incorrect. Similarly, project_root is confusing as it points to the root of the monorepo, not the Godot project.
For better clarity and maintainability, I suggest renaming these variables to reflect their actual purpose throughout the file, for example:
script_dir->project_dirproject_root->monorepo_root
This would make the code easier to understand, especially the logic in copy_addon and the cwd parameter in subprocess.run.
| var export_path = scene_path.get_file().get_basename() | ||
| var export_base = "res://_export/" + export_path |
There was a problem hiding this comment.
There's a discrepancy between the comment and the implementation for generating the export path. The comment suggests that a scene path like "res://levels/level1.tscn" should result in an export path of "res://_export/levels/level1", which is great for avoiding filename conflicts.
However, the current implementation var export_path = scene_path.get_file().get_basename() will result in "res://_export/level1", losing the subdirectory structure.
To match the behavior described in the comment, you could modify the implementation.
var export_path = scene_path.replace("res://", "").trim_suffix("." + scene_path.get_extension())
var export_base = "res://_export/" + export_path
Quick Start
Export files are saved in the
res://_export/<scene_name>/directory:test project: AA-01Platform.zip