feat(spx_tilemap_exporter): add preview PNG export functionality#244
feat(spx_tilemap_exporter): add preview PNG export functionality#244JiepengTan wants to merge 2 commits 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
- Add preview_exporter.gd for rendering TileMapLayer-only scene snapshots - Support Ctrl+Shift+E keyboard shortcut in editor - Add --no-preview CLI option to disable preview in headless mode - Update export.py to run non-headless by default for preview support - Update README documentation (EN/ZH)
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 plugin by introducing a preview PNG export feature. This allows users to generate a visual snapshot of their TileMap layers, which is useful for quick verification or documentation. The feature is seamlessly integrated into the Godot editor with a dedicated menu option and a convenient keyboard shortcut, and also supports command-line usage with an option to disable previews for headless operations. The underlying export logic has been refactored into a new PreviewExporter class, ensuring clean separation of concerns and reusability. 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
The pull request introduces a new feature for exporting a preview PNG of TileMapLayer content, along with corresponding updates to the Python export script and documentation. The implementation includes a new PreviewExporter class to handle the rendering and saving of the preview image, and integrates this functionality into both the editor plugin and the command-line export script. The documentation has been updated to reflect these new features, including a keyboard shortcut for quick preview export. Overall, the changes are well-structured and add valuable functionality. However, there are a few areas that require attention regarding path resolution in the Python script, the accuracy of export paths in the CLI script, and a potential issue with bounding box calculation in the preview exporter.
| 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 calculation for project_root here seems incorrect for the --copy functionality. If script_dir correctly resolves to the game project root (e.g., tutorial/AA-00Town/), then script_dir.parent.parent would point two levels above the game project, which is unlikely to be the engine source root where pkg/gdspx/godot/addons/ resides. The --copy functionality is intended to copy the plugin from the engine source to the current project. This path needs to be resolved correctly, perhaps by introducing a new command-line argument or environment variable for the engine source path, or by assuming a fixed relative path from the current working directory when --copy is used.
| # e.g., "res://main.tscn" -> "res://_export/main" | ||
| # e.g., "res://levels/level1.tscn" -> "res://_export/levels/level1" | ||
| var export_path = scene_path.get_file().get_basename() | ||
| _export_base = "res://_export/" + export_path |
There was a problem hiding this comment.
The comment on this line indicates that res://levels/level1.tscn should result in an export path like res://_export/levels/level1. However, scene_path.get_file().get_basename() will only return level1, leading to an export path of res://_export/level1. This loses the directory structure of the scene path. To preserve the directory structure, you might want to use scene_path.get_base_dir().trim_prefix("res://") + "/" + scene_path.get_file().get_basename() or a similar approach to construct the export_path.
For example:
var relative_scene_path = scene_path.trim_prefix("res://")
var export_subpath = relative_scene_path.get_basename()
if relative_scene_path.get_base_dir() != "":
export_subpath = relative_scene_path.get_base_dir() + "/" + export_subpath
_export_base = "res://_export/" + export_subpath
var relative_scene_path = scene_path.trim_prefix("res://")
var export_subpath = relative_scene_path.get_basename()
if relative_scene_path.get_base_dir() != "":
export_subpath = relative_scene_path.get_base_dir() + "/" + export_subpath
_export_base = "res://_export/" + export_subpath
| ## Calculate the bounding rectangle of all renderable content in the scene | ||
| func _get_scene_bounds(node: Node, layers: Array[TileMapLayer]) -> Rect2: | ||
| var total_rect = Rect2() | ||
|
|
||
| # Calculate TileMapLayer bounds | ||
| for layer in layers: | ||
| var layer_rect = _get_tilemap_bounds(layer) | ||
| if layer_rect.has_area(): | ||
| var global_rect = layer.get_global_transform() * layer_rect | ||
| total_rect = _merge_rects(total_rect, global_rect) | ||
|
|
||
| # Collect Sprite2D bounds recursively | ||
| total_rect = _collect_sprite_bounds_recursive(node, total_rect) | ||
|
|
||
| return total_rect | ||
|
|
||
|
|
||
| ## Calculate bounds of a single TileMapLayer | ||
| func _get_tilemap_bounds(layer: TileMapLayer) -> Rect2: | ||
| if not layer or not layer.tile_set: | ||
| return Rect2() | ||
|
|
||
| var used_rect = layer.get_used_rect() | ||
| if used_rect.size == Vector2i.ZERO: | ||
| return Rect2() | ||
|
|
||
| var tile_size = layer.tile_set.tile_size | ||
|
|
||
| # Convert tile coordinates to local pixel coordinates | ||
| var top_left = layer.map_to_local(used_rect.position) | ||
| var bottom_right = layer.map_to_local(used_rect.position + used_rect.size) | ||
|
|
||
| # Adjust for half-tile offset (tiles are centered) | ||
| var half_tile = Vector2(tile_size) / 2.0 | ||
| var rect = Rect2(top_left - half_tile, bottom_right - top_left) | ||
|
|
||
| return rect | ||
|
|
||
|
|
||
| func _collect_sprite_bounds_recursive(node: Node, total_rect: Rect2) -> Rect2: | ||
| # Skip TileMapLayer nodes (already handled) | ||
| if node is TileMapLayer: | ||
| return total_rect | ||
|
|
||
| if node is Sprite2D: | ||
| var sprite = node as Sprite2D | ||
| var texture = sprite.texture | ||
| if texture: | ||
| var size = texture.get_size() | ||
| var offset = -size / 2.0 if sprite.centered else Vector2.ZERO | ||
| offset += sprite.offset | ||
| var local_rect = Rect2(offset, size) | ||
| var global_rect = sprite.get_global_transform() * local_rect | ||
| total_rect = _merge_rects(total_rect, global_rect) | ||
|
|
||
| for child in node.get_children(): | ||
| total_rect = _collect_sprite_bounds_recursive(child, total_rect) | ||
|
|
||
| return total_rect | ||
|
|
||
|
|
||
| func _merge_rects(a: Rect2, b: Rect2) -> Rect2: | ||
| if not a.has_area(): | ||
| return b | ||
| if not b.has_area(): | ||
| return a | ||
| return a.merge(b) |
There was a problem hiding this comment.
The functions _get_scene_bounds, _get_tilemap_bounds, _collect_sprite_bounds_recursive, and _merge_rects are defined here but appear to be unused. The _start_preview_export function correctly calls the static methods from PreviewExporter for calculating bounds. These unused functions should be removed to avoid dead code and potential confusion.
| var rect = Rect2(top_left - half_tile, bottom_right - top_left) | ||
|
|
There was a problem hiding this comment.
The comment "Adjust for half-tile offset (tiles are centered)" and the half_tile adjustment might be incorrect or unnecessary. Godot's map_to_local typically returns the top-left corner of a tile when given tile coordinates. If top_left is the top-left of the first tile and bottom_right is the bottom-right of the last tile, then Rect2(top_left, bottom_right - top_left) should already represent the correct bounding box. Adding half_tile to top_left might shift the origin of the calculated rectangle, potentially leading to an inaccurate bounding box for the preview image. Please verify if this adjustment is truly needed for accurate bounding box calculation.
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| ## Calculate the bounding rectangle of all renderable content in the scene | ||
| func _get_scene_bounds(node: Node, layers: Array[TileMapLayer]) -> Rect2: | ||
| var total_rect = Rect2() | ||
|
|
||
| # Calculate TileMapLayer bounds | ||
| for layer in layers: | ||
| var layer_rect = _get_tilemap_bounds(layer) | ||
| if layer_rect.has_area(): | ||
| var global_rect = layer.get_global_transform() * layer_rect | ||
| total_rect = _merge_rects(total_rect, global_rect) | ||
|
|
||
| # Collect Sprite2D bounds recursively | ||
| total_rect = _collect_sprite_bounds_recursive(node, total_rect) | ||
|
|
||
| return total_rect | ||
|
|
||
|
|
||
| ## Calculate bounds of a single TileMapLayer | ||
| func _get_tilemap_bounds(layer: TileMapLayer) -> Rect2: | ||
| if not layer or not layer.tile_set: | ||
| return Rect2() | ||
|
|
||
| var used_rect = layer.get_used_rect() | ||
| if used_rect.size == Vector2i.ZERO: | ||
| return Rect2() | ||
|
|
||
| var tile_size = layer.tile_set.tile_size | ||
|
|
||
| # Convert tile coordinates to local pixel coordinates | ||
| var top_left = layer.map_to_local(used_rect.position) | ||
| var bottom_right = layer.map_to_local(used_rect.position + used_rect.size) | ||
|
|
||
| # Adjust for half-tile offset (tiles are centered) | ||
| var half_tile = Vector2(tile_size) / 2.0 | ||
| var rect = Rect2(top_left - half_tile, bottom_right - top_left) | ||
|
|
||
| return rect | ||
|
|
||
|
|
||
| func _collect_sprite_bounds_recursive(node: Node, total_rect: Rect2) -> Rect2: | ||
| # Skip TileMapLayer nodes (already handled) | ||
| if node is TileMapLayer: | ||
| return total_rect | ||
|
|
||
| if node is Sprite2D: | ||
| var sprite = node as Sprite2D | ||
| var texture = sprite.texture | ||
| if texture: | ||
| var size = texture.get_size() | ||
| var offset = -size / 2.0 if sprite.centered else Vector2.ZERO | ||
| offset += sprite.offset | ||
| var local_rect = Rect2(offset, size) | ||
| var global_rect = sprite.get_global_transform() * local_rect | ||
| total_rect = _merge_rects(total_rect, global_rect) | ||
|
|
||
| for child in node.get_children(): | ||
| total_rect = _collect_sprite_bounds_recursive(child, total_rect) | ||
|
|
||
| return total_rect | ||
|
|
||
|
|
||
| func _merge_rects(a: Rect2, b: Rect2) -> Rect2: | ||
| if not a.has_area(): | ||
| return b | ||
| if not b.has_area(): | ||
| return a | ||
| return a.merge(b) | ||
|
|
There was a problem hiding this comment.
Dead Code - Should Remove
These helper functions (_get_scene_bounds, _get_tilemap_bounds, _collect_sprite_bounds_recursive, _merge_rects) are never used. The code already uses PreviewExporter static methods instead (lines 121-122, 137).
Recommendation: Remove these 68 lines to reduce maintenance burden.
| func _parse_scene_argument() -> String: | ||
| """Parse --scene argument from command line, return default if not specified""" | ||
| # Use get_cmdline_user_args() for arguments after "--" separator | ||
| var args = OS.get_cmdline_user_args() | ||
|
|
||
| # Look for --scene argument | ||
| for i in range(args.size()): | ||
| if args[i] == "--scene" and i + 1 < args.size(): | ||
| var scene_arg = args[i + 1] | ||
| # Validate the scene path | ||
| if not scene_arg.begins_with("res://"): | ||
| scene_arg = "res://" + scene_arg | ||
| print("Using scene from command line: ", scene_arg) | ||
| return scene_arg | ||
|
|
||
| # Return default if not specified | ||
| return DEFAULT_SCENE_PATH |
There was a problem hiding this comment.
Security: Path Traversal Risk
Scene path from command line is not validated for path traversal sequences (../). While Godot's res:// protocol provides some protection, explicit validation is recommended as defense-in-depth.
Example Attack:
godot -s export_cli.gd -- --scene "../../other_project/secret.tscn"Recommendation: Validate that the resolved path contains no .. sequences and stays within the project directory.
| # Get the directory where this script is located (works regardless of where it's executed from) | ||
| 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.
Confusing Path Calculation
The comment says "go up two levels" but the code actually goes up 5 levels total (parent.parent.parent for script_dir, then parent.parent for project_root). This assumes a very specific directory structure that may not match all project layouts.
Recommendation: Either:
- Document this directory structure requirement clearly in the help text
- Or make path calculation more robust (e.g., search for
project.godotfile)
| # Global variable to store Godot path from command line argument | ||
| _godot_path_override: Path | None = None | ||
|
|
||
|
|
||
| def set_godot_path_override(path: str | None) -> None: | ||
| """Set the Godot path override from command line argument""" | ||
| global _godot_path_override | ||
| if path: | ||
| _godot_path_override = Path(path) | ||
| else: | ||
| _godot_path_override = None |
There was a problem hiding this comment.
Unnecessary Global Variable
Using a global variable for configuration adds complexity and could cause issues if the script is imported as a module.
Recommendation: Pass godot_path as a parameter through the function chain instead.
| ## Calculate the bounding rectangle of all TileMapLayers in the scene (instance method wrapper) | ||
| func get_scene_bounds(node: Node, layers: Array[TileMapLayer] = []) -> Rect2: | ||
| return PreviewExporter.calc_tilemap_bounds(layers) | ||
|
|
||
|
|
||
| ## Calculate bounds of a single TileMapLayer (instance method wrapper) | ||
| func get_tilemap_bounds(layer: TileMapLayer) -> Rect2: | ||
| return PreviewExporter.calc_single_layer_bounds(layer) | ||
|
|
There was a problem hiding this comment.
Unnecessary Wrapper Methods
These instance method wrappers add no value - they simply delegate to static methods. Note that get_scene_bounds ignores its node parameter entirely.
Recommendation: Remove these wrappers and commit to a fully static utility class design.
| | Parameter | Description | | ||
| |-----------|-------------| | ||
| | `--godot PATH` | Specify the Godot executable path (takes priority over `GODOT_PATH` environment variable) | | ||
| | `--copy` | Copy the latest plugin from `pkg/gdspx/godot/addons/` to the current project directory for rapid plugin development iteration | | ||
|
|
||
| **Workflow:** | ||
|
|
||
| 1. Modify the plugin code in `pkg/gdspx/godot/addons/spx_tilemap_exporter/` | ||
| 2. Navigate to the test project directory (e.g., `tutorial/AA-00Town/`) | ||
| 3. Run `python export.py --copy` to automatically copy the plugin and test the export | ||
|
|
There was a problem hiding this comment.
Documentation Inaccuracy
The --copy parameter documentation doesn't clearly explain the specific directory structure requirement. The export.py script assumes a specific layout where it goes up 5 directory levels to find pkg/gdspx/godot/addons/, which will only work in particular project structures.
Recommendation: Document the exact required directory structure or make the script more flexible.
|
|
||
| ## License | ||
|
|
||
| SPX Team © 2026 |
There was a problem hiding this comment.
Minor: License Year
The license states "© 2026" which appears to be a future year. Should this be 2024 or 2025?
Code Review SummaryThis PR adds useful preview PNG export functionality with comprehensive documentation. However, there are several noteworthy issues: Critical Issues:
Important Issues:
Positive Aspects:
Overall, the functionality is solid but code duplication and dead code should be addressed before merging. |
1 participant
Quick Start
Export preview png is saved in the
res://_export/<scene_name>/preview.png:test project: AA-01Platform.zip
eg：
