Skip to content

isometric tilemap rendering added#3

Open
pankaj-bind wants to merge 2 commits into
OpenSword:mainfrom
pankaj-bind:issue-1
Open

isometric tilemap rendering added#3
pankaj-bind wants to merge 2 commits into
OpenSword:mainfrom
pankaj-bind:issue-1

Conversation

@pankaj-bind

@pankaj-bind pankaj-bind commented Nov 15, 2025

Copy link
Copy Markdown

PR #2 I'm close to fulfilling all the requirements

image

@ggondim

ggondim commented Nov 19, 2025

Copy link
Copy Markdown
Member

Hi @pankaj-bind, when you think you're done, please mention me at this PR. I'm considering joining forces to make it happen in issue #1.

@SaurabhCodesAI

Copy link
Copy Markdown

Hi @ggondim and @pankaj-bind!

I reviewed PR #3 in detail. @pankaj-bind has built a really solid foundation here, the architecture is clean, the chunk system works well, and the documentation is Great work!

I found some bugs that need fixing before merge:

Critical Issues:

  1. Camera rotation (Q/E keys) - input actions defined but not wired up
  2. Highlight cursor positioning - appears in wrong location due to transform bug
  3. Only 3 pitch levels implemented instead of 4 (missing TOP_DOWN at 90°)

Missing Features:
4. Block placement/removal has no controller support (only mouse)
5. Block animation system exists but renderer doesn't use it
6. Chunk auto loading defined but not implemented
7. Undo/redo doesn't preserve block height

Code Quality:
8. Several unused functions (_create_colored_texture, trigger_block_walk_over)
9. chunk_load_distance variable defined but never used

I'd love to collaborate with @pankaj-bind to fix these and complete the implementation. I can:

  • Fix all the critical bugs
  • Add missing TOP_DOWN pitch
  • Complete controller support
  • Implement chunk auto loading
  • Add comprehensive testing

@pankaj-bind would you prefer I fork your branch and submit fixes as a PR to you, or should we coordinate on fixing these together?

@ggondim given the additional scope, how would you like to structure the bounty? The MVP is 70% there, but these fixes will take another 8-10 hours to do properly.

@pankaj-bind

Copy link
Copy Markdown
Author

@SaurabhCodesAI let's coordinate on fixing these together

@ggondim

ggondim commented Nov 24, 2025

Copy link
Copy Markdown
Member

@SaurabhCodesAI, that's an excellent outcome from the review. I can't precisely gauge the workload required for this, but I'm considering duplicating the bounty so that each of you receives the same amount. If you prefer, you can also suggest a new one following the README instructions. Just let me know. Additionally, I'm working on a new backlog/roadmap with more features.

@SaurabhCodesAI

SaurabhCodesAI commented Nov 24, 2025

Copy link
Copy Markdown

Hey @ggondim!

Works for me, duplicating the bounty is fair. Let's do it.

@pankaj-bind, here's the split:

You handle:

  • Animation system hookup - your get_current_frame() just needs calling from _render_block()
  • Code cleanup - unused functions if any

I'll take:

  • Q/E rotation wiring DONE
  • Camera transform bug fix DONE - Fixed screen_to_world() to apply reverse heading rotation
  • 4th pitch level (90° top down) DONE - All 4 levels verifying and documenting
  • Controller support for place/remove DONE - A/B buttons mapped (Home/End on keyboard)
  • Chunk auto loading implementation DONE - Dynamic loading within 3 chunk radius
  • Undo/redo height fix DONE - Added height preservation to action dictionary

You're building on your stuff, I'm handling the system integration. Sound good?

Let me know if you want PRs to your branch or we work in parallel.

@ggondim, my tasks are complete and tested in Godot 4.5.1. Demo video and final polish will come in the next phase to ensure we hit 100% of the README requirements. Happy to discuss the full scope, also curious about the roadmap you mentioned!

@SaurabhCodesAI

SaurabhCodesAI commented Nov 26, 2025

Copy link
Copy Markdown

Hey @pankaj-bind updated the task split above with your remaining items (animation hookup, code cleanup). Let's wrap this up!
Also, My tasks are done. Can you add me as a collaborator to your fork so I can push the changes to issue-1? Thanks!

@pankaj-bind

Copy link
Copy Markdown
Author

@SaurabhCodesAI You did an excellent job.
Thanks.
Soon, I'll finish code cleanup, and it's done.

@SaurabhCodesAI

Copy link
Copy Markdown

Thanks @pankaj-bind! Great working with you. Let me know if you need any help with the cleanup.

@pankaj-bind

Copy link
Copy Markdown
Author

Hey @ggondim, everything looks good to me. @SaurabhCodesAI has done an excellent job. I believe this PR is now ready to be merged.

@pankaj-bind

Copy link
Copy Markdown
Author

@ggondim please review

@MyTH-zyxeon

Copy link
Copy Markdown

Hi @ggondim and @pankaj-bind, I did a static review pass on PR #3 against issue #1's README checklist. I have not run the Godot project locally in this pass, so please treat this as a focused acceptance checklist rather than a runtime approval.

Useful merge-readiness points:

  • The PR does add the main Godot scaffold, world API, block registry, chunk model, renderer, camera controls, lighting/fog systems, UI controls, and documentation. That looks directionally aligned with the MVP scope discussed on the issue.
  • I would still verify controller support before merge: project.godot defines place_block and remove_block actions with joypad buttons, but InteractionSystem._input() currently handles raw InputEventMouseButton events directly. That means controller place/remove input may not go through the declared InputMap actions.
  • I would verify hover/pick accuracy after pan/zoom/rotation: _update_mouse_position() stores viewport mouse coordinates, then IsometricCamera.screen_to_world() subtracts get_screen_center_position(). Since those are not obviously in the same coordinate space, block highlighting may drift once the camera moves or zooms unless a runtime smoke test proves it.
  • Chunk loading may need one acceptance pass with real camera movement: Main._process() approximates the camera world position with camera.position.x / 100.0 and camera.position.y / 100.0 instead of reusing the isometric camera projection. After heading/pitch/zoom changes, that can load/unload chunks that do not match the visible world center.
  • The README asks for visual attributes such as animation. BlockType.get_current_frame() exists, but IsometricRenderer._render_block() currently takes textures from the renderer's textures dictionary and does not appear to use block.block_type.texture or animation frames. If animation is considered in scope for this bounty, this may need either wiring or an explicit MVP deferral.

Suggested acceptance path: record the requested Godot demo video showing (1) keyboard and controller-equivalent place/remove paths, (2) hover/select accuracy after camera pan/zoom/rotation/pitch, (3) chunks loading around the visible camera center, and (4) whether block animation is included or intentionally out of this first bounty slice. That should make the bounty review much easier without requiring another broad duplicate implementation.

@pankaj-bind

Copy link
Copy Markdown
Author

Please do not waste your valuable time here and do not expect any reply from maintaner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants