-
Notifications
You must be signed in to change notification settings - Fork 19
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
Connect to 20Minutes, web socket on same port #34
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, merci pour l'ajout ! J'ai créé ce jeu il y a 6 ans pour y jouer avec des amis, et ça fait vraiment plaisir de voir que des gens l'aiment et y jouent encore 😄 !
J'ai noté quelques petites choses dans ta MR, mon plus gros concern étant surtout qu'il reste utilisable pour le plus grand nombre en local et avec un maximum de fonctionalitées 😕. Si tu as le temps de régler ça, alors ta MR est la plus que bienvenue. Encore merci a toi 😉!
"SOCKET_ADDR": "http://127.0.0.1", | ||
"SOCKET_PORT": 1337, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Si tu commentes ça, les autres joueurs ne pourront plus se connecter non ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ils pourront car la socket.io utilise le même port que le web "standard" cf https://github.com/tcoupin/mots/blob/master/game_files/motsFleches.js#L201
@@ -104,8 +104,50 @@ function parseGrid(callback, serverText) { | |||
nbWords: 0, | |||
cases: [] | |||
}; | |||
|
|||
eval(serverText); // Parse and evaluate js, generate variable gamedata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ce qui me dérange un peu avec cette approche, c'est qu'on enlève complètement la possibilité d'avoir les grilles du Metro pour le remplacer par celles du 20mn.
Plutôt que de perdre tout un provider, penses-tu que tu pourrais extraire les 2 logique dans 2 fichiers séparés ? Ainsi le gridManager
irait lire quel est le provider dans le fichier de config et chargerait le bon loader de grille selon qu'on veuille jouer Metro ou 20mn !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'avais compris que Metro ne fonctionnait plus...
On pourrait effectivement garder les 2 formats, je viens d'ailleurs de voir que NotreTemps utilisait aussi ce format : https://rcijeux.fr/drupal_game/notretemps/mfleches/grids/mfleches_1_1730.mfj
Potentiellement beaucoup de grille :)
@@ -179,7 +221,7 @@ function parseGrid(callback, serverText) { | |||
} | |||
} | |||
}; | |||
|
|||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Donc toute cette "logique Metro" se trouverait dans un autre fichier aussi...
@@ -12,27 +12,6 @@ define(function () { | |||
} | |||
|
|||
|
|||
function injectInGameInfoPanel() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Donc y a pas d'infos du jeu avec 20mn (j'ai pas encore pu tester 😃) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le soucis c'est que 20minutes a 2 panneaux dans ses grilles et que la div créée recouvre l'ensemble donc cache des cases. J'ai passé du temps à essayer de détecter des groupes de cases pour faire 2 div mais je n'ai pas réussi. Du coups j'ai préféré mettre les cases en gris et déplacé le panneau d'info sous les scores
/** Call when the express server has started */ | ||
async function onServerReady() { | ||
console.log('Express server listening on port ' + app.get('port')); | ||
|
||
var addresses = getLocalIpAddresses(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pourquoi enlever cette feature ? Est ce que t'héberges le jeu quelque part ? Si oui je veux l'adresse 😄 !!
Mais sinon d'un point de vue fonctionnalités, faut laisser aux gens la possibilité de le lancer en local. Je te proposerai plutôt encore une fois de laisser une option dans le fichier de configuration qui force une adresse et bypass ce choix par défaut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parce que je travaille avec Docker qui isole le processus et la partie réseau. De toute façon cette fonction n'était utile que pour la websocket qui est fusionée avec l'app expressjs. Et l'app expressjs écoutait déjà sur toutes les ips
Merci pour cette belle appli, j'avais besoin de ça pour continuer à se divertir avec les collègues pendant notre période de télétravail forcé (covid19...). |
@tcoupin on a un fork ici https://github.com/Drestin/mots qui permet de rejoindre une partie en cours si ça t'intéresse ! (il marche aussi sur téléphone maintenant). On voulait aussi intégrer d'autres sources maintenant donc ça tombe bien. |
Salut @hverlin ! C'est dommage d'avoir forke le projet et develope de nouvelles features dans ton coin ! Quand j'ai developpe ce jeu, je l'ai mis en ligne sur Github afin que tout le monde puisse s'amuser avec et participer a son developpement. Du coup je trouve dommage que tu developpes des features interessantes sur ton fork plutot que sur le projet original car tu les gardes pour toi au lieu de les paratager a tout les gens qui ont deja le jeu. De plus quand quelqu'un rajoute une fonctionalite comme c'est le cas avec @tcoupin, tu dois rebase ton fork et gerer les conflits plutot que de simplement pull la |
@benVigie Oui oui, nous voudrions intégrer les nouvelles fonctionalités ici. Nous avons encore pas mal de choses à corriger avant de pouvoir faire une bonne PR (en particulier le score ne marche pas encore correctement avec + de 5 utilisateurs donc on l'a temporairement enlevé). On voulait juste signaler qu'on avait commencé à corriger quelques uns des problèmes mentionnés ici :) |
Super cool @hverlin 😉 ! |
No description provided.