Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor flushevents #163

Closed
wants to merge 11 commits into from

Conversation

mind-ar
Copy link
Contributor

@mind-ar mind-ar commented Jun 11, 2024

Este PR es para mejorar el comportamiento del bucle flushEvent.

  • Hasta ahora, el bucle se ejecutaba con una frecuencia fija de 100ms y si el procesamiento de todos los eventos demoraba más de ese tiempo había un desfasaje entre el tiempo del flushEvent (>100ms) y el tiempo enviado a wollok (100ms). Ahora el tiempo del flushEvent se bajó a 17ms y se mantiene sincronizado el tiempo del loop con el tiempo enviado a wollok

  • se eliminó el método draw que estaba como evento de tiempo dentro de wollok y se lo agregó como método nativo dentro del bucle flushEvent

    • esto corrige el "efecto rebote" de los personajes cuando colisionaban con un objeto
  • Del lado del frontend se eliminó el loop automático de p5, ya que dibujaba en pantalla aunque no hayan datos nuevos.

mind-ar added 2 commits June 9, 2024 18:13
se reemplaza el loop de p5 que llama a draw()
por una llamada manual a draw() cada vez que se recibe un evento
@mind-ar mind-ar marked this pull request as draft June 11, 2024 00:21
@mind-ar mind-ar marked this pull request as ready for review June 11, 2024 23:25
Copy link
Contributor

@PalumboN PalumboN left a comment

Choose a reason for hiding this comment

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

Grande @mind-ar !!! 🥳 🎈 🐎

Dejé un par de comentarios contestando los tuyos y otras cosas para poder mergear todo

src/commands/run.ts Outdated Show resolved Hide resolved
src/commands/run.ts Outdated Show resolved Hide resolved
Comment on lines 198 to 207
muestras += 1


// cada 30 muestras se imprime por consola el tiempo promedio
// que tardó en procesar todos los eventos
if(muestras >= 30) {
logger.debug(`flushEvents: ${(tEvents / muestras).toFixed(2)} ms`)
muestras = 0
tEvents = 0
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Esto se podría mover a una función aparte, y dejar la llamada comentada acá. Cosa de descomentar esa línea y ya tener info en algún lado.

Después podemos iterar la técnica.

Ojo que el linter marca que hay espacios raros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Esto se podría mover a una función aparte, y dejar la llamada comentada acá. Cosa de descomentar esa línea y ya tener info en algún lado

si, pero si lo muevo tengo que hacer tEvent y muestras globales. Si te parece, lo hago

Copy link
Contributor

Choose a reason for hiding this comment

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

Lo que yo diría es de tener una función para profilear otra función. Y las variables que mencionás podrían estar "globales" en ese otro módulo.
Por ejemplo:

// profile.ts

let n = 0
let totalTime = 0
const hitCount = 30

export const profile = (f: () => any) => {
  const start = performance.now()
  f()
  const end = performance.now()
  totalTime += end - start
  n++
  if(n >= hitCount) {
     logger.debug(`flushEvents: ${(tEvents / muestras).toFixed(2)} ms`)
     n = 0
     totalTime = 0
  }
}

Y así se podría usar por ejemplo:

profile( () =>
        interpreter.send('flushEvents', gameSingleton, interpreter.reify(timer))
)

Que no es tan invasivo.

Después podemos ver si se puede mover a una annotation y controlarla globalmente para "prender y apagar" todos los profiles de una. Pero paso a paso.

Algo que también podría recibir la función es una etiqueta para tener contexto de lo que se está profilieando... ahora esa función tiene hardcodeado el flushEvents: en el string.

Sé que también hay otras funciones para loggear esto, podés ver por ejemplo el script cuando TS buildea el WRE: https://github.com/uqbar-project/wollok-ts/blob/master/scripts/buildWRE.ts

src/commands/run.ts Outdated Show resolved Hide resolved
src/commands/run.ts Show resolved Hide resolved
@@ -28,6 +28,7 @@ function setup() {
socket.on("updateSound", (data) => {
updateSound(data.soundInstances)
})
noLoop()
Copy link
Contributor

Choose a reason for hiding this comment

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

Esto para qué es? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

la librería que utilizamos para dibujar, ejecuta un ciclo continuo llamando a la función draw(), redibujando todo aunque no hayan nuevos datos que dibujar (no se recibió ninguna actualización del backend). Habría que probarlo en otro entorno, pero en mi PC ocasionaba bastante uso de CPU.
Con este cambio lo que hice fue que la llamada a draw() sea manual, sólo cuando recibimos un evento. la contra de esto es que cuando exista un nuevo mensaje aún no implementado, hay que acordarse de llamar a draw() si queremos que se actualice el juego

@mind-ar mind-ar requested a review from PalumboN June 18, 2024 22:41
@PalumboN
Copy link
Contributor

@mind-ar actualicé esta branch al master actual en #183

Queda pendiente lo del noLoop() que ahora hay que hacerlo en web-tools, abrí este issue para esto uqbar-project/wollok-web-tools#10

Cierro este PR, muchas gracias!! 🚀

@PalumboN PalumboN closed this Sep 16, 2024
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