Skip to content

feat(skills): water tile fishing difficulty and catch rate - #166

Open
Rodrigoue9 wants to merge 32 commits into
Bitcoindefi:mainfrom
Rodrigoue9:feat/fishing-success-rate
Open

feat(skills): water tile fishing difficulty and catch rate#166
Rodrigoue9 wants to merge 32 commits into
Bitcoindefi:mainfrom
Rodrigoue9:feat/fishing-success-rate

Conversation

@Rodrigoue9

@Rodrigoue9 Rodrigoue9 commented Aug 26, 2026

Copy link
Copy Markdown

feat(skills): water tile fishing difficulty and catch rate

  • Game engine module implementation for OpenAO web MMORPG.
  • Strictly typed and zero external dependencies.

Ready for review! 🚀


Summary by Gitar

  • Game features and modules:
    • Added fishing difficulty and catch rate calculation in api/src/lib/fishingRate.ts
    • Added mineral vein depletion and respawn tracking in api/src/lib/miningNode.ts
    • Implemented weapon and armor durability loss calculations in api/src/lib/itemDurability.ts
    • Added stamina depletion and recovery logic for resting versus running in api/src/lib/staminaRegen.ts
  • Engine and world systems:
    • Implemented deterministic dungeon instance key generation in api/src/lib/dungeonKey.ts
    • Added seasonal guild score and ranking accumulator in api/src/lib/guildRanking.ts
    • Implemented 2D spatial audio volume attenuation by distance in api/src/lib/audioDistance.ts
    • Added branching dialogue tree node traversal engine in api/src/lib/dialogueEngine.ts
    • Implemented server tick day-night ambient lighting curve in api/src/lib/dayNightLight.ts
    • Added dynamic rain, fog, and sun weather state machine in api/src/lib/weatherMachine.ts

This will update automatically on new commits.

* Bitcoindefi/OpenAO - Chat Flood Rate Limiter
*/
export class ChatRateLimiter {
private userMessageTimestamps = new Map<string, 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.

💡 Performance: ChatRateLimiter map grows unbounded per user

userMessageTimestamps keeps a Map entry for every userId that ever sent a message and never evicts stale/empty entries. In a long-running server this leaks memory proportional to the number of distinct users seen. Prune entries whose filtered timestamp array is empty (e.g. if (timestamps.length === 0) this.userMessageTimestamps.delete(userId)), or periodically sweep the map.

Was this helpful? React with 👍 / 👎

Comment thread api/src/lib/critStrike.ts
/**
* Bitcoindefi/OpenAO - critical-strike
*/
export function calcCrit(agi: number): { chance: number; mult: number } { return { chance: agi * 0.01, mult: 1.5 }; }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Bug: calcCrit returns chance >1 for high agility

calcCrit computes chance = agi * 0.01 with no upper bound, so any agility above 100 yields a probability greater than 1.0 (and negative agi yields a negative chance). If callers treat this as a 0–1 roll probability the crit will effectively always/never trigger. Clamp the result, e.g. chance: Math.min(1, Math.max(0, agi * 0.01)).

Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Aug 26, 2026

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

Adds core game engine features including fishing mechanics, weather states, stamina systems, and spatial audio. Consider addressing unbounded growth in ChatRateLimiter and PotionCooldownManager, alongside refining calcCrit agility scaling.

💡 Performance: ChatRateLimiter map grows unbounded per user

📄 api/src/lib/chatRateLimiter.ts:5 📄 api/src/lib/chatRateLimiter.ts:14-22 📄 api/src/lib/potionCooldown.ts:5 📄 api/src/lib/potionCooldown.ts:7-11

userMessageTimestamps keeps a Map entry for every userId that ever sent a message and never evicts stale/empty entries. In a long-running server this leaks memory proportional to the number of distinct users seen. Prune entries whose filtered timestamp array is empty (e.g. if (timestamps.length === 0) this.userMessageTimestamps.delete(userId)), or periodically sweep the map.

💡 Bug: calcCrit returns chance >1 for high agility

📄 api/src/lib/critStrike.ts:4

calcCrit computes chance = agi * 0.01 with no upper bound, so any agility above 100 yields a probability greater than 1.0 (and negative agi yields a negative chance). If callers treat this as a 0–1 roll probability the crit will effectively always/never trigger. Clamp the result, e.g. chance: Math.min(1, Math.max(0, agi * 0.01)).

🤖 Prompt for agents
Code Review: Adds core game engine features including fishing mechanics, weather states, stamina systems, and spatial audio. Consider addressing unbounded growth in ChatRateLimiter and PotionCooldownManager, alongside refining calcCrit agility scaling.

1. 💡 Performance: ChatRateLimiter map grows unbounded per user
   Files: api/src/lib/chatRateLimiter.ts:5, api/src/lib/chatRateLimiter.ts:14-22, api/src/lib/potionCooldown.ts:5, api/src/lib/potionCooldown.ts:7-11

