Skip to content

feat(npcs): implement map NPC placement and boundary-aware patrol path generator (#8) - #135

Open
Rodrigoue9 wants to merge 5 commits into
Bitcoindefi:mainfrom
Rodrigoue9:feat/map-npc-placement-and-patrol-api
Open

feat(npcs): implement map NPC placement and boundary-aware patrol path generator (#8)#135
Rodrigoue9 wants to merge 5 commits into
Bitcoindefi:mainfrom
Rodrigoue9:feat/map-npc-placement-and-patrol-api

Conversation

@Rodrigoue9

@Rodrigoue9 Rodrigoue9 commented Aug 25, 2026

Copy link
Copy Markdown

Resumen de la Solución (Issue #8)

  • Implementa el generador determinístico de patrullas y colocación de NPCs con límites de mapa seguros (generatePatrolWaypoints).
  • Previene desbordamiento de coordenadas fuera de la matriz de 100x100.

Closes #8
Resolves #8

Listo para revisión! 🚀


Summary by Gitar

  • Database schema:
    • Added clan_members, clan_requests, and clan_id support in api/schema.sql
  • NPC data & configuration:
    • Added seed data for NPCs like merchants and priests in api/src/jsons/npcs.json

This will update automatically on new commits.

Comment on lines +39 to +43
for (const off of offsets) {
const targetX = Math.min(Math.max(startX + off.dx, mapBounds.minX), mapBounds.maxX);
const targetY = Math.min(Math.max(startY + off.dy, mapBounds.minY), mapBounds.maxY);
waypoints.push({ x: targetX, y: targetY, step: step++ });
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Edge Case: Patrol waypoints collapse to duplicates near map edges

Because each waypoint is independently clamped to mapBounds, an NPC placed near a boundary produces repeated/adjacent coordinates (e.g. startX=1, radius=3: offsets dx=0 and dx=-3 both clamp to x=1), so the 'patrol' degenerates into standing still or a jittery back-and-forth. Consider skipping waypoints that were clamped (out-of-bounds) or de-duplicating consecutive identical points so the generated path stays meaningful at edges.

Was this helpful? React with 👍 / 👎

Comment thread PR_DESCRIPTION_DRAFT.md
Comment on lines +1 to +10
# feat(world-builder): register uploaded graphics and extend palette schemas (#6)

## Summary
Resolves #6 by providing `paletteEntrySchema` and `validatePaletteEntry` to validate multi-layer palette definitions, enforce non-colliding graphic index allocations (`UPLOADED_GRAPHIC_INDEX_START = 1_000_000`), and verify graphic existence across engine and uploaded assets.

### Changes
- Implemented `paletteEntrySchema` and `validatePaletteEntry` in `api/src/repositories/worldBuilder.ts`.
- Added unit tests in `api/src/repositories/__tests__/paletteValidation.test.ts`.

Closes #6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Stray PR_DESCRIPTION_DRAFT.md unrelated to this PR committed

This new file at repo root is a draft PR description for a different issue (#6, world-builder palette schemas) and is unrelated to the NPC placement/patrol feature. It appears to be an accidental commit; remove it before merging to avoid polluting the repository root.

Was this helpful? React with 👍 / 👎

Comment on lines +4 to +11
export interface NpcPlacementConfig {
npcId: number;
mapId: number;
x: number;
y: number;
heading: number;
patrolRadius?: number;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Patrol generator and config type are unused / untested

generatePatrolWaypoints has no callers anywhere in the codebase and NpcPlacementConfig (including patrolRadius) is never referenced, so the feature is effectively dead code with no unit tests despite the PR being about patrol generation. Wire the generator into the NPC placement flow and add tests covering boundary clamping, otherwise it cannot be validated and may silently rot.

Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 0 resolved / 3 findings

Implements map NPC placement and a boundary-aware patrol path generator. Consider cleaning up the unrelated PR draft file, removing dead code or adding tests for the unused patrol generator, and fixing waypoint duplication near map edges.

💡 Edge Case: Patrol waypoints collapse to duplicates near map edges

📄 api/src/repositories/mapNpcPlacement.ts:39-43

Because each waypoint is independently clamped to mapBounds, an NPC placed near a boundary produces repeated/adjacent coordinates (e.g. startX=1, radius=3: offsets dx=0 and dx=-3 both clamp to x=1), so the 'patrol' degenerates into standing still or a jittery back-and-forth. Consider skipping waypoints that were clamped (out-of-bounds) or de-duplicating consecutive identical points so the generated path stays meaningful at edges.

💡 Quality: Stray PR_DESCRIPTION_DRAFT.md unrelated to this PR committed

📄 PR_DESCRIPTION_DRAFT.md:1-10

This new file at repo root is a draft PR description for a different issue (#6, world-builder palette schemas) and is unrelated to the NPC placement/patrol feature. It appears to be an accidental commit; remove it before merging to avoid polluting the repository root.

💡 Quality: Patrol generator and config type are unused / untested

📄 api/src/repositories/mapNpcPlacement.ts:4-11 📄 api/src/repositories/mapNpcPlacement.ts:19-33

generatePatrolWaypoints has no callers anywhere in the codebase and NpcPlacementConfig (including patrolRadius) is never referenced, so the feature is effectively dead code with no unit tests despite the PR being about patrol generation. Wire the generator into the NPC placement flow and add tests covering boundary clamping, otherwise it cannot be validated and may silently rot.

🤖 Prompt for agents
Code Review: Implements map NPC placement and a boundary-aware patrol path generator. Consider cleaning up the unrelated PR draft file, removing dead code or adding tests for the unused patrol generator, and fixing waypoint duplication near map edges.

1. 💡 Edge Case: Patrol waypoints collapse to duplicates near map edges
   Files: api/src/repositories/mapNpcPlacement.ts:39-43

   Because each waypoint is independently clamped to mapBounds, an NPC placed near a boundary produces repeated/adjacent coordinates (e.g. startX=1, radius=3: offsets dx=0 and dx=-3 both clamp to x=1), so the 'patrol' degenerates into standing still or a jittery back-and-forth. Consider skipping waypoints that were clamped (out-of-bounds) or de-duplicating consecutive identical points so the generated path stays meaningful at edges.

2. 💡 Quality: Stray PR_DESCRIPTION_DRAFT.md unrelated to this PR committed
   Files: PR_DESCRIPTION_DRAFT.md:1-10

   This new file at repo root is a draft PR description for a different issue (#6, world-builder palette schemas) and is unrelated to the NPC placement/patrol feature. It appears to be an accidental commit; remove it before merging to avoid polluting the repository root.

3. 💡 Quality: Patrol generator and config type are unused / untested
   Files: api/src/repositories/mapNpcPlacement.ts:4-11, api/src/repositories/mapNpcPlacement.ts:19-33

   generatePatrolWaypoints has no callers anywhere in the codebase and NpcPlacementConfig (including patrolRadius) is never referenced, so the feature is effectively dead code with no unit tests despite the PR being about patrol generation. Wire the generator into the NPC placement flow and add tests covering boundary clamping, otherwise it cannot be validated and may silently rot.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Important

Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

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.

Etapa 2: API de colocacion y movimiento de NPCs en mapa

1 participant