   `userMessageTimestamps` keeps a Map entry for every userId that ever sent a message and never evicts stale/empty entries. In a long-running server this leaks memory proportional to the number of distinct users seen. Prune entries whose filtered timestamp array is empty (e.g. `if (timestamps.length === 0) this.userMessageTimestamps.delete(userId)`), or periodically sweep the map.

2. 💡 Bug: calcCrit returns chance >1 for high agility
   Files: api/src/lib/critStrike.ts:4

   `calcCrit` computes `chance = agi * 0.01` with no upper bound, so any agility above 100 yields a probability greater than 1.0 (and negative agi yields a negative chance). If callers treat this as a 0–1 roll probability the crit will effectively always/never trigger. Clamp the result, e.g. `chance: Math.min(1, Math.max(0, agi * 0.01))`.

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

@leocagli

Copy link
Copy Markdown
Collaborator

Freno acá antes de que sigas invirtiendo tiempo, porque el problema no es el código.

El estado del repositorio

100 PR abiertos
 42 de Rodrigoue9
 41 tuyos

Ochenta y tres de dos personas. Y hay siete títulos que existen dos veces con autores
distintos: los dos escribieron lo mismo sin saberlo. Ese trabajo duplicado no lo puede
recuperar nadie.

Ninguno cita una issue

Revisé los 26 que abriste hoy y ninguno referencia una issue del repositorio. Y las
funcionalidades que traen (pesca, minería, durabilidad, clima, ciclo día y noche, guild
wars, alquimia) no tienen issue abierta. Busqué: cero resultados para stamina,
fishing, durability, mining, weather, guild y spell.

Las 29 issues abiertas son de otra cosa: el editor de mapas por etapas, soporte mobile,
reconexión de WebSocket, el endpoint de ranking, y migraciones de schema.

O sea que esto es trabajo real sobre cosas que nadie pidió, y por lo tanto no se puede
acreditar a ninguna issue ni a ninguna campaña.

Lo que dice el CONTRIBUTING

Se subió hoy a las 03:02, unas horas antes de tu primera PR de esta tanda, así que es
probable que no lo hayas visto:

Comentá la issue que querés con un plan concreto, esperá a que te la asignen, y recién
ahí abrí la PR con Closes #N.

No es burocracia. Es exactamente lo que evita que dos personas escriban lo mismo, que es
lo que pasó siete veces acá.

Qué te propongo

Elegí una issue abierta que te interese, comentala con qué archivo vas a tocar y cómo
lo vas a verificar, y te la asigno. Con una PR enfocada sobre una issue asignada vas a
llegar más lejos que con cuarenta sin dueño.

Y decime qué querés hacer con las que ya están abiertas. Si hay alguna que sí corresponde
a una issue existente, decime cuál y la miro primero.

@Rodrigoue9

Copy link
Copy Markdown
Author

¡Hola @leocagli! Muchas gracias por el aviso y por cuidar nuestro tiempo. Tiene total sentido para mantener el repositorio limpio, coordinado y evitar duplicaciones.

Para avanzar de forma ordenada con las issues abiertas reales del repositorio, tenemos dos PRs listos y testeados que corresponden exactamente a problemas abiertos:

  1. Issue Etapa 1: registrar PNG subidos como graficos del motor y extender la paleta #6: Resuelta en el PR feat(world-builder): register uploaded graphics and extend palette schemas (#6) #111 (feat(world-builder): register uploaded graphics starting from index 1_000_000).
  2. Issue Falta api/src/jsons/npcs.json en el repositorio y el test de market no puede correr #83: Resuelta en el PR fix(api): add seed npcs.json to unblock main CI and market tests (#83) #122 (fix(api): add seed npcs.json to unblock market integration test).

Podemos cerrar los demás PRs especulativos sin ningún problema para que la cola del repositorio quede despejada y enfocada.

¿Podrías revisar el PR #111 y el PR #122 y asignarme la #6 y la #83 para avanzar con esos merges? ¡Muchas gracias por la buena onda y el proyecto! 🚀

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.

2 participants